Skip to content

Commit

Permalink
refactor!: remove unnecessary generics from postprocessing (#88)
Browse files Browse the repository at this point in the history
# Rationale for this change
Remove unnecessary generics from `postprocessing`
<!--
Why are you proposing this change? If this is already explained clearly
in the linked Jira ticket then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand
your changes and offer better suggestions for fixes.
-->

# What changes are included in this PR?
- remove `S: Scalar` where it is unnecessary
<!--
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.
-->

# Are these changes tested?
Existing tests do pass
<!--
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)?
-->
  • Loading branch information
iajoiner authored Aug 6, 2024
1 parent 824936a commit 618b4bb
Show file tree
Hide file tree
Showing 5 changed files with 46 additions and 58 deletions.
12 changes: 4 additions & 8 deletions crates/proof-of-sql/src/sql/postprocessing/order_by_expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,22 +10,18 @@ use serde::{Deserialize, Serialize};

/// A node representing a list of `OrderBy` expressions.
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
pub struct OrderByExpr<S: Scalar> {
pub struct OrderByExpr {
by_exprs: Vec<OrderBy>,
_phantom: core::marker::PhantomData<S>,
}

impl<S: Scalar> OrderByExpr<S> {
impl OrderByExpr {
/// Create a new `OrderByExpr` node.
pub fn new(by_exprs: Vec<OrderBy>) -> Self {
Self {
by_exprs,
_phantom: core::marker::PhantomData,
}
Self { by_exprs }
}
}

impl<S: Scalar> PostprocessingStep<S> for OrderByExpr<S> {
impl<S: Scalar> PostprocessingStep<S> for OrderByExpr {
/// Apply the slice transformation to the given `OwnedTable`.
fn apply(&self, owned_table: OwnedTable<S>) -> PostprocessingResult<OwnedTable<S>> {
let mut indexes = (0..owned_table.num_rows()).collect::<Vec<_>>();
Expand Down
52 changes: 26 additions & 26 deletions crates/proof-of-sql/src/sql/postprocessing/order_by_expr_test.rs
Original file line number Diff line number Diff line change
@@ -1,18 +1,21 @@
use crate::{
base::{database::owned_table_utility::*, scalar::Curve25519Scalar},
base::{
database::{owned_table_utility::*, OwnedTable},
scalar::Curve25519Scalar,
},
sql::postprocessing::{apply_postprocessing_steps, test_utility::*, OwnedTablePostprocessing},
};
use proof_of_sql_parser::intermediate_ast::OrderByDirection::{Asc, Desc};
use rand::{seq::SliceRandom, Rng};

#[test]
fn we_can_transform_a_result_using_a_single_order_by_in_ascending_direction() {
let table = owned_table([
let table: OwnedTable<Curve25519Scalar> = owned_table([
bigint("c", [1_i64, -5, i64::MAX]),
varchar("a", ["a", "d", "b"]),
]);
let postprocessing: [OwnedTablePostprocessing<Curve25519Scalar>; 1] = [orders(&["a"], &[Asc])];
let expected_table = owned_table([
let postprocessing: [OwnedTablePostprocessing; 1] = [orders(&["a"], &[Asc])];
let expected_table: OwnedTable<Curve25519Scalar> = owned_table([
bigint("c", [1_i64, i64::MAX, -5]),
varchar("a", ["a", "b", "d"]),
]);
Expand All @@ -22,12 +25,12 @@ fn we_can_transform_a_result_using_a_single_order_by_in_ascending_direction() {

#[test]
fn we_can_transform_a_result_using_a_single_order_by_in_descending_direction() {
let table = owned_table([
let table: OwnedTable<Curve25519Scalar> = owned_table([
int128("c", [1_i128, i128::MIN, i128::MAX]),
varchar("a", ["a", "d", "b"]),
]);
let postprocessing: [OwnedTablePostprocessing<Curve25519Scalar>; 1] = [orders(&["c"], &[Desc])];
let expected_table = owned_table([
let postprocessing: [OwnedTablePostprocessing; 1] = [orders(&["c"], &[Desc])];
let expected_table: OwnedTable<Curve25519Scalar> = owned_table([
int128("c", [i128::MAX, 1, i128::MIN]),
varchar("a", ["b", "a", "d"]),
]);
Expand All @@ -37,13 +40,12 @@ fn we_can_transform_a_result_using_a_single_order_by_in_descending_direction() {

#[test]
fn we_can_transform_a_result_ordering_by_the_first_column_then_the_second_column() {
let table = owned_table([
let table: OwnedTable<Curve25519Scalar> = owned_table([
int("a", [123_i32, 342, i32::MIN, i32::MAX, 123, 34]),
varchar("d", ["alfa", "beta", "abc", "f", "kl", "f"]),
]);
let postprocessing: [OwnedTablePostprocessing<Curve25519Scalar>; 1] =
[orders(&["a", "d"], &[Desc, Desc])];
let expected_table = owned_table([
let postprocessing: [OwnedTablePostprocessing; 1] = [orders(&["a", "d"], &[Desc, Desc])];
let expected_table: OwnedTable<Curve25519Scalar> = owned_table([
int("a", [i32::MAX, 342, 123, 123, 34, i32::MIN]),
varchar("d", ["f", "beta", "kl", "alfa", "f", "abc"]),
]);
Expand All @@ -53,13 +55,12 @@ fn we_can_transform_a_result_ordering_by_the_first_column_then_the_second_column

#[test]
fn we_can_transform_a_result_ordering_by_the_second_column_then_the_first_column() {
let table = owned_table([
let table: OwnedTable<Curve25519Scalar> = owned_table([
smallint("a", [123_i16, 342, -234, i16::MAX, 123, i16::MIN]),
varchar("d", ["alfa", "beta", "abc", "f", "kl", "f"]),
]);
let postprocessing: [OwnedTablePostprocessing<Curve25519Scalar>; 1] =
[orders(&["d", "a"], &[Desc, Asc])];
let expected_table = owned_table([
let postprocessing: [OwnedTablePostprocessing; 1] = [orders(&["d", "a"], &[Desc, Asc])];
let expected_table: OwnedTable<Curve25519Scalar> = owned_table([
smallint("a", [123_i16, i16::MIN, i16::MAX, 342, 123, -234]),
varchar("d", ["kl", "f", "f", "beta", "alfa", "abc"]),
]);
Expand Down Expand Up @@ -87,16 +88,15 @@ fn we_can_use_int128_columns_inside_order_by_in_desc_order() {
i128::MAX,
];

let table = owned_table([int128("h", s), int128("j", s)]);
let postprocessing: [OwnedTablePostprocessing<Curve25519Scalar>; 1] =
[orders(&["j", "h"], &[Desc, Asc])];
let table: OwnedTable<Curve25519Scalar> = owned_table([int128("h", s), int128("j", s)]);
let postprocessing: [OwnedTablePostprocessing; 1] = [orders(&["j", "h"], &[Desc, Asc])];
let actual_table = apply_postprocessing_steps(table, &postprocessing).unwrap();

let mut sorted_s = s;
sorted_s.sort_unstable();
let reverse_sorted_s = sorted_s.into_iter().rev().collect::<Vec<_>>();

let expected_table = owned_table([
let expected_table: OwnedTable<Curve25519Scalar> = owned_table([
int128("h", reverse_sorted_s.clone()),
int128("j", reverse_sorted_s),
]);
Expand All @@ -123,15 +123,15 @@ fn we_can_use_int128_columns_inside_order_by_in_asc_order() {
i128::MAX,
];

let table = owned_table([int128("h", s), int128("j", s)]);
let postprocessing: [OwnedTablePostprocessing<Curve25519Scalar>; 1] =
[orders(&["j", "h"], &[Asc, Desc])];
let table: OwnedTable<Curve25519Scalar> = owned_table([int128("h", s), int128("j", s)]);
let postprocessing: [OwnedTablePostprocessing; 1] = [orders(&["j", "h"], &[Asc, Desc])];
let actual_table = apply_postprocessing_steps(table, &postprocessing).unwrap();

let mut sorted_s = s;
sorted_s.sort_unstable();

let expected_table = owned_table([int128("h", sorted_s), int128("j", sorted_s)]);
let expected_table: OwnedTable<Curve25519Scalar> =
owned_table([int128("h", sorted_s), int128("j", sorted_s)]);
assert_eq!(actual_table, expected_table);
}

Expand All @@ -153,9 +153,9 @@ fn we_can_do_order_by_with_random_i128_data() {
(shuffled_s, sorted_s)
};

let table = owned_table([int128("h", shuffled_data)]);
let expected_table = owned_table([int128("h", sorted_data)]);
let postprocessing: [OwnedTablePostprocessing<Curve25519Scalar>; 1] = [orders(&["h"], &[Asc])];
let table: OwnedTable<Curve25519Scalar> = owned_table([int128("h", shuffled_data)]);
let expected_table: OwnedTable<Curve25519Scalar> = owned_table([int128("h", sorted_data)]);
let postprocessing: [OwnedTablePostprocessing; 1] = [orders(&["h"], &[Asc])];
let actual_table = apply_postprocessing_steps(table, &postprocessing).unwrap();
assert_eq!(actual_table, expected_table);
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,14 @@ use crate::base::{database::OwnedTable, scalar::Scalar};

/// An enum for nodes that can apply postprocessing to a `OwnedTable`.
#[derive(Debug, Clone)]
pub enum OwnedTablePostprocessing<S: Scalar> {
pub enum OwnedTablePostprocessing {
/// Slice the `OwnedTable` with the given `SliceExpr`.
Slice(SliceExpr<S>),
Slice(SliceExpr),
/// Order the `OwnedTable` with the given `OrderByExpr`.
OrderBy(OrderByExpr<S>),
OrderBy(OrderByExpr),
}

impl<S: Scalar> PostprocessingStep<S> for OwnedTablePostprocessing<S> {
impl<S: Scalar> PostprocessingStep<S> for OwnedTablePostprocessing {
/// Apply the postprocessing step to the `OwnedTable` and return the result.
fn apply(&self, owned_table: OwnedTable<S>) -> PostprocessingResult<OwnedTable<S>> {
match self {
Expand All @@ -20,21 +20,21 @@ impl<S: Scalar> PostprocessingStep<S> for OwnedTablePostprocessing<S> {
}
}

impl<S: Scalar> OwnedTablePostprocessing<S> {
impl OwnedTablePostprocessing {
/// Create a new `OwnedTablePostprocessing` with the given `SliceExpr`.
pub fn new_slice(slice_expr: SliceExpr<S>) -> Self {
pub fn new_slice(slice_expr: SliceExpr) -> Self {
Self::Slice(slice_expr)
}
/// Create a new `OwnedTablePostprocessing` with the given `OrderByExpr`.
pub fn new_order_by(order_by_expr: OrderByExpr<S>) -> Self {
pub fn new_order_by(order_by_expr: OrderByExpr) -> Self {
Self::OrderBy(order_by_expr)
}
}

/// Apply a list of postprocessing steps to an `OwnedTable`.
pub fn apply_postprocessing_steps<S: Scalar>(
owned_table: OwnedTable<S>,
postprocessing_steps: &[OwnedTablePostprocessing<S>],
postprocessing_steps: &[OwnedTablePostprocessing],
) -> PostprocessingResult<OwnedTable<S>> {
// Sadly try_fold() only works on Options
let mut current_table = owned_table;
Expand Down
12 changes: 4 additions & 8 deletions crates/proof-of-sql/src/sql/postprocessing/slice_expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ use super::{PostprocessingError, PostprocessingResult, PostprocessingStep};
use crate::base::{database::OwnedTable, scalar::Scalar};
use serde::{Deserialize, Serialize};

/// A `SliceExpr` represents a slice of a `LazyFrame`.
/// A `SliceExpr` represents a slice of an `OwnedTable`.
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
pub struct SliceExpr<S: Scalar> {
pub struct SliceExpr {
/// number of rows to return
///
/// - if None, specify all rows
Expand All @@ -17,23 +17,19 @@ pub struct SliceExpr<S: Scalar> {
/// - if Some(negative), specify the offset from the end
/// (e.g. -1 is the last row, -2 is the second to last row, etc.)
offset_value: Option<i64>,

/// Phantom
_phantom: core::marker::PhantomData<S>,
}

impl<S: Scalar> SliceExpr<S> {
impl SliceExpr {
/// Create a new `SliceExpr` with the given `number_rows` and `offset`.
pub fn new(number_rows: Option<u64>, offset_value: Option<i64>) -> Self {
Self {
number_rows,
offset_value,
_phantom: core::marker::PhantomData,
}
}
}

impl<S: Scalar> PostprocessingStep<S> for SliceExpr<S> {
impl<S: Scalar> PostprocessingStep<S> for SliceExpr {
/// Apply the slice transformation to the given `OwnedTable`.
fn apply(&self, owned_table: OwnedTable<S>) -> PostprocessingResult<OwnedTable<S>> {
let num_rows = owned_table.num_rows();
Expand Down
12 changes: 4 additions & 8 deletions crates/proof-of-sql/src/sql/postprocessing/test_utility.rs
Original file line number Diff line number Diff line change
@@ -1,15 +1,11 @@
use super::*;
use crate::base::scalar::Scalar;
use proof_of_sql_parser::intermediate_ast::{OrderBy, OrderByDirection};

pub fn slice<S: Scalar>(limit: Option<u64>, offset: Option<i64>) -> OwnedTablePostprocessing<S> {
OwnedTablePostprocessing::<S>::new_slice(SliceExpr::new(limit, offset))
pub fn slice(limit: Option<u64>, offset: Option<i64>) -> OwnedTablePostprocessing {
OwnedTablePostprocessing::new_slice(SliceExpr::new(limit, offset))
}

pub fn orders<S: Scalar>(
cols: &[&str],
directions: &[OrderByDirection],
) -> OwnedTablePostprocessing<S> {
pub fn orders(cols: &[&str], directions: &[OrderByDirection]) -> OwnedTablePostprocessing {
let by_exprs = cols
.iter()
.zip(directions.iter())
Expand All @@ -18,5 +14,5 @@ pub fn orders<S: Scalar>(
direction: *direction,
})
.collect();
OwnedTablePostprocessing::<S>::new_order_by(OrderByExpr::new(by_exprs))
OwnedTablePostprocessing::new_order_by(OrderByExpr::new(by_exprs))
}

0 comments on commit 618b4bb

Please sign in to comment.