feat: add BinaryNumericFn for array arithmetic #1640

danking · 2024-12-10T22:21:09Z

I did not implement any binary numeric functions because it is not clear that there are any cases where we can out run decompression. Two run end arrays might be a happy path? Two dictionaries, maybe, if the dictionaries are much smaller than the decompressed arrays?

Binary scalar numeric functions are more obviously valuable: clickbench includes several uses of scalar add or subtract.

gatesn · 2024-12-10T23:01:20Z

Why ScalarNumeric instead of using is_constant as with all other compute functions?

danking · 2024-12-10T23:05:25Z

subtract_scalar was already present and this was the natural generalization.

We could remove subtract_scalar and friends and replace them with functions that create constant arrays of the right length and apply the binary operator and have the binary operators handle constants RHSes. What was the reasoning for subtract_scalar?

danking · 2024-12-11T16:59:22Z

Alright, scalar_subtract and scalar_numeric are completely gone. Four convenience functions survive: sub_scalar, etc. which delegate to binary_numeric.

I did not implement any binary numeric functions because it is not clear that there are any cases where we can out run decompression. Two run end arrays might be a happy path? Two dictionaries, maybe, if the dictionaries are much smaller than the decompressed arrays? Scalar numeric functions are more obviously valuable: clickbench includes several uses of scalar add or subtract.

gatesn · 2024-12-13T19:52:45Z

@danking none of our compute functions have internal casting.

Let's just check LHS and RHS are exactly equal (including null-ability) and fail if not. The caller has to decide casting rules, e.g. the compute engine, else it gets very confusing to keep track of coercion semantics

danking · 2024-12-13T19:55:41Z

@gatesn the old subtract_scalar casted the constant (which is what this PR preserves and extends to add, multiply, and divide), we seem to rely on this for shifting usize indices around. I could push the cast into the call sites though?

gatesn · 2024-12-13T20:00:35Z

Yeah I think the call site is best, albeit I bit more annoying

danking · 2024-12-13T20:07:19Z

@gatesn done.

vortex-array/src/array/chunked/mod.rs

vortex-array/src/array/constant/compute/mod.rs

vortex-array/src/compute/binary_numeric.rs

gatesn

Sorry it's taken me a while to get to this

vortex-array/src/array/chunked/mod.rs

vortex-array/src/array/constant/compute/mod.rs

vortex-array/src/array/null/compute.rs

vortex-scalar/src/primitive.rs

gatesn · 2024-12-14T10:52:58Z

vortex-scalar/src/primitive.rs

