feat!: remove `table_length` from `ProofExpr::result_evaluate` and return `ColumnarValue` #334

iajoiner · 2024-11-01T19:30:04Z

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

In order to make ProofPlans composable it is necessary to allow one or more ProofPlans as the input to another. As such it is a great idea to remove table_length as an argument in ProofPlan::result_evaluate which in turn requires removal in ProofExpr.

What changes are included in this PR?

Remove table_length from ProofExpr::result_evaluate and return ColumnarValue.
Remove table_length from ProofPlan::result_evaluate.

Are these changes tested?

Yes

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

JayWhite2357 · 2024-11-07T02:22:47Z

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

+        let rhs_columnar_value: ColumnarValue<'a, S> = self.rhs.result_evaluate(alloc, accessor);
+        lhs_columnar_value
+            .apply_boolean_binary_operator(&rhs_columnar_value, BinaryOperator::And, alloc)
+            .expect("Failed to apply boolean binary operator")


This expect also wouldn't be needed if you do my suggestion above.

JayWhite2357 · 2024-11-07T02:34:32Z

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

+        let is_literal = matches!(
+            (&lhs_columnar_value, &rhs_columnar_value),
+            (&ColumnarValue::Literal(_), &ColumnarValue::Literal(_))
+        );
        let lhs_scale = self.lhs.data_type().scale().unwrap_or(0);
        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_length, alloc, res))
+        let res = scale_and_subtract_columnar_value(
+            alloc,
+            lhs_columnar_value,
+            rhs_columnar_value,
+            lhs_scale,
+            rhs_scale,
+            true,
+        )
+        .expect("Failed to scale and subtract");
+        let raw_result = result_evaluate_equals_zero(res.len(), alloc, res);
+        if is_literal {
+            ColumnarValue::Literal(LiteralValue::Boolean(raw_result[0]))
+        } else {
+            ColumnarValue::Column(Column::Boolean(raw_result))
+        }


Using an is_literal flag and returning a length 1 slice for Literals is very messy.
This code would be extremely difficult to modify in the future.

At this point, I think the result_evaluate_equals_zero is outdated and simply needs to be replaced with a proper solution.

There's no reason this can't look very similar to what the AndExpr::result_evaluate now looks like.

JayWhite2357 · 2024-11-07T02:58:32Z

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

See #342 for a better alternative to the changes in this file.

JayWhite2357 · 2024-11-07T02:59:25Z

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

+        let is_literal = matches!(
+            (&lhs_columnar_value, &rhs_columnar_value),
+            (&ColumnarValue::Literal(_), &ColumnarValue::Literal(_))
+        );
        let lhs_scale = self.lhs.data_type().scale().unwrap_or(0);
        let rhs_scale = self.rhs.data_type().scale().unwrap_or(0);
        let diff = if self.is_lte {
-            scale_and_subtract(alloc, lhs_column, rhs_column, lhs_scale, rhs_scale, false)
-                .expect("Failed to scale and subtract")
+            scale_and_subtract_columnar_value(
+                alloc,
+                lhs_columnar_value,
+                rhs_columnar_value,
+                lhs_scale,
+                rhs_scale,
+                false,
+            )
+            .expect("Failed to scale and subtract")
        } else {
-            scale_and_subtract(alloc, rhs_column, lhs_column, rhs_scale, lhs_scale, false)
-                .expect("Failed to scale and subtract")
+            scale_and_subtract_columnar_value(
+                alloc,
+                rhs_columnar_value,
+                lhs_columnar_value,
+                rhs_scale,
+                lhs_scale,
+                false,
+            )
+            .expect("Failed to scale and subtract")
        };
+        let diff_len = diff.len();

        // diff == 0
-        let equals_zero = result_evaluate_equals_zero(table_length, alloc, diff);
+        let equals_zero = result_evaluate_equals_zero(diff_len, alloc, diff);

        // sign(diff) == -1
-        let sign = result_evaluate_sign(table_length, alloc, diff);
+        let sign = result_evaluate_sign(diff_len, alloc, diff);

        // (diff == 0) || (sign(diff) == -1)
-        Column::Boolean(result_evaluate_or(table_length, alloc, equals_zero, sign))
+        let raw_result = result_evaluate_or(diff_len, alloc, equals_zero, sign);
+        if is_literal {
+            ColumnarValue::Literal(LiteralValue::Boolean(raw_result[0]))
+        } else {
+            ColumnarValue::Column(Column::Boolean(raw_result))
+        }


