Skip to content

Commit

Permalink
fix: derive copy for Column to prevent unnecessary cloning (#139)
Browse files Browse the repository at this point in the history 
# Rationale for this change

Derives Copy for Column, reducing the need for cloning in production and
test code. Note the diff includes changes to test code where this change
isnt really necessary, but these changes satisfy clippy.

# What changes are included in this PR?

1. Derives Copy on Column
2. Removes unnecessary clones where applicable 

# Are these changes tested?

existing tests pass
  • Loading branch information
@Dustin-Ray
Dustin-Ray authored Sep 9, 2024
1 parent a1cab14 commit 9c25104
Show file tree
Hide file tree
Showing 12 changed files with 124 additions and 124 deletions.
4 changes: 2 additions & 2 deletions crates/proof-of-sql/benches/scaffold/benchmark_accessor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ impl<'a, C: Commitment> BenchmarkAccessor<'a, C> {
for (column, commitment) in columns.iter().zip(commitments) {
self.columns.insert(
ColumnRef::new(table_ref, column.0, column.1.column_type()),
column.1.clone(),
column.1,
);
self.commitments.insert(
ColumnRef::new(table_ref, column.0, column.1.column_type()),
Expand All @@ -64,7 +64,7 @@ impl<'a, C: Commitment> BenchmarkAccessor<'a, C> {

impl<C: Commitment> DataAccessor<C::Scalar> for BenchmarkAccessor<'_, C> {
fn get_column(&self, column: ColumnRef) -> Column<C::Scalar> {
self.columns.get(&column).unwrap().clone()
*self.columns.get(&column).unwrap()
}
}
impl<C: Commitment> MetadataAccessor for BenchmarkAccessor<'_, C> {
Expand Down
4 changes: 2 additions & 2 deletions crates/proof-of-sql/src/base/database/column.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use std::sync::Arc;
/// Note: The types here should correspond to native SQL database types.
/// See `<https://ignite.apache.org/docs/latest/sql-reference/data-types>` for
/// a description of the native types used by Apache Ignite.
#[derive(Debug, Eq, PartialEq, Clone)]
#[derive(Debug, Eq, PartialEq, Clone, Copy)]
#[non_exhaustive]
pub enum Column<'a, S: Scalar> {
/// Boolean columns
Expand Down Expand Up @@ -179,7 +179,7 @@ impl<'a, S: Scalar> Column<'a, S> {
}

/// Convert a column to a vector of Scalar values with scaling
pub(crate) fn to_scalar_with_scaling(&self, scale: i8) -> Vec<S> {
pub(crate) fn to_scalar_with_scaling(self, scale: i8) -> Vec<S> {
let scale_factor = scale_scalar(S::ONE, scale).expect("Invalid scale factor");
match self {
Self::Boolean(col) => col
Expand Down
42 changes: 21 additions & 21 deletions crates/proof-of-sql/src/base/database/group_by_util_test.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ fn we_can_aggregate_empty_columns() {
let column_b = Column::VarChar((&[], &[]));
let column_c = Column::Int128(&[]);
let column_d = Column::Scalar(&[]);
let group_by = &[column_a.clone(), column_b.clone()];
let sum_columns = &[column_c.clone(), column_d.clone()];
let group_by = &[column_a, column_b];
let sum_columns = &[column_c, column_d];
let selection = &[];
let alloc = Bump::new();
let aggregate_result = aggregate_columns(&alloc, group_by, sum_columns, &[], &[], selection)
Expand All @@ -37,9 +37,9 @@ fn we_can_aggregate_columns_with_empty_group_by_and_no_rows_selected() {
let column_c = Column::Int128(slice_c);
let column_d = Column::Scalar(&scals_d);
let group_by = &[];
let sum_columns = &[column_c.clone(), column_d.clone()];
let max_columns = &[column_c.clone(), column_d.clone()];
let min_columns = &[column_c.clone(), column_d.clone()];
let sum_columns = &[column_c, column_d];
let max_columns = &[column_c, column_d];
let min_columns = &[column_c, column_d];
let alloc = Bump::new();
let aggregate_result = aggregate_columns(
&alloc,
Expand Down Expand Up @@ -73,9 +73,9 @@ fn we_can_aggregate_columns_with_empty_group_by() {
let column_c = Column::Int128(slice_c);
let column_d = Column::Scalar(&scals_d);
let group_by = &[];
let sum_columns = &[column_c.clone(), column_d.clone()];
let max_columns = &[column_c.clone(), column_d.clone()];
let min_columns = &[column_c.clone(), column_d.clone()];
let sum_columns = &[column_c, column_d];
let max_columns = &[column_c, column_d];
let min_columns = &[column_c, column_d];
let alloc = Bump::new();
let aggregate_result = aggregate_columns(
&alloc,
Expand Down Expand Up @@ -128,10 +128,10 @@ fn we_can_aggregate_columns() {
let column_b = Column::VarChar((slice_b, &scals_b));
let column_c = Column::Int128(slice_c);
let column_d = Column::Scalar(&scals_d);
let group_by = &[column_a.clone(), column_b.clone()];
let sum_columns = &[column_c.clone(), column_d.clone()];
let max_columns = &[column_c.clone(), column_d.clone()];
let min_columns = &[column_c.clone(), column_d.clone()];
let group_by = &[column_a, column_b];
let sum_columns = &[column_c, column_d];
let max_columns = &[column_c, column_d];
let min_columns = &[column_c, column_d];
let alloc = Bump::new();
let aggregate_result = aggregate_columns(
&alloc,
Expand Down Expand Up @@ -233,19 +233,19 @@ fn we_can_compare_indexes_by_columns_for_bigint_columns() {
let column_b = Column::BigInt::<DoryScalar>(slice_b);
let column_c = Column::BigInt::<DoryScalar>(slice_c);

let columns = &[column_a.clone()];
let columns = &[column_a];
assert_eq!(compare_indexes_by_columns(columns, 0, 1), Ordering::Greater);
assert_eq!(compare_indexes_by_columns(columns, 1, 2), Ordering::Less);
assert_eq!(compare_indexes_by_columns(columns, 2, 3), Ordering::Equal);
assert_eq!(compare_indexes_by_columns(columns, 2, 1), Ordering::Greater);
assert_eq!(compare_indexes_by_columns(columns, 1, 0), Ordering::Less);
let columns = &[column_a.clone(), column_b.clone()];
let columns = &[column_a, column_b];
assert_eq!(compare_indexes_by_columns(columns, 0, 1), Ordering::Greater);
assert_eq!(compare_indexes_by_columns(columns, 1, 2), Ordering::Less);
assert_eq!(compare_indexes_by_columns(columns, 2, 3), Ordering::Less);
assert_eq!(compare_indexes_by_columns(columns, 3, 4), Ordering::Greater);
assert_eq!(compare_indexes_by_columns(columns, 2, 7), Ordering::Equal);
let columns = &[column_a.clone(), column_b.clone(), column_c.clone()];
let columns = &[column_a, column_b, column_c];
assert_eq!(compare_indexes_by_columns(columns, 0, 1), Ordering::Greater);
assert_eq!(compare_indexes_by_columns(columns, 1, 2), Ordering::Less);
assert_eq!(compare_indexes_by_columns(columns, 2, 3), Ordering::Less);
Expand All @@ -263,19 +263,19 @@ fn we_can_compare_indexes_by_columns_for_mixed_columns() {
let column_b = Column::Int128(slice_b);
let column_c = Column::BigInt(slice_c);

let columns = &[column_a.clone()];
let columns = &[column_a];
assert_eq!(compare_indexes_by_columns(columns, 0, 1), Ordering::Greater);
assert_eq!(compare_indexes_by_columns(columns, 1, 2), Ordering::Less);
assert_eq!(compare_indexes_by_columns(columns, 2, 3), Ordering::Equal);
assert_eq!(compare_indexes_by_columns(columns, 2, 1), Ordering::Greater);
assert_eq!(compare_indexes_by_columns(columns, 1, 0), Ordering::Less);
let columns = &[column_a.clone(), column_b.clone()];
let columns = &[column_a, column_b];
assert_eq!(compare_indexes_by_columns(columns, 0, 1), Ordering::Greater);
assert_eq!(compare_indexes_by_columns(columns, 1, 2), Ordering::Less);
assert_eq!(compare_indexes_by_columns(columns, 2, 3), Ordering::Less);
assert_eq!(compare_indexes_by_columns(columns, 3, 4), Ordering::Greater);
assert_eq!(compare_indexes_by_columns(columns, 2, 7), Ordering::Equal);
let columns = &[column_a.clone(), column_b.clone(), column_c.clone()];
let columns = &[column_a, column_b, column_c];
assert_eq!(compare_indexes_by_columns(columns, 0, 1), Ordering::Greater);
assert_eq!(compare_indexes_by_columns(columns, 1, 2), Ordering::Less);
assert_eq!(compare_indexes_by_columns(columns, 2, 3), Ordering::Less);
Expand Down Expand Up @@ -373,19 +373,19 @@ fn we_can_compare_indexes_by_columns_for_scalar_columns() {
let column_b = Column::Int128(slice_b);
let column_c = Column::BigInt(slice_c);

let columns = &[column_a.clone()];
let columns = &[column_a];
assert_eq!(compare_indexes_by_columns(columns, 0, 1), Ordering::Greater);
assert_eq!(compare_indexes_by_columns(columns, 1, 2), Ordering::Less);
assert_eq!(compare_indexes_by_columns(columns, 2, 3), Ordering::Equal);
assert_eq!(compare_indexes_by_columns(columns, 2, 1), Ordering::Greater);
assert_eq!(compare_indexes_by_columns(columns, 1, 0), Ordering::Less);
let columns = &[column_a.clone(), column_b.clone()];
let columns = &[column_a, column_b];
assert_eq!(compare_indexes_by_columns(columns, 0, 1), Ordering::Greater);
assert_eq!(compare_indexes_by_columns(columns, 1, 2), Ordering::Less);
assert_eq!(compare_indexes_by_columns(columns, 2, 3), Ordering::Less);
assert_eq!(compare_indexes_by_columns(columns, 3, 4), Ordering::Greater);
assert_eq!(compare_indexes_by_columns(columns, 2, 7), Ordering::Equal);
let columns = &[column_a.clone(), column_b.clone(), column_c.clone()];
let columns = &[column_a, column_b, column_c];
assert_eq!(compare_indexes_by_columns(columns, 0, 1), Ordering::Greater);
assert_eq!(compare_indexes_by_columns(columns, 1, 2), Ordering::Less);
assert_eq!(compare_indexes_by_columns(columns, 2, 3), Ordering::Less);
Expand Down
Loading

0 comments on commit 9c25104

Please sign in to comment.