+        other: PrimitiveScalar<'_>,
+        op: NumericOperator,
+    ) -> VortexResult<Scalar> {
+        if !self.dtype().eq_ignore_nullability(other.dtype()) {


Why are we ignoring nullability? Not saying we shouldn't be (although maybe we shouldn't be), but if we do this it should be commented.

Supporting different nullabilities isn't difficult and does not seem to me likely to affect speed much since we're already working with Scalar rather than primitives. Compare works similarly. What kind of comment are you looking for?

Only because we of the general approach in Vortex that a compute function should never perform type coercion.

vortex-scalar/src/primitive.rs

danking

OK, I resolved all the threads that I think are uncontroversially resolved. I think the unresolved ones still need confirmation or more discussion.

vortex-array/src/array/chunked/mod.rs

vortex-array/src/array/constant/compute/mod.rs

vortex-array/src/array/null/compute.rs

vortex-array/src/compute/binary_numeric.rs

vortex-scalar/src/primitive.rs

danking · 2024-12-17T17:36:52Z

vortex-scalar/src/primitive.rs

+        other: PrimitiveScalar<'_>,
+        op: NumericOperator,
+    ) -> VortexResult<Scalar> {
+        if !self.dtype().eq_ignore_nullability(other.dtype()) {


Supporting different nullabilities isn't difficult and does not seem to me likely to affect speed much since we're already working with Scalar rather than primitives. Compare works similarly. What kind of comment are you looking for?

vortex-scalar/src/primitive.rs

gatesn · 2024-12-17T19:42:38Z

vortex-scalar/src/primitive.rs

+                let lhs = self.typed_value::<$P>();
+                let rhs = other.typed_value::<$P>();
+                match (lhs, rhs) {
+                    (_, None) | (None, _) => Some(Scalar::null(self.dtype().clone().as_nullable())),


This is the bug I think is still here. If (_, None) is true, and lhs is non-nullable, then you're going to try to create a Scalar::null that's non-nullable

This case is correct (b/c of the as_nullable) but now every case uses the same (least viable) nullability.

gatesn · 2024-12-17T19:45:01Z

vortex-scalar/src/primitive.rs

+            vortex_bail!("types must match: {} {}", self.dtype(), other.dtype());
+        }
+
+        let nullability = self.dtype().nullability();


I also think this should be let nullability = self.dtype.is_nullable() || other.dtype.is_nullable()

…me way regardless of values

danking marked this pull request as ready for review December 10, 2024 22:21

danking added the benchmark Run benchmarks on this branch label Dec 11, 2024

github-actions bot removed the benchmark Run benchmarks on this branch label Dec 11, 2024

danking added the benchmark Run benchmarks on this branch label Dec 11, 2024

github-actions bot removed the benchmark Run benchmarks on this branch label Dec 11, 2024

This comment was marked as outdated.

Sign in to view

danking force-pushed the dk/arithmetic branch from 557f141 to 2f9fc40 Compare December 11, 2024 18:45

danking added 3 commits December 13, 2024 14:47

remove irrelevant test

acdacc3

move tests to a more relevant location

6e171f9

danking force-pushed the dk/arithmetic branch from 18a0e3a to 6e171f9 Compare December 13, 2024 19:52

fix import

a78f7fc

and remove bad mod statement

c84262a

feat: prefer use-site casting

eb58e18

danking requested review from gatesn and lwwmanning December 13, 2024 20:07

remove scalar_numeric

704fceb

danking changed the title ~~feat: add BinaryNumericFn and ScalarNumericFn for array arithmetic~~ feat: add BinaryNumericFn for array arithmetic Dec 13, 2024

lwwmanning reviewed Dec 13, 2024

View reviewed changes

vortex-array/src/array/chunked/mod.rs Show resolved Hide resolved

lwwmanning reviewed Dec 13, 2024

View reviewed changes

vortex-array/src/array/constant/compute/mod.rs Outdated Show resolved Hide resolved

lwwmanning reviewed Dec 13, 2024

View reviewed changes

vortex-array/src/compute/binary_numeric.rs Outdated Show resolved Hide resolved

gatesn requested changes Dec 14, 2024

View reviewed changes

address comments

4ebb7ed

danking commented Dec 17, 2024

View reviewed changes

danking requested a review from gatesn December 17, 2024 18:13

lwwmanning approved these changes Dec 17, 2024

View reviewed changes

describe SQL null semantics in doc string

2a347e0

danking enabled auto-merge (squash) December 17, 2024 18:35

gatesn requested changes Dec 17, 2024

View reviewed changes

gatesn reviewed Dec 17, 2024

View reviewed changes

always take the join of the nullabilities, compute nullability the sa…

a30cd96

…me way regardless of values

danking requested a review from gatesn December 17, 2024 20:16

gatesn approved these changes Dec 17, 2024

View reviewed changes

danking merged commit fa08a07 into develop Dec 17, 2024
20 checks passed

danking deleted the dk/arithmetic branch December 17, 2024 20:17

doki23 mentioned this pull request Dec 24, 2024

implement subtract_scalar_fn for ConstantEncoding #1591

Closed

feat: add BinaryNumericFn for array arithmetic #1640

feat: add BinaryNumericFn for array arithmetic #1640

Uh oh!

Conversation

danking commented Dec 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gatesn commented Dec 10, 2024

Uh oh!

danking commented Dec 10, 2024

Uh oh!

danking commented Dec 11, 2024

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

gatesn commented Dec 13, 2024

Uh oh!

danking commented Dec 13, 2024

Uh oh!

gatesn commented Dec 13, 2024

Uh oh!

danking commented Dec 13, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gatesn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gatesn Dec 14, 2024

Choose a reason for hiding this comment

Uh oh!

danking Dec 17, 2024

Choose a reason for hiding this comment

Uh oh!

gatesn Dec 17, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danking left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danking Dec 17, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gatesn Dec 17, 2024

Choose a reason for hiding this comment

Uh oh!

danking Dec 17, 2024

Choose a reason for hiding this comment

Uh oh!

gatesn Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danking commented Dec 10, 2024 •

edited

Loading

gatesn Dec 17, 2024 •

edited

Loading