fix!: change the behavior of `compare_indexes_by_owned_columns_with_d…

…irection` to fix it for decimals and refactor the code
spaceandtimelabs · Nov 21, 2024 · 69c17eb · 69c17eb
1 parent 2fb37b4
commit 69c17eb
Show file tree

Hide file tree

Showing 3 changed files with 12 additions and 19 deletions.
diff --git a/crates/proof-of-sql/src/base/database/order_by_util.rs b/crates/proof-of-sql/src/base/database/order_by_util.rs
@@ -3,6 +3,7 @@ use crate::base::{
     database::{Column, OwnedColumn},
     scalar::{Scalar, ScalarExt},
 };
+use alloc::vec::Vec;
 use core::cmp::Ordering;
 use proof_of_sql_parser::intermediate_ast::OrderByDirection;
 
@@ -39,25 +40,15 @@ pub(crate) fn compare_indexes_by_owned_columns<S: Scalar>(
     i: usize,
     j: usize,
 ) -> Ordering {
-    order_by
+    let order_by_pairs = order_by
         .iter()
-        .map(|col| match col {
-            OwnedColumn::Boolean(col) => col[i].cmp(&col[j]),
-            OwnedColumn::TinyInt(col) => col[i].cmp(&col[j]),
-            OwnedColumn::SmallInt(col) => col[i].cmp(&col[j]),
-            OwnedColumn::Int(col) => col[i].cmp(&col[j]),
-            OwnedColumn::BigInt(col) | OwnedColumn::TimestampTZ(_, _, col) => col[i].cmp(&col[j]),
-            OwnedColumn::Int128(col) => col[i].cmp(&col[j]),
-            OwnedColumn::Decimal75(_, _, col) => col[i].signed_cmp(&col[j]),
-            OwnedColumn::Scalar(col) => col[i].cmp(&col[j]),
-            OwnedColumn::VarChar(col) => col[i].cmp(&col[j]),
-        })
-        .find(|&ord| ord != Ordering::Equal)
-        .unwrap_or(Ordering::Equal)
+        .map(|&col| (col.clone(), OrderByDirection::Asc))
+        .collect::<Vec<_>>();
+    compare_indexes_by_owned_columns_with_direction(&order_by_pairs, i, j)
 }
 
-/// Compares the tuples `(order_by_pairs[0][i], order_by_pairs[1][i], ...)` and
-/// `(order_by_pairs[0][j], order_by_pairs[1][j], ...)` in lexicographic order.
+/// Compares the tuples `(left[0][i], left[1][i], ...)` and
+/// `(right[0][j], right[1][j], ...)` in lexicographic order.
 /// Note that direction flips the ordering.
 pub(crate) fn compare_indexes_by_owned_columns_with_direction<S: Scalar>(
     order_by_pairs: &[(OwnedColumn<S>, OrderByDirection)],
@@ -76,7 +67,8 @@ pub(crate) fn compare_indexes_by_owned_columns_with_direction<S: Scalar>(
                     col[i].cmp(&col[j])
                 }
                 OwnedColumn::Int128(col) => col[i].cmp(&col[j]),
-                OwnedColumn::Decimal75(_, _, col) | OwnedColumn::Scalar(col) => col[i].cmp(&col[j]),
+                OwnedColumn::Decimal75(_, _, col) => col[i].signed_cmp(&col[j]),
+                OwnedColumn::Scalar(col) => col[i].cmp(&col[j]),
                 OwnedColumn::VarChar(col) => col[i].cmp(&col[j]),
             };
             match direction {

diff --git a/crates/proof-of-sql/src/base/database/order_by_util_test.rs b/crates/proof-of-sql/src/base/database/order_by_util_test.rs
@@ -1,11 +1,13 @@
 use crate::{
     base::{
         database::{order_by_util::*, Column, OwnedColumn},
+        math::decimal::Precision,
         scalar::test_scalar::TestScalar,
     },
     proof_primitive::dory::DoryScalar,
 };
 use core::cmp::Ordering;
+use proof_of_sql_parser::intermediate_ast::OrderByDirection;
 
 #[test]
 fn we_can_compare_indexes_by_columns_with_no_columns() {
@@ -200,7 +202,7 @@ fn we_can_compare_columns_with_direction() {
     let col3: OwnedColumn<TestScalar> = OwnedColumn::Decimal75(
         Precision::new(70).unwrap(),
         20,
-        [1, 2, 2, 1, 2]
+        [-3, 2, 2, -3, 2]
             .iter()
             .map(|&i| TestScalar::from(i))
             .collect(),

diff --git a/crates/proof-of-sql/src/base/database/owned_column.rs b/crates/proof-of-sql/src/base/database/owned_column.rs
@@ -347,7 +347,6 @@ mod test {
     use crate::base::{math::decimal::Precision, scalar::test_scalar::TestScalar};
     use alloc::vec;
     use bumpalo::Bump;
-    use proof_of_sql_parser::intermediate_ast::OrderByDirection;
 
     #[test]
     fn we_can_slice_a_column() {