Use datum arithmetic scalar value #7375
Conversation
github-actions
bot
added
physical-expr
| /// Wrapping addition of `ScalarValue` | ||
| /// | ||
| /// NB: operating on `ScalarValue` directly is not efficient, performance sensitive code | ||
| /// should instead make use of vectorized array kernels | ||
| pub fn add<T: Borrow<ScalarValue>>(&self, other: T) -> Result<ScalarValue> { |
There was a problem hiding this comment.
The performance of this is likely worse, however, following #7358 the only use of this logic is within the range analysis framework, the median accumulator (to average two values in the event of an even number), and Range window functions, none of which I would consider performance critical
|
#7376 should resolve the median issue |
tustvold
force-pushed
the
use-datum-arithmetic-scalar-value
branch
from
5064a79
to
86b4453
Compare
github-actions
bot
removed
optimizer
tustvold
force-pushed
the
use-datum-arithmetic-scalar-value
branch
from
86b4453
to
4bc82e5
Compare
There was a problem hiding this comment.
This is great -- thank you @tustvold . I took the liberty of merging up from main (and tweaking the performance comment, because I couldn't help myself) but I think we should merge this PR once the tests are passing
| @@ -2072,53 +1188,48 @@ impl ScalarValue { | |||
| } | |||
| } | |||
|
|
|||
| /// Wrapping addition of `ScalarValue` | |||
| /// | |||
| /// NB: operating on `ScalarValue` directly is not efficient, performance sensitive code | |||
There was a problem hiding this comment.
I like the performance admonition 👍
These are all bug fixes in my mind |
Which issue does this PR close?
Part of #7353
Rationale for this change
This avoids duplicated arithmetic logic, and moves us closer to replacing the custom ScalarValue logic with upstream kernels (#7353)
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?
The upstream kernels have a couple of differences compared to the existing impls: