fix!: address NITs in PR #361 (#367)

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
- [x] 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.
- [x] I have run the ci check script with `source
scripts/run_ci_checks.sh`.

# Rationale for this change
Since I had auto-merge on NITs didn't get addressed. I promised to file
a PR to resolve them today and hence I do now.
<!--
Why are you proposing this change? If this is already explained clearly
in the linked issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand
your changes and offer better suggestions for fixes.

 Example:
 Add `NestedLoopJoinExec`.
 Closes #345.

Since we added `HashJoinExec` in #323 it has been possible to do
provable inner joins. However performance is not satisfactory in some
cases. Hence we need to fix the problem by implement
`NestedLoopJoinExec` and speed up the code
 for `HashJoinExec`.
-->

# What changes are included in this PR?
- remove `UnimplementedTestAccessor` which is no longer used
- remove `ProofPlan::is_empty` which is only used in
`VerifiableQueryResult` by inlining
- make code more compact in `query_proof_test.rs` and
`verifiable_query_result_test.rs`
<!--
There is no need to duplicate the description in the ticket here but it
is sometimes worth providing a summary of the individual changes in this
PR.

Example:
- Add `NestedLoopJoinExec`.
- Speed up `HashJoinExec`.
- Route joins to `NestedLoopJoinExec` if the outer input is sufficiently
small.
-->

# Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code

If tests are not included in your PR, please explain why (for example,
are they covered by existing tests)?

Example:
Yes.
-->
Existing tests should pass.

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

@@ -81,9 +81,6 @@ pub use expression_evaluation_error::{ExpressionEvaluationError, ExpressionEvalu
 
 mod test_accessor;
 pub use test_accessor::TestAccessor;
-#[cfg(test)]
-#[allow(unused_imports)]
-pub(crate) use test_accessor::UnimplementedTestAccessor;
 
 #[cfg(test)]
 mod test_schema_accessor;

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

@@ -1,11 +1,6 @@
-use super::{
-    Column, ColumnRef, ColumnType, CommitmentAccessor, DataAccessor, MetadataAccessor,
-    SchemaAccessor, TableRef,
-};
-use crate::base::{commitment::Commitment, scalar::Curve25519Scalar};
+use super::{CommitmentAccessor, DataAccessor, MetadataAccessor, SchemaAccessor, TableRef};
+use crate::base::commitment::Commitment;
 use alloc::vec::Vec;
-use curve25519_dalek::ristretto::RistrettoPoint;
-use proof_of_sql_parser::Identifier;
 
 /// A trait that defines the interface for a combined metadata, schema, commitment, and data accessor for unit testing or example purposes.
 pub trait TestAccessor<C: Commitment>:
@@ -31,54 +26,3 @@ pub trait TestAccessor<C: Commitment>:
     /// Update the table offset alongside its column commitments
     fn update_offset(&mut self, table_ref: TableRef, new_offset: usize);
 }
-
-#[derive(Clone, Default)]
-/// A test accessor that leaves all of the required methods except `new` `unimplemented!()`.
-pub struct UnimplementedTestAccessor;
-impl TestAccessor<RistrettoPoint> for UnimplementedTestAccessor {
-    type Table = ();
-
-    fn new_empty() -> Self {
-        UnimplementedTestAccessor
-    }
-
-    fn add_table(&mut self, _table_ref: TableRef, _data: (), _table_offset: usize) {
-        unimplemented!()
-    }
-
-    fn get_column_names(&self, _table_ref: TableRef) -> Vec<&str> {
-        unimplemented!()
-    }
-
-    fn update_offset(&mut self, _table_ref: TableRef, _new_offset: usize) {
-        unimplemented!()
-    }
-}
-impl DataAccessor<Curve25519Scalar> for UnimplementedTestAccessor {
-    fn get_column(&self, _column: ColumnRef) -> Column<Curve25519Scalar> {
-        unimplemented!()
-    }
-}
-impl CommitmentAccessor<RistrettoPoint> for UnimplementedTestAccessor {
-    fn get_commitment(&self, _column: ColumnRef) -> RistrettoPoint {
-        unimplemented!()
-    }
-}
-impl MetadataAccessor for UnimplementedTestAccessor {
-    fn get_length(&self, _table_ref: TableRef) -> usize {
-        unimplemented!()
-    }
-
-    fn get_offset(&self, _table_ref: TableRef) -> usize {
-        unimplemented!()
-    }
-}
-impl SchemaAccessor for UnimplementedTestAccessor {
-    fn lookup_column(&self, _table_ref: TableRef, _column_id: Identifier) -> Option<ColumnType> {
-        unimplemented!()
-    }
-
-    fn lookup_schema(&self, _table_ref: TableRef) -> Vec<(Identifier, ColumnType)> {
-        unimplemented!()
-    }
-}

crates/proof-of-sql/src/sql/proof/proof_plan.rs

@@ -2,8 +2,7 @@ use super::{CountBuilder, FinalRoundBuilder, FirstRoundBuilder, VerificationBuil
 use crate::base::{
     commitment::Commitment,
     database::{
-        Column, ColumnField, ColumnRef, CommitmentAccessor, DataAccessor, MetadataAccessor,
-        OwnedTable, TableRef,
+        Column, ColumnField, ColumnRef, CommitmentAccessor, DataAccessor, OwnedTable, TableRef,
     },
     map::IndexSet,
     proof::ProofError,
@@ -19,14 +18,6 @@ pub trait ProofPlan: Debug + Send + Sync + ProverEvaluate {
     /// Count terms used within the Query's proof
     fn count(&self, builder: &mut CountBuilder) -> Result<(), ProofError>;
 
-    /// Check if the input table is empty
-    fn is_empty(&self, accessor: &dyn MetadataAccessor) -> bool {
-        let table_refs = self.get_table_references();
-        table_refs
-            .iter()
-            .all(|table_ref| accessor.get_length(*table_ref) == 0)
-    }
-
     /// Form components needed to verify and proof store into `VerificationBuilder`
     fn verifier_evaluate<C: Commitment>(
         &self,

crates/proof-of-sql/src/sql/proof/query_proof_test.rs

@@ -112,7 +112,7 @@ fn verify_a_trivial_query_proof_with_given_offset(n: usize, offset_generators: u
     let column: Vec<i64> = vec![0_i64; n];
     let accessor = OwnedTableTestAccessor::<InnerProductProof>::new_from_table(
         "sxt.test".parse().unwrap(),
-        owned_table([bigint("a1", column)]),
+        owned_table([bigint("a1", column.clone())]),
         offset_generators,
         (),
     );
@@ -122,8 +122,7 @@ fn verify_a_trivial_query_proof_with_given_offset(n: usize, offset_generators: u
         table,
     } = proof.verify(&expr, &accessor, &result, &()).unwrap();
     assert_ne!(verification_hash, [0; 32]);
-    let expected_col = vec![0_i64; n];
-    let expected_result = owned_table([bigint("a1", expected_col)]);
+    let expected_result = owned_table([bigint("a1", column)]);
     assert_eq!(table, expected_result);
 }
 

crates/proof-of-sql/src/sql/proof/verifiable_query_result.rs

@@ -88,7 +88,11 @@ impl<CP: CommitmentEvaluationProof> VerifiableQueryResult<CP> {
         // have been rejected at the parsing stage.
 
         // handle the empty case
-        if expr.is_empty(accessor) {
+        let table_refs = expr.get_table_references();
+        if table_refs
+            .into_iter()
+            .all(|table_ref| accessor.get_length(table_ref) == 0)
+        {
             return VerifiableQueryResult {
                 provable_result: None,
                 proof: None,
@@ -124,7 +128,11 @@ impl<CP: CommitmentEvaluationProof> VerifiableQueryResult<CP> {
         // have been rejected at the parsing stage.
 
         // handle the empty case
-        if expr.is_empty(accessor) {
+        let table_refs = expr.get_table_references();
+        if table_refs
+            .into_iter()
+            .all(|table_ref| accessor.get_length(table_ref) == 0)
+        {
             if self.provable_result.is_some() || self.proof.is_some() {
                 return Err(ProofError::VerificationError {
                     error: "zero sumcheck variables but non-empty result",

crates/proof-of-sql/src/sql/proof/verifiable_query_result_test.rs

@@ -91,10 +91,9 @@ fn we_can_verify_queries_on_an_empty_table() {
         columns: 1,
         ..Default::default()
     };
-    let column: Vec<i64> = vec![0_i64; 0];
     let accessor = OwnedTableTestAccessor::<InnerProductProof>::new_from_table(
         "sxt.test".parse().unwrap(),
-        owned_table([bigint("a1", column)]),
+        owned_table([bigint("a1", [0_i64; 0])]),
         0,
         (),
     );
@@ -113,10 +112,9 @@ fn empty_verification_fails_if_the_result_contains_non_null_members() {
         columns: 1,
         ..Default::default()
     };
-    let column: Vec<i64> = vec![0_i64; 0];
     let accessor = OwnedTableTestAccessor::<InnerProductProof>::new_from_table(
         "sxt.test".parse().unwrap(),
-        owned_table([bigint("a1", column)]),
+        owned_table([bigint("a1", [0_i64; 0])]),
         0,
         (),
     );

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

@@ -3,8 +3,7 @@ use crate::{
     base::{
         commitment::Commitment,
         database::{
-            Column, ColumnField, ColumnRef, CommitmentAccessor, DataAccessor, MetadataAccessor,
-            OwnedTable, TableRef,
+            Column, ColumnField, ColumnRef, CommitmentAccessor, DataAccessor, OwnedTable, TableRef,
         },
         map::IndexSet,
         proof::ProofError,