Same concern here as with equality. This one is even worse.

JayWhite2357 · 2024-11-07T03:03:03Z

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

+            .where_clause
+            .result_evaluate(alloc, accessor)
+            .into_column(input_length, alloc)
+            .expect("Failed to convert columnar value to column");


Can you add a # Panics doc to ProverEvaluate indicating that the underlying columns must all have the same length?

iajoiner changed the title ~~feat: remove table_length from ProofExpr::result_evaluate~~ feat!: remove table_length from ProofExpr::result_evaluate Nov 1, 2024

iajoiner mentioned this pull request Nov 4, 2024

refactor!: add get_table_references, get_output_length and modify get_length #248

Closed

2 tasks

iajoiner force-pushed the feat/remove-len branch 12 times, most recently from b1d74e6 to 327ebb1 Compare November 5, 2024 15:57

iajoiner marked this pull request as ready for review November 5, 2024 17:46

iajoiner force-pushed the feat/remove-len branch from 327ebb1 to ef6b931 Compare November 5, 2024 17:46

iajoiner requested a review from JayWhite2357 November 5, 2024 17:46

iajoiner force-pushed the feat/remove-len branch 2 times, most recently from 2b92209 to bbff8fb Compare November 5, 2024 18:31

iajoiner mentioned this pull request Nov 5, 2024

refactor!: generalize ProverEvaluate::result_evaluate and final_round_evaluate to multiple tables #272

Closed

2 tasks

iajoiner force-pushed the feat/remove-len branch from bbff8fb to 3e7cd99 Compare November 5, 2024 21:42

iajoiner mentioned this pull request Nov 5, 2024

Replace proof-of-sql-parser with sqlparser. #235

Open

4 tasks

iajoiner force-pushed the feat/remove-len branch from 3e7cd99 to 0835eab Compare November 6, 2024 14:56

iajoiner changed the title ~~feat!: remove table_length from ProofExpr::result_evaluate~~ feat!: remove table_length from ProofExpr::result_evaluate and return ColumnarValue Nov 6, 2024

iajoiner force-pushed the feat/remove-len branch 4 times, most recently from d17b716 to e8e94a5 Compare November 6, 2024 22:01

JayWhite2357 requested changes Nov 7, 2024

View reviewed changes

iajoiner enabled auto-merge November 7, 2024 12:03

iajoiner force-pushed the feat/remove-len branch from e8e94a5 to bdc0f9f Compare November 7, 2024 14:07

iajoiner closed this Nov 7, 2024

auto-merge was automatically disabled November 7, 2024 18:19
Pull request was closed

iajoiner force-pushed the feat/remove-len branch from bdc0f9f to bb46fcb Compare November 7, 2024 18:19

iajoiner mentioned this pull request Nov 7, 2024

refactor!: modify ProofExpr::result_evaluate to return ColumnarValue and remove table_length as arg #357

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: remove `table_length` from `ProofExpr::result_evaluate` and return `ColumnarValue` #334

feat!: remove `table_length` from `ProofExpr::result_evaluate` and return `ColumnarValue` #334

iajoiner commented Nov 1, 2024 •

edited

Loading

JayWhite2357 Nov 7, 2024

JayWhite2357 Nov 7, 2024

JayWhite2357 Nov 7, 2024

JayWhite2357 Nov 7, 2024

JayWhite2357 Nov 7, 2024

feat!: remove table_length from ProofExpr::result_evaluate and return ColumnarValue #334

feat!: remove table_length from ProofExpr::result_evaluate and return ColumnarValue #334

Conversation

iajoiner commented Nov 1, 2024 • edited Loading

Please go through the following checklist

Rationale for this change

What changes are included in this PR?

Are these changes tested?

JayWhite2357 Nov 7, 2024

Choose a reason for hiding this comment

JayWhite2357 Nov 7, 2024

Choose a reason for hiding this comment

JayWhite2357 Nov 7, 2024

Choose a reason for hiding this comment

JayWhite2357 Nov 7, 2024

Choose a reason for hiding this comment

JayWhite2357 Nov 7, 2024

Choose a reason for hiding this comment

feat!: remove `table_length` from `ProofExpr::result_evaluate` and return `ColumnarValue` #334

feat!: remove `table_length` from `ProofExpr::result_evaluate` and return `ColumnarValue` #334

iajoiner commented Nov 1, 2024 •

edited

Loading