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

iajoiner · 2024-11-11T19:57:12Z

Please be sure to look over the pull request guidelines here: https://github.com/spaceandtimelabs/sxt-proof-of-sql/blob/main/CONTRIBUTING.md#submit-pr.

Please go through the following checklist

The PR title and commit messages adhere to guidelines here: https://github.com/spaceandtimelabs/sxt-proof-of-sql/blob/main/CONTRIBUTING.md. In particular ! is used if and only if at least one breaking change has been introduced.
I have run the ci check script with source scripts/run_ci_checks.sh.

Rationale for this change

We would like to evaluate ProofExpr on Table similar to how datafusion evaluates PhysicalExpr on RecordBatch. It is better to use a Table than to use a DataAccessor since the latter has access to more than one table while a ProofExpr shouldn't be able to be evaluated on multiple tables (CTEs etc count as tables). Moreover it is absurd to have a single input_length in a ProofPlan since it can have multiple sources.

What changes are included in this PR?

remove arg input_length from ProofPlan::result_evaluate
remove arg table_length from ProofExpr::result_evaluate
replace arg accessor from ProofPlan::result_evaluate with table: &Table<'a, S>
replace arg accessor from ProofPlan::prover_evaluate with table: &Table<'a, S>

Are these changes tested?

Yes.

JayWhite2357 · 2024-11-13T05:10:38Z

crates/proof-of-sql/src/sql/proof_exprs/and_expr.rs

        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().unwrap_or(0), |i| lhs[i] && rhs[i]),


I'm not convinced that 0 is the correct default.
If table has no columns, it feels like lhs_column and rhs_column will have to be literals.
Eventually, we should get a proper solution with ColumnarValue, but for now, I think it makes more sense to pull the length from the lhs_column and rhs_column (and assert that the lengths equal).

This is also what is done in prover_evaluate.

JayWhite2357 · 2024-11-13T05:13:54Z

crates/proof-of-sql/src/sql/proof_exprs/column_expr.rs

+    ///
+    /// Will panic if the column is not found. Shouldn't happen in practice since
+    /// code in `sql/parse` should have already checked that the column exists.
+    pub fn get_column<'a, S: Scalar>(&self, table: &Table<'a, S>) -> Column<'a, S> {


With a few exceptions, the get_ prefix is not used for getters in Rust code.

JayWhite2357 · 2024-11-13T05:17:28Z

crates/proof-of-sql/src/sql/proof_exprs/literal_expr.rs

    ) -> Column<'a, S> {
-        Column::from_literal_with_length(&self.value, table_length, alloc)
+        Column::from_literal_with_length(&self.value, table.num_rows().unwrap_or(0), alloc)


This is the only spot where a default makes sense to me. If there are no columns, it's not obvious to me what the proper behavior here is. Probably 1 instead of 0.

Also, please add a test to cover this case.

JayWhite2357 · 2024-11-13T05:19:44Z

crates/proof-of-sql/src/sql/proof_plans/filter_exec.rs

        alloc: &'a Bump,
        accessor: &'a dyn DataAccessor<S>,
    ) -> Vec<Column<'a, S>> {
+        let column_refs = self.get_column_references();
+        let used_table = Table::<'a, S>::try_from_iter(column_refs.iter().map(|column_ref| {


NIT:

Suggested change

let used_table = Table::<'a, S>::try_from_iter(column_refs.iter().map(|column_ref| {

let used_table = Table::<'a, S>::try_from_iter(column_refs.into_iter().map(|column_ref| {

JayWhite2357 · 2024-11-13T05:21:06Z

crates/proof-of-sql/src/sql/proof_plans/filter_exec.rs

+        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");


Might be nice to put this in a function since it is repeated multiple times.

JayWhite2357 · 2024-11-13T05:25:15Z

crates/proof-of-sql/src/sql/proof_plans/group_by_exec.rs

+            //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.


Might be wise to add row_count as a field of Table. This would fix this issue as well as the comments I have above. See RecordBatch.

…ve `table_length` from `ProofPlan::result_evaluate`

JayWhite2357 · 2024-11-13T21:08:14Z

crates/proof-of-sql/src/base/database/table.rs

 }
 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 });


This default behavior doesn't really make sense.
This is what arrow does:
https://github.com/apache/arrow-rs/blob/3ee5048c8ea3aa531d111afe33d0a3551eabcd84/arrow-array/src/record_batch.rs#L86
https://github.com/apache/arrow-rs/blob/3ee5048c8ea3aa531d111afe33d0a3551eabcd84/arrow-array/src/record_batch.rs#L501

