Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor!: split and simplify owned_column_operation.rs && remove unused functions from slice_operation.rs #359

Merged
merged 4 commits into from
Nov 12, 2024

Conversation

iajoiner
Copy link
Contributor

@iajoiner iajoiner commented Nov 8, 2024

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

Rationale for this change

owned_column_operation.rs is extremely tedious hence we need to simplify it.

What changes are included in this PR?

  • split out arithmetic operations into column_arithmetic_operation.rs and unify them.
  • split out comparison operations into column_comparison_operation.rs and unify them.
  • remove unused functions.

Are these changes tested?

Yes.

@iajoiner iajoiner force-pushed the refactor/owned-arith branch 2 times, most recently from bff8e8a to e22bbd9 Compare November 8, 2024 15:05
@iajoiner iajoiner requested a review from JayWhite2357 November 8, 2024 18:00
Copy link
Contributor

@JayWhite2357 JayWhite2357 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's my suggestion:

pub fn element_wise_add<S: Scalar>(
    lhs: &OwnedColumn<S>,
    rhs: &OwnedColumn<S>,
) -> ColumnOperationResult<OwnedColumn<S>> {
    struct AddOp {}
    impl ArithmeticOp for AddOp {
        fn op<T>(l: &T, r: &T) -> ColumnOperationResult<T>
        where
            T: CheckedAdd + Debug,
        {
            try_add(l, r)
        }

        fn decimal_op<S, T0, T1>(
            lhs: &[T0],
            rhs: &[T1],
            left_column_type: ColumnType,
            right_column_type: ColumnType,
        ) -> ColumnOperationResult<(Precision, i8, Vec<S>)>
        where
            S: Scalar + From<T0> + From<T1>,
            T0: Copy,
            T1: Copy,
        {
            try_add_decimal_columns(lhs, rhs, left_column_type, right_column_type)
        }
    }
    AddOp::element_wise_arithmetic(lhs, rhs)
}

pub fn element_wise_sub<S: Scalar>(
    lhs: &OwnedColumn<S>,
    rhs: &OwnedColumn<S>,
) -> ColumnOperationResult<OwnedColumn<S>> {
    struct SubOp {}
    impl ArithmeticOp for SubOp {
        fn op<T>(l: &T, r: &T) -> ColumnOperationResult<T>
        where
            T: CheckedSub + Debug,
        {
            try_sub(l, r)
        }

        fn decimal_op<S, T0, T1>(
            lhs: &[T0],
            rhs: &[T1],
            left_column_type: ColumnType,
            right_column_type: ColumnType,
        ) -> ColumnOperationResult<(Precision, i8, Vec<S>)>
        where
            S: Scalar + From<T0> + From<T1>,
            T0: Copy,
            T1: Copy,
        {
            try_subtract_decimal_columns(lhs, rhs, left_column_type, right_column_type)
        }
    }
    SubOp::element_wise_arithmetic(lhs, rhs)
}

trait ArithmeticOp {
    fn op<T>(l: &T, r: &T) -> ColumnOperationResult<T>
    where
        T: CheckedAdd + Debug;
    fn decimal_op<S, T0, T1>(
        lhs: &[T0],
        rhs: &[T1],
        left_column_type: ColumnType,
        right_column_type: ColumnType,
    ) -> ColumnOperationResult<(Precision, i8, Vec<S>)>
    where
        S: Scalar + From<T0> + From<T1>,
        T0: Copy,
        T1: Copy;

    fn element_wise_arithmetic<S: Scalar>(
        lhs: &OwnedColumn<S>,
        rhs: &OwnedColumn<S>,
    ) -> ColumnOperationResult<OwnedColumn<S>> {
        if lhs.len() != rhs.len() {
            return Err(ColumnOperationError::DifferentColumnLength {
                len_a: lhs.len(),
                len_b: rhs.len(),
            });
        }
        match (&lhs, &rhs) {
            (OwnedColumn::TinyInt(lhs), OwnedColumn::TinyInt(rhs)) => Ok(OwnedColumn::TinyInt(
                try_slice_binary_op(lhs, rhs, Self::op)?,
            )),
            // remaining branches
            _ => Err(ColumnOperationError::BinaryOperationInvalidColumnType {
                operator: BinaryOperator::Add,
                left_type: lhs.column_type(),
                right_type: rhs.column_type(),
            }),
        }
    }
}

@iajoiner iajoiner force-pushed the refactor/owned-arith branch 4 times, most recently from 30b608e to f2a3951 Compare November 11, 2024 03:13
@iajoiner iajoiner changed the title refactor: simplify arithmetic in owned_column_operation.rs refactor: split and simplify owned_column_operation.rs Nov 11, 2024
@iajoiner iajoiner changed the title refactor: split and simplify owned_column_operation.rs refactor!: split and simplify owned_column_operation.rs && remove unused functions from slice_operation.rs Nov 11, 2024
@iajoiner iajoiner force-pushed the refactor/owned-arith branch 4 times, most recently from ff2c1e3 to 0529e45 Compare November 11, 2024 04:15
Copy link
Contributor

@JayWhite2357 JayWhite2357 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Will approve after rebase. Why is this a breaking change?

crates/proof-of-sql/src/base/database/slice_operation.rs Outdated Show resolved Hide resolved
@iajoiner iajoiner force-pushed the refactor/owned-arith branch from 0529e45 to 6820389 Compare November 11, 2024 14:16
@iajoiner
Copy link
Contributor Author

@JayWhite2357 It is breaking since we removed the awkward Add, Sub, Mul and Div from OwnedColumn<S> which is public. We don't expect downstream projects to use these but they may.

@iajoiner iajoiner force-pushed the refactor/owned-arith branch from 50b3dbc to 11596ef Compare November 12, 2024 14:16
@iajoiner iajoiner merged commit c60c143 into main Nov 12, 2024
11 checks passed
Copy link

🎉 This PR is included in version 0.42.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@iajoiner iajoiner deleted the refactor/owned-arith branch November 12, 2024 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants