Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor!: replace DataAccessor with Table in ProofExpr && remove input_length from ProofPlan::result_evaluate #366

Merged
merged 4 commits into from
Nov 14, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 42 additions & 7 deletions crates/proof-of-sql/src/base/database/table.rs
Original file line number Diff line number Diff line change
@@ -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;

Expand All @@ -18,18 +23,20 @@ pub enum TableError {
#[derive(Debug, Clone, Eq)]
pub struct Table<'a, S: Scalar> {
table: IndexMap<Identifier, Column<'a, S>>,
num_rows: usize,
}
impl<'a, S: Scalar> Table<'a, S> {
/// Creates a new [`Table`].
pub fn try_new(table: IndexMap<Identifier, Column<'a, S>>) -> Result<Self, TableError> {
if table.is_empty() {
return Ok(Self { table });
// `EmptyExec` should have one row for queries such as `SELECT 1`.
return Ok(Self { table, num_rows: 1 });
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let num_rows = table[0].len();
if table.values().any(|column| column.len() != num_rows) {
Err(TableError::ColumnLengthMismatch)
} else {
Ok(Self { table })
Ok(Self { table, num_rows })
}
}
/// Creates a new [`Table`].
Expand All @@ -38,15 +45,43 @@ impl<'a, S: Scalar> Table<'a, S> {
) -> Result<Self, TableError> {
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<ColumnRef>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer for this to be a provided method on DataAccessor, since Table doesn't really need to know about the concept of DataAccessor to be useful.

table_ref: TableRef,
accessor: &'a dyn DataAccessor<S>,
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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the proper behavior to create a Table with no columns, but the proper length?

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 {
self.table.len()
}
/// Number of rows in the table. For an empty table, this will return `None`.
/// Number of rows in the table.
#[must_use]
pub fn num_rows(&self) -> Option<usize> {
(!self.table.is_empty()).then(|| self.table[0].len())
pub fn num_rows(&self) -> usize {
self.num_rows
}
/// Whether the table has no columns.
#[must_use]
Expand Down
2 changes: 1 addition & 1 deletion crates/proof-of-sql/src/base/database/table_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use proof_of_sql_parser::{
fn we_can_create_a_table_with_no_columns() {
let table = Table::<TestScalar>::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() {
Expand Down
7 changes: 1 addition & 6 deletions crates/proof-of-sql/src/base/database/table_test_accessor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,12 +111,7 @@ impl<CP: CommitmentEvaluationProof> 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()
.unwrap_or(0)
self.tables.get(&table_ref).unwrap().0.num_rows()
}
///
/// # Panics
Expand Down
3 changes: 1 addition & 2 deletions crates/proof-of-sql/src/sql/proof/proof_plan.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,9 @@ pub trait ProofPlan: Debug + Send + Sync + ProverEvaluate {

#[enum_dispatch::enum_dispatch(DynProofPlan)]
pub trait ProverEvaluate {
/// Evaluate the query and modify `FirstRoundBuilder` to track the result of the query.
/// Evaluate the query and return the result.
fn result_evaluate<'a, S: Scalar>(
&self,
input_length: usize,
alloc: &'a Bump,
accessor: &'a dyn DataAccessor<S>,
) -> Vec<Column<'a, S>>;
Expand Down
2 changes: 1 addition & 1 deletion crates/proof-of-sql/src/sql/proof/query_proof.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ impl<CP: CommitmentEvaluationProof> QueryProof<CP> {
let alloc = Bump::new();

// Evaluate query result
let result_cols = expr.result_evaluate(range_length, &alloc, accessor);
let result_cols = expr.result_evaluate(&alloc, accessor);
let output_length = result_cols.first().map_or(0, Column::len);
let provable_result = ProvableQueryResult::new(output_length as u64, &result_cols);

Expand Down
4 changes: 0 additions & 4 deletions crates/proof-of-sql/src/sql/proof/query_proof_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ impl Default for TrivialTestProofPlan {
impl ProverEvaluate for TrivialTestProofPlan {
fn result_evaluate<'a, S: Scalar>(
&self,
_input_length: usize,
alloc: &'a Bump,
_accessor: &'a dyn DataAccessor<S>,
) -> Vec<Column<'a, S>> {
Expand Down Expand Up @@ -212,7 +211,6 @@ impl Default for SquareTestProofPlan {
impl ProverEvaluate for SquareTestProofPlan {
fn result_evaluate<'a, S: Scalar>(
&self,
_table_length: usize,
alloc: &'a Bump,
_accessor: &'a dyn DataAccessor<S>,
) -> Vec<Column<'a, S>> {
Expand Down Expand Up @@ -390,7 +388,6 @@ impl Default for DoubleSquareTestProofPlan {
impl ProverEvaluate for DoubleSquareTestProofPlan {
fn result_evaluate<'a, S: Scalar>(
&self,
_input_length: usize,
alloc: &'a Bump,
_accessor: &'a dyn DataAccessor<S>,
) -> Vec<Column<'a, S>> {
Expand Down Expand Up @@ -598,7 +595,6 @@ struct ChallengeTestProofPlan {}
impl ProverEvaluate for ChallengeTestProofPlan {
fn result_evaluate<'a, S: Scalar>(
&self,
_input_length: usize,
_alloc: &'a Bump,
_accessor: &'a dyn DataAccessor<S>,
) -> Vec<Column<'a, S>> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ pub(super) struct EmptyTestQueryExpr {
impl ProverEvaluate for EmptyTestQueryExpr {
fn result_evaluate<'a, S: Scalar>(
&self,
_input_length: usize,
alloc: &'a Bump,
_accessor: &'a dyn DataAccessor<S>,
) -> Vec<Column<'a, S>> {
Expand Down
15 changes: 7 additions & 8 deletions crates/proof-of-sql/src/sql/proof_exprs/add_subtract_expr.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use super::{add_subtract_columns, scale_and_add_subtract_eval, DynProofExpr, ProofExpr};
use crate::{
base::{
database::{try_add_subtract_column_types, Column, ColumnRef, ColumnType, DataAccessor},
database::{try_add_subtract_column_types, Column, ColumnRef, ColumnType, Table},
map::{IndexMap, IndexSet},
proof::ProofError,
scalar::Scalar,
Expand Down Expand Up @@ -45,12 +45,11 @@ impl ProofExpr for AddSubtractExpr {

fn result_evaluate<'a, S: Scalar>(
&self,
table_length: usize,
alloc: &'a Bump,
accessor: &'a dyn DataAccessor<S>,
table: &Table<'a, S>,
) -> Column<'a, S> {
let lhs_column: Column<'a, S> = self.lhs.result_evaluate(table_length, alloc, accessor);
let rhs_column: Column<'a, S> = self.rhs.result_evaluate(table_length, alloc, accessor);
let lhs_column: Column<'a, S> = self.lhs.result_evaluate(alloc, table);
let rhs_column: Column<'a, S> = self.rhs.result_evaluate(alloc, table);
Column::Scalar(add_subtract_columns(
lhs_column,
rhs_column,
Expand All @@ -70,10 +69,10 @@ impl ProofExpr for AddSubtractExpr {
&self,
builder: &mut FinalRoundBuilder<'a, S>,
alloc: &'a Bump,
accessor: &'a dyn DataAccessor<S>,
table: &Table<'a, S>,
) -> Column<'a, S> {
let lhs_column: Column<'a, S> = self.lhs.prover_evaluate(builder, alloc, accessor);
let rhs_column: Column<'a, S> = self.rhs.prover_evaluate(builder, alloc, accessor);
let lhs_column: Column<'a, S> = self.lhs.prover_evaluate(builder, alloc, table);
let rhs_column: Column<'a, S> = self.rhs.prover_evaluate(builder, alloc, table);
Column::Scalar(add_subtract_columns(
lhs_column,
rhs_column,
Expand Down
21 changes: 12 additions & 9 deletions crates/proof-of-sql/src/sql/proof_exprs/add_subtract_expr_test.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
use crate::{
base::{
commitment::InnerProductProof,
database::{owned_table_utility::*, Column, OwnedTableTestAccessor},
database::{
owned_table_utility::*, table_utility::*, Column, OwnedTableTestAccessor,
TableTestAccessor,
},
scalar::{test_scalar::TestScalar, Curve25519Scalar},
},
sql::{
Expand Down Expand Up @@ -306,20 +309,20 @@ fn we_can_query_random_tables_using_a_non_zero_offset() {
// b + a - 1
#[test]
fn we_can_compute_the_correct_output_of_an_add_subtract_expr_using_result_evaluate() {
let data = owned_table([
smallint("a", [1_i16, 2, 3, 4]),
int("b", [0_i32, 1, 0, 1]),
varchar("d", ["ab", "t", "efg", "g"]),
bigint("c", [0_i64, 2, 2, 0]),
let alloc = Bump::new();
let data = table([
borrowed_smallint("a", [1_i16, 2, 3, 4], &alloc),
borrowed_int("b", [0_i32, 1, 0, 1], &alloc),
borrowed_varchar("d", ["ab", "t", "efg", "g"], &alloc),
borrowed_bigint("c", [0_i64, 2, 2, 0], &alloc),
]);
let t = "sxt.t".parse().unwrap();
let accessor = OwnedTableTestAccessor::<InnerProductProof>::new_from_table(t, data, 0, ());
let accessor = TableTestAccessor::<InnerProductProof>::new_from_table(t, data.clone(), 0, ());
let add_subtract_expr: DynProofExpr = add(
column(t, "b", &accessor),
subtract(column(t, "a", &accessor), const_bigint(1)),
);
let alloc = Bump::new();
let res = add_subtract_expr.result_evaluate(4, &alloc, &accessor);
let res = add_subtract_expr.result_evaluate(&alloc, &data);
let expected_res_scalar = [0, 2, 2, 4]
.iter()
.map(|v| Curve25519Scalar::from(*v))
Expand Down
11 changes: 5 additions & 6 deletions crates/proof-of-sql/src/sql/proof_exprs/aggregate_expr.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use super::{DynProofExpr, ProofExpr};
use crate::{
base::{
database::{Column, ColumnRef, ColumnType, DataAccessor},
database::{Column, ColumnRef, ColumnType, Table},
map::{IndexMap, IndexSet},
proof::ProofError,
scalar::Scalar,
Expand Down Expand Up @@ -45,21 +45,20 @@ impl ProofExpr for AggregateExpr {
#[tracing::instrument(name = "AggregateExpr::result_evaluate", level = "debug", skip_all)]
fn result_evaluate<'a, S: Scalar>(
&self,
table_length: usize,
alloc: &'a Bump,
accessor: &'a dyn DataAccessor<S>,
table: &Table<'a, S>,
) -> Column<'a, S> {
self.expr.result_evaluate(table_length, alloc, accessor)
self.expr.result_evaluate(alloc, table)
}

#[tracing::instrument(name = "AggregateExpr::prover_evaluate", level = "debug", skip_all)]
fn prover_evaluate<'a, S: Scalar>(
&self,
builder: &mut FinalRoundBuilder<'a, S>,
alloc: &'a Bump,
accessor: &'a dyn DataAccessor<S>,
table: &Table<'a, S>,
) -> Column<'a, S> {
self.expr.prover_evaluate(builder, alloc, accessor)
self.expr.prover_evaluate(builder, alloc, table)
}

fn verifier_evaluate<S: Scalar>(
Expand Down
17 changes: 8 additions & 9 deletions crates/proof-of-sql/src/sql/proof_exprs/and_expr.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use super::{DynProofExpr, ProofExpr};
use crate::{
base::{
database::{Column, ColumnRef, ColumnType, DataAccessor},
database::{Column, ColumnRef, ColumnType, Table},
map::{IndexMap, IndexSet},
proof::ProofError,
scalar::Scalar,
Expand Down Expand Up @@ -43,26 +43,25 @@ impl ProofExpr for AndExpr {
#[tracing::instrument(name = "AndExpr::result_evaluate", level = "debug", skip_all)]
fn result_evaluate<'a, S: Scalar>(
&self,
table_length: usize,
alloc: &'a Bump,
accessor: &'a dyn DataAccessor<S>,
table: &Table<'a, S>,
) -> Column<'a, S> {
let lhs_column: Column<'a, S> = self.lhs.result_evaluate(table_length, alloc, accessor);
let rhs_column: Column<'a, S> = self.rhs.result_evaluate(table_length, alloc, accessor);
let lhs_column: Column<'a, S> = self.lhs.result_evaluate(alloc, table);
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_length, |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)]
fn prover_evaluate<'a, S: Scalar>(
&self,
builder: &mut FinalRoundBuilder<'a, S>,
alloc: &'a Bump,
accessor: &'a dyn DataAccessor<S>,
table: &Table<'a, S>,
) -> Column<'a, S> {
let lhs_column: Column<'a, S> = self.lhs.prover_evaluate(builder, alloc, accessor);
let rhs_column: Column<'a, S> = self.rhs.prover_evaluate(builder, alloc, accessor);
let lhs_column: Column<'a, S> = self.lhs.prover_evaluate(builder, alloc, table);
let rhs_column: Column<'a, S> = self.rhs.prover_evaluate(builder, alloc, table);
let lhs = lhs_column.as_boolean().expect("lhs is not boolean");
let rhs = rhs_column.as_boolean().expect("rhs is not boolean");
let n = lhs.len();
Expand Down
21 changes: 12 additions & 9 deletions crates/proof-of-sql/src/sql/proof_exprs/and_expr_test.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
use crate::{
base::{
commitment::InnerProductProof,
database::{owned_table_utility::*, Column, OwnedTableTestAccessor},
database::{
owned_table_utility::*, table_utility::*, Column, OwnedTableTestAccessor,
TableTestAccessor,
},
scalar::test_scalar::TestScalar,
},
sql::{
Expand Down Expand Up @@ -153,20 +156,20 @@ fn we_can_query_random_tables_using_a_non_zero_offset() {

#[test]
fn we_can_compute_the_correct_output_of_an_and_expr_using_result_evaluate() {
let data = owned_table([
bigint("a", [1, 2, 3, 4]),
bigint("b", [0, 1, 0, 1]),
varchar("d", ["ab", "t", "efg", "g"]),
bigint("c", [0, 2, 2, 0]),
let alloc = Bump::new();
let data = table([
borrowed_bigint("a", [1, 2, 3, 4], &alloc),
borrowed_bigint("b", [0, 1, 0, 1], &alloc),
borrowed_varchar("d", ["ab", "t", "efg", "g"], &alloc),
borrowed_bigint("c", [0, 2, 2, 0], &alloc),
]);
let t = "sxt.t".parse().unwrap();
let accessor = OwnedTableTestAccessor::<InnerProductProof>::new_from_table(t, data, 0, ());
let accessor = TableTestAccessor::<InnerProductProof>::new_from_table(t, data.clone(), 0, ());
let and_expr: DynProofExpr = and(
equal(column(t, "b", &accessor), const_int128(1)),
equal(column(t, "d", &accessor), const_varchar("t")),
);
let alloc = Bump::new();
let res = and_expr.result_evaluate(4, &alloc, &accessor);
let res = and_expr.result_evaluate(&alloc, &data);
let expected_res = Column::Boolean(&[false, true, false, false]);
assert_eq!(res, expected_res);
}
Loading
Loading