Skip to content

Commit

Permalink
fix: correct scale bug in inequality expr (#110)
Browse files Browse the repository at this point in the history
# Rationale for this change

Suppose I have a table of times denoted in seconds:

```rust
accessor.add_table(
    "sxt.table".parse().unwrap(),
    owned_table([timestamptz(
        "times",
        PoSQLTimeUnit::Second,
        PoSQLTimeZone::Utc,
        test_timestamps,
    )]),
    0,
);
```

Assume that the table contains the following:
```rust
vec![-1, 0, 1];
```

And I wish to query the following against this table:

```sql
"SELECT * FROM table WHERE times < timestamp '1970-01-01T00:00:00.000000001Z'"
```

One would expect that the query should give back [-1, 0], but instead,
after parsing the query and generating a proof:

```rust
// Parse and execute the query
let query = QueryExpr::try_new(
    query_str.parse().unwrap(),
    "sxt".parse().unwrap(),
    &accessor,
)
.unwrap();

let proof = VerifiableQueryResult::<InnerProductProof>::new(query.proof_expr(), &accessor, &());

// Verify the results
let owned_table_result = proof
    .verify(query.proof_expr(), &accessor, &())
    .unwrap()
    .table;
```

we receive the following:

```bash
ProofError(VerificationError("sumcheck evaluation check failed"))
```

from the ```unwrap()``` on the ```verify()``` call:

```rust
    // Verify the results
    let owned_table_result = proof
        .verify(query.proof_expr(), &accessor, &())
        .unwrap()
```

The bug is
[here](https://github.com/spaceandtimelabs/sxt-proof-of-sql/blob/644b0a784abb633f1824ddb73dd3d46c565ee19b/crates/proof-of-sql/src/sql/ast/inequality_expr.rs#L68):

```rust
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")
} else {
    scale_and_subtract(alloc, rhs_column, lhs_column, lhs_scale, rhs_scale, false)
        .expect("Failed to scale and subtract")
};
 ```
 
 rhs and lhs should be reversed in the gte branch. Additionally, there is a testing gap for >= / gte inequalities. This issue should be resolved in #110 

# What changes are included in this PR?

Corrects the branch for lte and gte and adds tests to cover the gap for gte comparisons.

# Are these changes tested?

yes
  • Loading branch information
Dustin-Ray authored Aug 13, 2024
1 parent 0fdd1f9 commit 1967d3b
Show file tree
Hide file tree
Showing 2 changed files with 103 additions and 3 deletions.
4 changes: 2 additions & 2 deletions crates/proof-of-sql/src/sql/ast/inequality_expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ impl<C: Commitment> ProvableExpr<C> for InequalityExpr<C> {
scale_and_subtract(alloc, lhs_column, rhs_column, lhs_scale, rhs_scale, false)
.expect("Failed to scale and subtract")
} else {
scale_and_subtract(alloc, rhs_column, lhs_column, lhs_scale, rhs_scale, false)
scale_and_subtract(alloc, rhs_column, lhs_column, rhs_scale, lhs_scale, false)
.expect("Failed to scale and subtract")
};

Expand Down Expand Up @@ -97,7 +97,7 @@ impl<C: Commitment> ProvableExpr<C> for InequalityExpr<C> {
scale_and_subtract(alloc, lhs_column, rhs_column, lhs_scale, rhs_scale, false)
.expect("Failed to scale and subtract")
} else {
scale_and_subtract(alloc, rhs_column, lhs_column, lhs_scale, rhs_scale, false)
scale_and_subtract(alloc, rhs_column, lhs_column, rhs_scale, lhs_scale, false)
.expect("Failed to scale and subtract")
};

Expand Down
102 changes: 101 additions & 1 deletion crates/proof-of-sql/src/sql/ast/inequality_expr_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@ use crate::{
base::{
commitment::InnerProductProof,
database::{
owned_table_utility::*, Column, OwnedTable, OwnedTableTestAccessor, TestAccessor,
owned_table_utility::*, Column, LiteralValue, OwnedTable, OwnedTableTestAccessor,
TestAccessor,
},
math::decimal::scale_scalar,
scalar::{Curve25519Scalar, Scalar},
Expand All @@ -16,12 +17,81 @@ use crate::{
use bumpalo::Bump;
use curve25519_dalek::RistrettoPoint;
use itertools::{multizip, MultiUnzip};
use proof_of_sql_parser::posql_time::{PoSQLTimeUnit, PoSQLTimeZone};
use rand::{
distributions::{Distribution, Uniform},
rngs::StdRng,
};
use rand_core::SeedableRng;

#[test]
fn we_can_compare_columns_with_small_timestamp_values_gte() {
let data: OwnedTable<Curve25519Scalar> = owned_table([timestamptz(
"a",
PoSQLTimeUnit::Second,
PoSQLTimeZone::Utc,
vec![-1, 0, 1],
)]);
let t = "sxt.t".parse().unwrap();
let accessor = OwnedTableTestAccessor::<InnerProductProof>::new_from_table(t, data, 0, ());
let ast = dense_filter(
cols_expr_plan(t, &["a"], &accessor),
tab(t),
gte(
column(t, "a", &accessor),
ProvableExprPlan::new_literal(LiteralValue::TimeStampTZ(
PoSQLTimeUnit::Nanosecond,
PoSQLTimeZone::Utc,
1,
)),
),
);

let verifiable_res = VerifiableQueryResult::<InnerProductProof>::new(&ast, &accessor, &());
let res = verifiable_res.verify(&ast, &accessor, &()).unwrap().table;
let expected_res = owned_table([timestamptz(
"a",
PoSQLTimeUnit::Second,
PoSQLTimeZone::Utc,
vec![1],
)]);
assert_eq!(res, expected_res);
}

#[test]
fn we_can_compare_columns_with_small_timestamp_values_lte() {
let data: OwnedTable<Curve25519Scalar> = owned_table([timestamptz(
"a",
PoSQLTimeUnit::Second,
PoSQLTimeZone::Utc,
vec![-1, 0, 1],
)]);
let t = "sxt.t".parse().unwrap();
let accessor = OwnedTableTestAccessor::<InnerProductProof>::new_from_table(t, data, 0, ());
let ast = dense_filter(
cols_expr_plan(t, &["a"], &accessor),
tab(t),
lte(
column(t, "a", &accessor),
ProvableExprPlan::new_literal(LiteralValue::TimeStampTZ(
PoSQLTimeUnit::Nanosecond,
PoSQLTimeZone::Utc,
1,
)),
),
);

let verifiable_res = VerifiableQueryResult::<InnerProductProof>::new(&ast, &accessor, &());
let res = verifiable_res.verify(&ast, &accessor, &()).unwrap().table;
let expected_res = owned_table([timestamptz(
"a",
PoSQLTimeUnit::Second,
PoSQLTimeZone::Utc,
vec![-1, 0],
)]);
assert_eq!(res, expected_res);
}

#[test]
fn we_can_compare_a_constant_column() {
let data = owned_table([bigint("a", [123_i64, 123, 123]), bigint("b", [1_i64, 2, 3])]);
Expand Down Expand Up @@ -149,6 +219,36 @@ fn we_can_compare_columns_with_small_decimal_values_with_scale() {
assert_eq!(res, expected_res);
}

#[test]
fn we_can_compare_columns_with_small_decimal_values_with_differing_scale_gte() {
let scalar_pos = scale_scalar(Curve25519Scalar::ONE, 38).unwrap() - Curve25519Scalar::ONE;
let scalar_neg = -scalar_pos;
let data: OwnedTable<Curve25519Scalar> = owned_table([
bigint("a", [123, 25]),
bigint("b", [55, -53]),
varchar("d", ["abc", "de"]),
decimal75("e", 38, 0, [scalar_pos, scalar_neg]),
decimal75("f", 38, 38, [scalar_neg, scalar_pos]),
]);
let t = "sxt.t".parse().unwrap();
let accessor = OwnedTableTestAccessor::<InnerProductProof>::new_from_table(t, data, 0, ());
let ast = dense_filter(
cols_expr_plan(t, &["a", "d", "e", "f"], &accessor),
tab(t),
gte(column(t, "f", &accessor), const_bigint(0_i64)),
);
let verifiable_res = VerifiableQueryResult::new(&ast, &accessor, &());
exercise_verification(&verifiable_res, &ast, &accessor, t);
let res = verifiable_res.verify(&ast, &accessor, &()).unwrap().table;
let expected_res = owned_table([
bigint("a", [25]),
varchar("d", ["de"]),
decimal75("e", 38, 0, [scalar_neg]),
decimal75("f", 38, 38, [scalar_pos]),
]);
assert_eq!(res, expected_res);
}

#[test]
fn we_can_compare_columns_returning_extreme_decimal_values() {
let scalar_min_signed = -Curve25519Scalar::MAX_SIGNED - Curve25519Scalar::ONE;
Expand Down

0 comments on commit 1967d3b

Please sign in to comment.