fix: binary_numeric is now correct (+ tests!) #1721
Conversation
Lessons learned: do not merge without at least basic tests. We check for a constant array but do not create a new constant array with the appropriate length for the child on which we want to delegate the binary numeric operation. That is now fixed. I also added a very basic test suite for applying the (now six) numeric operations on pairs of arrays where one of the two are constant. In order to properly support constant arrays on the left- hand-side, I added FlippedSub and FlippedDiv which allow us to commute/flip the operator so that we need not teach ConstantArray to check if its right-hand-side supports an operation on constants.
encodings/dict/Cargo.toml
Outdated
| @@ -30,6 +30,7 @@ workspace = true | |||
| [dev-dependencies] | |||
| criterion = { workspace = true } | |||
| rand = { workspace = true } | |||
| vortex-array = { workspace = true, features = ["test_util"] } | |||
There was a problem hiding this comment.
Oh I liked the test-harness! "util" is forbidden!
There was a problem hiding this comment.
I renamed the extant test_util to test_harness and adjusted all of this PR's features to test_harness.
| @@ -138,7 +142,7 @@ pub fn binary_numeric( | |||
|
|
|||
| // Check if RHS supports the operation directly. | |||
| if let Some(fun) = rhs.encoding().binary_numeric_fn() { | |||
| if let Some(result) = fun.binary_numeric(rhs, lhs, op)? { | |||
| if let Some(result) = fun.binary_numeric(rhs, lhs, op.flip_parameters())? { | |||
There was a problem hiding this comment.
Can you be consistent with naming for CompareFn, in this case op.swap()
| @@ -186,13 +190,114 @@ fn arrow_numeric( | |||
| let array = match operator { | |||
| BinaryNumericOperator::Add => arrow_arith::numeric::add(&lhs, &rhs)?, | |||
| BinaryNumericOperator::Sub => arrow_arith::numeric::sub(&lhs, &rhs)?, | |||
| BinaryNumericOperator::Div => arrow_arith::numeric::div(&lhs, &rhs)?, | |||
| BinaryNumericOperator::FlippedSub => arrow_arith::numeric::sub(&rhs, &lhs)?, | |||
There was a problem hiding this comment.
Ok, I'm convinced enough that we should keep these. But can we call it RSub RDiv (following Python)
vortex-array/src/compute/mod.rs
Outdated
| @@ -25,7 +27,7 @@ pub use take::{take, TakeFn}; | |||
|
|
|||
| use crate::ArrayData; | |||
|
|
|||
| mod binary_numeric; | |||
| pub mod binary_numeric; | |||
There was a problem hiding this comment.
Done. I added a pub mod test_harness to this file and pub used test_binary_numeric in there instead of conditionally pub useing it in the regular module.
encodings/dict/Cargo.toml
Outdated
| @@ -30,6 +30,7 @@ workspace = true | |||
| [dev-dependencies] | |||
| criterion = { workspace = true } | |||
| rand = { workspace = true } | |||
| vortex-array = { workspace = true, features = ["test_harness"] } | |||
There was a problem hiding this comment.
I think features are typically kebab case? Might be wrong, can change this later though, don't block the PR
There was a problem hiding this comment.
Done.
(Seemed slightly shitty that test-harness gave you a module named test_harness 🤷. @gatesn Should I fix canonical_counter in a separate PR?)
vortex-scalar/src/primitive.rs
Outdated
| } | ||
| } | ||
|
|
||
| pub fn math_symbol(&self) -> String { |
There was a problem hiding this comment.
What is this used for? If it's for display, it should just be the Display trait. But I don't think we should do this, since it feels like a foot-gun when printing, e.g. format!("{lhs} {op} {rhs}", ...) would be wrong for RDiv and Sub.
There was a problem hiding this comment.
Agreed. Too clever. Nixed.
vortex-scalar/src/primitive.rs
Outdated
| match self { | ||
| BinaryNumericOperator::Add => "+", | ||
| BinaryNumericOperator::Sub => "-", | ||
| BinaryNumericOperator::RSub => "+", |
There was a problem hiding this comment.
Also this seems not quite right
There was a problem hiding this comment.
@gatesn FWIW, this PR now has quite a few renames of test_util or test_utils to test-harness or test_harness (as appropriate for feature vs module). Can you verify this was what you had in mind?
I also fixed all the dev-dependencies to use workspace=true rather than path.
encodings/dict/Cargo.toml
Outdated
| @@ -30,6 +30,7 @@ workspace = true | |||
| [dev-dependencies] | |||
| criterion = { workspace = true } | |||
| rand = { workspace = true } | |||
| vortex-array = { workspace = true, features = ["test_harness"] } | |||
There was a problem hiding this comment.
Done.
(Seemed slightly shitty that test-harness gave you a module named test_harness 🤷. @gatesn Should I fix canonical_counter in a separate PR?)
vortex-scalar/src/primitive.rs
Outdated
| } | ||
| } | ||
|
|
||
| pub fn math_symbol(&self) -> String { |
There was a problem hiding this comment.
Agreed. Too clever. Nixed.
vortex-scalar/src/primitive.rs
Outdated
| match self { | ||
| BinaryNumericOperator::Add => "+", | ||
| BinaryNumericOperator::Sub => "-", | ||
| BinaryNumericOperator::RSub => "+", |
Lessons learned: do not merge without at least basic tests.
Four major changes:
binary_numeric.rsWe check for a constant array but do not create a new constant array with the appropriate length for the child on which we want to delegate the binary numeric operation. That is now fixed.
I also added a very basic test suite for applying the (now six) numeric operations on pairs of arrays where one of the two are constant.
In order to properly support constant arrays on the left-hand-side, I added FlippedSub and FlippedDiv which allow us to commute/flip the operator so that we need not teach ConstantArray to check if its right-hand-side supports an operation on constants.