JayWhite2357 · 2024-11-13T21:41:50Z

crates/proof-of-sql/src/base/database/table.rs

+    /// # 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(


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.

JayWhite2357 · 2024-11-13T21:43:28Z

crates/proof-of-sql/src/base/database/table.rs

+            // 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)))


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

github-actions · 2024-11-14T13:32:14Z

🎉 This PR is included in version 0.45.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

iajoiner force-pushed the refactor/rem-acc branch 6 times, most recently from f5cfcae to 33c745b Compare November 12, 2024 20:09

iajoiner requested a review from JayWhite2357 November 12, 2024 20:11

iajoiner force-pushed the refactor/rem-acc branch from f8ce3b5 to ed5d485 Compare November 12, 2024 20:35

iajoiner marked this pull request as ready for review November 12, 2024 20:36

iajoiner force-pushed the refactor/rem-acc branch from ed5d485 to 30cf15b Compare November 12, 2024 20:36

iajoiner changed the title ~~refactor!: replace DataAccessor with Table in ProofExpr~~ refactor!: replace DataAccessor with Table in ProofExpr && remove table_length from ProofPlan::result_evaluate Nov 12, 2024

iajoiner changed the title ~~refactor!: replace DataAccessor with Table in ProofExpr && remove table_length from ProofPlan::result_evaluate~~ refactor!: replace DataAccessor with Table in ProofExpr && remove table_length from ProofPlan::result_evaluate Nov 12, 2024

iajoiner changed the title ~~refactor!: replace DataAccessor with Table in ProofExpr && remove table_length from ProofPlan::result_evaluate~~ refactor!: replace DataAccessor with Table in ProofExpr && remove input_length from ProofPlan::result_evaluate Nov 12, 2024

iajoiner force-pushed the refactor/rem-acc branch 4 times, most recently from 695d4d1 to 4b8f3e0 Compare November 12, 2024 21:55

iajoiner enabled auto-merge November 13, 2024 01:10

iajoiner force-pushed the refactor/rem-acc branch from 4b8f3e0 to e380b59 Compare November 13, 2024 04:55

JayWhite2357 requested changes Nov 13, 2024

View reviewed changes

refactor!: replace DataAccessor with Table in ProofExpr && remo…

56a1885

…ve `table_length` from `ProofPlan::result_evaluate`

iajoiner force-pushed the refactor/rem-acc branch from e380b59 to 56a1885 Compare November 13, 2024 09:40

fix: add num_rows to Table

27abe76

iajoiner requested a review from JayWhite2357 November 13, 2024 15:41

iajoiner force-pushed the refactor/rem-acc branch 3 times, most recently from 61da2f0 to 118f432 Compare November 13, 2024 20:11

fix: address review comments

36a8eba

iajoiner force-pushed the refactor/rem-acc branch from 118f432 to 36a8eba Compare November 13, 2024 20:24

JayWhite2357 requested changes Nov 13, 2024

View reviewed changes

iajoiner force-pushed the refactor/rem-acc branch from caf7502 to e06f251 Compare November 13, 2024 23:12

iajoiner requested a review from JayWhite2357 November 13, 2024 23:12

iajoiner force-pushed the refactor/rem-acc branch from e06f251 to 3525484 Compare November 13, 2024 23:14

fix: address review comments

e1ac21d

iajoiner force-pushed the refactor/rem-acc branch from 3525484 to e1ac21d Compare November 13, 2024 23:15

JayWhite2357 approved these changes Nov 14, 2024

View reviewed changes

iajoiner merged commit cecf821 into main Nov 14, 2024
11 checks passed

github-actions bot added the released label Nov 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

iajoiner commented Nov 11, 2024 •

edited

Loading

JayWhite2357 Nov 13, 2024

JayWhite2357 Nov 13, 2024

JayWhite2357 Nov 13, 2024

JayWhite2357 Nov 13, 2024

JayWhite2357 Nov 13, 2024

JayWhite2357 Nov 13, 2024

JayWhite2357 Nov 13, 2024

JayWhite2357 Nov 13, 2024

JayWhite2357 Nov 13, 2024

github-actions bot commented Nov 14, 2024

	let used_table = Table::<'a, S>::try_from_iter(column_refs.iter().map(\|column_ref\| {
	let used_table = Table::<'a, S>::try_from_iter(column_refs.into_iter().map(\|column_ref\| {

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

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

Conversation

iajoiner commented Nov 11, 2024 • edited Loading

Please go through the following checklist

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 14, 2024

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

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

iajoiner commented Nov 11, 2024 •

edited

Loading