From 36a8eba2692600082d4c7fda54c09442180e5632 Mon Sep 17 00:00:00 2001 From: Ian Joiner <14581281+iajoiner@users.noreply.github.com> Date: Wed, 13 Nov 2024 10:40:19 -0500 Subject: [PATCH] fix: address review comments --- .../proof-of-sql/src/base/database/table.rs | 40 +++++++++++++++++-- .../src/base/database/table_test.rs | 2 +- .../src/base/database/table_test_accessor.rs | 6 +-- .../src/sql/proof_exprs/and_expr.rs | 4 +- .../src/sql/proof_exprs/equals_expr.rs | 6 +-- .../src/sql/proof_exprs/or_expr.rs | 7 +--- .../src/sql/proof_plans/filter_exec.rs | 14 ++----- .../filter_exec_test_dishonest_prover.rs | 14 ++----- .../src/sql/proof_plans/group_by_exec.rs | 34 ++-------------- .../src/sql/proof_plans/projection_exec.rs | 14 ++----- .../src/sql/proof_plans/table_exec.rs | 1 - 11 files changed, 58 insertions(+), 84 deletions(-) diff --git a/crates/proof-of-sql/src/base/database/table.rs b/crates/proof-of-sql/src/base/database/table.rs index d48701d93..29aebaf36 100644 --- a/crates/proof-of-sql/src/base/database/table.rs +++ b/crates/proof-of-sql/src/base/database/table.rs @@ -1,5 +1,10 @@ -use super::Column; -use crate::base::{map::IndexMap, scalar::Scalar}; +use super::{Column, ColumnRef, DataAccessor, TableRef}; +use crate::base::{ + map::{IndexMap, IndexSet}, + scalar::Scalar, +}; +use alloc::vec; +use bumpalo::Bump; use proof_of_sql_parser::Identifier; use snafu::Snafu; @@ -24,7 +29,8 @@ impl<'a, S: Scalar> Table<'a, S> { /// Creates a new [`Table`]. pub fn try_new(table: IndexMap>) -> Result { if table.is_empty() { - return Ok(Self { table, num_rows: 0 }); + // `EmptyExec` should have one row for queries such as `SELECT 1`. + return Ok(Self { table, num_rows: 1 }); } let num_rows = table[0].len(); if table.values().any(|column| column.len() != num_rows) { @@ -39,6 +45,34 @@ impl<'a, S: Scalar> Table<'a, S> { ) -> Result { Self::try_new(IndexMap::from_iter(iter)) } + /// Creates a new [`Table`] from a [`DataAccessor`], [`TableRef`] and [`ColumnRef`]s. + /// + /// Columns are retrieved from the [`DataAccessor`] using the provided [`ColumnRef`]s. + /// # Panics + /// Missing columns or column length mismatches can occur if the accessor doesn't + /// contain the necessary columns. In practice, this should not happen. + pub(crate) fn from_columns( + column_refs: &IndexSet, + table_ref: TableRef, + accessor: &'a dyn DataAccessor, + alloc: &'a Bump, + ) -> Self { + if column_refs.is_empty() { + // TODO: Currently we have to have non-empty column references to have a non-empty table + // to evaluate `ProofExpr`s on. Once we restrict [`DataAccessor`] to [`TableExec`] + // and use input `DynProofPlan`s we should no longer need this. + let input_length = accessor.get_length(table_ref); + let bogus_vec = vec![true; input_length]; + let bogus_col = Column::Boolean(alloc.alloc_slice_copy(&bogus_vec)); + Table::<'a, S>::try_from_iter(core::iter::once(("bogus".parse().unwrap(), bogus_col))) + } else { + Table::<'a, S>::try_from_iter(column_refs.into_iter().map(|column_ref| { + let column = accessor.get_column(*column_ref); + (column_ref.column_id(), column) + })) + } + .expect("Failed to create table from column references") + } /// Number of columns in the table. #[must_use] pub fn num_columns(&self) -> usize { diff --git a/crates/proof-of-sql/src/base/database/table_test.rs b/crates/proof-of-sql/src/base/database/table_test.rs index 515de66fa..179dff057 100644 --- a/crates/proof-of-sql/src/base/database/table_test.rs +++ b/crates/proof-of-sql/src/base/database/table_test.rs @@ -13,7 +13,7 @@ use proof_of_sql_parser::{ fn we_can_create_a_table_with_no_columns() { let table = Table::::try_new(IndexMap::default()).unwrap(); assert_eq!(table.num_columns(), 0); - assert_eq!(table.num_rows(), None); + assert_eq!(table.num_rows(), 1); } #[test] fn we_can_create_an_empty_table() { diff --git a/crates/proof-of-sql/src/base/database/table_test_accessor.rs b/crates/proof-of-sql/src/base/database/table_test_accessor.rs index edebb583f..8053fa2d8 100644 --- a/crates/proof-of-sql/src/base/database/table_test_accessor.rs +++ b/crates/proof-of-sql/src/base/database/table_test_accessor.rs @@ -111,11 +111,7 @@ impl MetadataAccessor for TableTestAccessor<'_, C /// /// Will panic if the `table_ref` is not found in `self.tables`, indicating that an invalid reference was provided. fn get_length(&self, table_ref: TableRef) -> usize { - self.tables - .get(&table_ref) - .unwrap() - .0 - .num_rows() + self.tables.get(&table_ref).unwrap().0.num_rows() } /// /// # Panics diff --git a/crates/proof-of-sql/src/sql/proof_exprs/and_expr.rs b/crates/proof-of-sql/src/sql/proof_exprs/and_expr.rs index 945b2a8c3..2159c7bf6 100644 --- a/crates/proof-of-sql/src/sql/proof_exprs/and_expr.rs +++ b/crates/proof-of-sql/src/sql/proof_exprs/and_expr.rs @@ -50,9 +50,7 @@ impl ProofExpr for AndExpr { let rhs_column: Column<'a, S> = self.rhs.result_evaluate(alloc, table); let lhs = lhs_column.as_boolean().expect("lhs is not boolean"); let rhs = rhs_column.as_boolean().expect("rhs is not boolean"); - Column::Boolean( - alloc.alloc_slice_fill_with(table.num_rows(), |i| lhs[i] && rhs[i]), - ) + Column::Boolean(alloc.alloc_slice_fill_with(table.num_rows(), |i| lhs[i] && rhs[i])) } #[tracing::instrument(name = "AndExpr::prover_evaluate", level = "debug", skip_all)] diff --git a/crates/proof-of-sql/src/sql/proof_exprs/equals_expr.rs b/crates/proof-of-sql/src/sql/proof_exprs/equals_expr.rs index 6491e2a42..213e88c18 100644 --- a/crates/proof-of-sql/src/sql/proof_exprs/equals_expr.rs +++ b/crates/proof-of-sql/src/sql/proof_exprs/equals_expr.rs @@ -51,11 +51,7 @@ impl ProofExpr for EqualsExpr { let rhs_scale = self.rhs.data_type().scale().unwrap_or(0); let res = scale_and_subtract(alloc, lhs_column, rhs_column, lhs_scale, rhs_scale, true) .expect("Failed to scale and subtract"); - Column::Boolean(result_evaluate_equals_zero( - table.num_rows(), - alloc, - res, - )) + Column::Boolean(result_evaluate_equals_zero(table.num_rows(), alloc, res)) } #[tracing::instrument(name = "EqualsExpr::prover_evaluate", level = "debug", skip_all)] diff --git a/crates/proof-of-sql/src/sql/proof_exprs/or_expr.rs b/crates/proof-of-sql/src/sql/proof_exprs/or_expr.rs index 3979326af..a8336a372 100644 --- a/crates/proof-of-sql/src/sql/proof_exprs/or_expr.rs +++ b/crates/proof-of-sql/src/sql/proof_exprs/or_expr.rs @@ -48,12 +48,7 @@ impl ProofExpr for OrExpr { let rhs_column: Column<'a, S> = self.rhs.result_evaluate(alloc, table); let lhs = lhs_column.as_boolean().expect("lhs is not boolean"); let rhs = rhs_column.as_boolean().expect("rhs is not boolean"); - Column::Boolean(result_evaluate_or( - table.num_rows(), - alloc, - lhs, - rhs, - )) + Column::Boolean(result_evaluate_or(table.num_rows(), alloc, lhs, rhs)) } #[tracing::instrument(name = "OrExpr::prover_evaluate", level = "debug", skip_all)] diff --git a/crates/proof-of-sql/src/sql/proof_plans/filter_exec.rs b/crates/proof-of-sql/src/sql/proof_plans/filter_exec.rs index acec0a02a..3cd87dac0 100644 --- a/crates/proof-of-sql/src/sql/proof_plans/filter_exec.rs +++ b/crates/proof-of-sql/src/sql/proof_plans/filter_exec.rs @@ -143,11 +143,8 @@ impl ProverEvaluate for FilterExec { accessor: &'a dyn DataAccessor, ) -> Vec> { let column_refs = self.get_column_references(); - let used_table = Table::<'a, S>::try_from_iter(column_refs.iter().map(|column_ref| { - let column = accessor.get_column(*column_ref); - (column_ref.column_id(), column) - })) - .expect("Failed to create table from column references"); + let used_table = + Table::<'a, S>::from_columns(&column_refs, self.table.table_ref, accessor, alloc); // 1. selection let selection_column: Column<'a, S> = self.where_clause.result_evaluate(alloc, &used_table); let selection = selection_column @@ -179,11 +176,8 @@ impl ProverEvaluate for FilterExec { accessor: &'a dyn DataAccessor, ) -> Vec> { let column_refs = self.get_column_references(); - let used_table = Table::<'a, S>::try_from_iter(column_refs.iter().map(|column_ref| { - let column = accessor.get_column(*column_ref); - (column_ref.column_id(), column) - })) - .expect("Failed to create table from column references"); + let used_table = + Table::<'a, S>::from_columns(&column_refs, self.table.table_ref, accessor, alloc); // 1. selection let selection_column: Column<'a, S> = self.where_clause diff --git a/crates/proof-of-sql/src/sql/proof_plans/filter_exec_test_dishonest_prover.rs b/crates/proof-of-sql/src/sql/proof_plans/filter_exec_test_dishonest_prover.rs index 234b22658..56f7fac9e 100644 --- a/crates/proof-of-sql/src/sql/proof_plans/filter_exec_test_dishonest_prover.rs +++ b/crates/proof-of-sql/src/sql/proof_plans/filter_exec_test_dishonest_prover.rs @@ -39,11 +39,8 @@ impl ProverEvaluate for DishonestFilterExec { accessor: &'a dyn DataAccessor, ) -> Vec> { let column_refs = self.get_column_references(); - let used_table = Table::<'a, S>::try_from_iter(column_refs.iter().map(|column_ref| { - let column = accessor.get_column(*column_ref); - (column_ref.column_id(), column) - })) - .expect("Failed to create table from column references"); + let used_table = + Table::<'a, S>::from_columns(&column_refs, self.table.table_ref, accessor, alloc); // 1. selection let selection_column: Column<'a, S> = self.where_clause.result_evaluate(alloc, &used_table); let selection = selection_column @@ -78,11 +75,8 @@ impl ProverEvaluate for DishonestFilterExec { accessor: &'a dyn DataAccessor, ) -> Vec> { let column_refs = self.get_column_references(); - let used_table = Table::<'a, S>::try_from_iter(column_refs.iter().map(|column_ref| { - let column = accessor.get_column(*column_ref); - (column_ref.column_id(), column) - })) - .expect("Failed to create table from column references"); + let used_table = + Table::<'a, S>::from_columns(&column_refs, self.table.table_ref, accessor, alloc); // 1. selection let selection_column: Column<'a, S> = self.where_clause diff --git a/crates/proof-of-sql/src/sql/proof_plans/group_by_exec.rs b/crates/proof-of-sql/src/sql/proof_plans/group_by_exec.rs index 66f3beade..a3ad2e8a0 100644 --- a/crates/proof-of-sql/src/sql/proof_plans/group_by_exec.rs +++ b/crates/proof-of-sql/src/sql/proof_plans/group_by_exec.rs @@ -202,21 +202,8 @@ impl ProverEvaluate for GroupByExec { accessor: &'a dyn DataAccessor, ) -> Vec> { let column_refs = self.get_column_references(); - let used_table = if column_refs.is_empty() { - //TODO: Currently we have to have non-empty column references to have a non-empty table - // to evaluate `ProofExpr`s on. Once we restrict [`DataAccessor`] to [`TableExec`] - // and use input `DynProofPlan`s we should no longer need this. - let input_length = accessor.get_length(self.table.table_ref); - let bogus_vec = vec![true; input_length]; - let bogus_col = Column::Boolean(alloc.alloc_slice_copy(&bogus_vec)); - Table::<'a, S>::try_from_iter(core::iter::once(("bogus".parse().unwrap(), bogus_col))) - } else { - Table::<'a, S>::try_from_iter(column_refs.iter().map(|column_ref| { - let column = accessor.get_column(*column_ref); - (column_ref.column_id(), column) - })) - } - .expect("Failed to create table from column references"); + let used_table = + Table::<'a, S>::from_columns(&column_refs, self.table.table_ref, accessor, alloc); // 1. selection let selection_column: Column<'a, S> = self.where_clause.result_evaluate(alloc, &used_table); @@ -264,21 +251,8 @@ impl ProverEvaluate for GroupByExec { accessor: &'a dyn DataAccessor, ) -> Vec> { let column_refs = self.get_column_references(); - let used_table = if column_refs.is_empty() { - //TODO: Currently we have to have non-empty column references to have a non-empty table - // to evaluate `ProofExpr`s on. Once we restrict [`DataAccessor`] to [`TableExec`] - // and use input `DynProofPlan`s we should no longer need this. - let input_length = accessor.get_length(self.table.table_ref); - let bogus_vec = vec![true; input_length]; - let bogus_col = Column::Boolean(alloc.alloc_slice_copy(&bogus_vec)); - Table::<'a, S>::try_from_iter(core::iter::once(("bogus".parse().unwrap(), bogus_col))) - } else { - Table::<'a, S>::try_from_iter(column_refs.iter().map(|column_ref| { - let column = accessor.get_column(*column_ref); - (column_ref.column_id(), column) - })) - } - .expect("Failed to create table from column references"); + let used_table = + Table::<'a, S>::from_columns(&column_refs, self.table.table_ref, accessor, alloc); // 1. selection let selection_column: Column<'a, S> = self.where_clause diff --git a/crates/proof-of-sql/src/sql/proof_plans/projection_exec.rs b/crates/proof-of-sql/src/sql/proof_plans/projection_exec.rs index fe0ff506e..326158c76 100644 --- a/crates/proof-of-sql/src/sql/proof_plans/projection_exec.rs +++ b/crates/proof-of-sql/src/sql/proof_plans/projection_exec.rs @@ -91,11 +91,8 @@ impl ProverEvaluate for ProjectionExec { accessor: &'a dyn DataAccessor, ) -> Vec> { let column_refs = self.get_column_references(); - let used_table = Table::<'a, S>::try_from_iter(column_refs.iter().map(|column_ref| { - let column = accessor.get_column(*column_ref); - (column_ref.column_id(), column) - })) - .expect("Failed to create table from column references"); + let used_table = + Table::<'a, S>::from_columns(&column_refs, self.table.table_ref, accessor, alloc); let columns: Vec<_> = self .aliased_results .iter() @@ -119,11 +116,8 @@ impl ProverEvaluate for ProjectionExec { accessor: &'a dyn DataAccessor, ) -> Vec> { let column_refs = self.get_column_references(); - let used_table = Table::<'a, S>::try_from_iter(column_refs.iter().map(|column_ref| { - let column = accessor.get_column(*column_ref); - (column_ref.column_id(), column) - })) - .expect("Failed to create table from column references"); + let used_table = + Table::<'a, S>::from_columns(&column_refs, self.table.table_ref, accessor, alloc); // 1. Evaluate result expressions let res: Vec<_> = self .aliased_results diff --git a/crates/proof-of-sql/src/sql/proof_plans/table_exec.rs b/crates/proof-of-sql/src/sql/proof_plans/table_exec.rs index ce8a8f495..4a61f0cd9 100644 --- a/crates/proof-of-sql/src/sql/proof_plans/table_exec.rs +++ b/crates/proof-of-sql/src/sql/proof_plans/table_exec.rs @@ -85,7 +85,6 @@ impl ProverEvaluate for TableExec { #[tracing::instrument(name = "TableExec::result_evaluate", level = "debug", skip_all)] fn result_evaluate<'a, S: Scalar>( &self, - _input_length: usize, _alloc: &'a Bump, accessor: &'a dyn DataAccessor, ) -> Vec> {