andygrove commented on code in PR #5044:
URL: https://github.com/apache/datafusion-comet/pull/5044#discussion_r3691138122
##########
native/spark-expr/src/math_funcs/wide_decimal_binary_expr.rs:
##########
@@ -281,7 +281,13 @@ impl PhysicalExpr for WideDecimalBinaryExpr {
}
};
- let result = if eval_mode != EvalMode::Ansi {
+ let result = if eval_mode != EvalMode::Ansi &&
result.values().contains(&i128::MAX) {
Review Comment:
Did you consider tracking the overflow during the arithmetic pass instead of
scanning for the sentinel afterwards? `check_overflow_and_convert` already
knows the answer at the moment it writes the sentinel, so `contains` is
re-deriving a fact we had for free. `try_binary` takes an `Fn`, so a captured
`Cell<bool>` works and only gets written on the rare overflow branch.
That would drop the extra 128 KB read from both paths, which should make the
no-overflow win larger than the 8-10% you measured, and it removes the
regression risk on the overflow shapes entirely.
I realize the tradeoff is losing the symmetry with
`decimal_rescale_check.rs`, and the issue text did prescribe `contains`, so I
am happy to hear the case for keeping it as is. But the scan is essentially all
that is left of the cost here, so it seems worth weighing.
##########
native/spark-expr/benches/wide_decimal.rs:
##########
@@ -49,6 +50,30 @@ fn make_decimal_batch(p1: u8, s1: i8, p2: u8, s2: i8) ->
RecordBatch {
RecordBatch::try_new(Arc::new(schema), vec![Arc::new(left),
Arc::new(right)]).unwrap()
}
+fn make_overflow_batch(null_every: usize, overflow_every: usize) ->
RecordBatch {
Review Comment:
The two overflow shapes both end up with an overflow at index 0, since `0 %
17 == 0` and `0 % 2 == 0`. That means `contains` returns on the very first
element in both cases, so the sparse and dense benches never exercise a scan
longer than one element.
Could we add a shape where the only overflow sits at the last index? That is
the case where the guard costs the most, because you pay the full 8192-element
scan and then still pay the masking pass on top. It would be good to see a
number for it. Right now the "No significant change" results for the sparse and
dense shapes are really measuring the index-0 case, so they do not tell us much
about the overflow path.
##########
native/spark-expr/src/math_funcs/wide_decimal_binary_expr.rs:
##########
@@ -281,7 +281,13 @@ impl PhysicalExpr for WideDecimalBinaryExpr {
}
};
- let result = if eval_mode != EvalMode::Ansi {
+ let result = if eval_mode != EvalMode::Ansi &&
result.values().contains(&i128::MAX) {
+ // The arithmetic pass writes i128::MAX as an overflow sentinel
for values that do
+ // not fit the output precision. Only when a sentinel is present
do we need the
+ // extra null-masking pass (which allocates a new array);
`contains` short-circuits
+ // at the first sentinel, so the common no-overflow case skips
that allocation
+ // entirely. ANSI mode raises on overflow and never produces a
sentinel, so it also
+ // skips this pass.
Review Comment:
One thing about the wording. "`contains` short-circuits at the first
sentinel, so the common no-overflow case skips that allocation entirely" joins
two facts that are not actually connected. In the no-overflow case `contains`
does not short-circuit at all, it reads the entire buffer. The short-circuit
only helps when an overflow is present and found early.
Could we say instead that the no-overflow path trades the allocating masking
pass for a single read-only scan? Since this comment is the place a reader
learns the cost model, it would help for it to be explicit that the common path
is not free. The same wording exists in `decimal_rescale_check.rs`, so it is
probably worth fixing in both places.
##########
native/spark-expr/src/math_funcs/wide_decimal_binary_expr.rs:
##########
@@ -506,6 +512,23 @@ mod tests {
assert!(arr.is_null(0));
}
+ #[test]
+ fn test_overflow_with_nulls_legacy_mode() {
Review Comment:
Good test, and I like that `test_null_propagation` already covers the
complementary shape where the fast path runs and nulls have to survive.
Since the guard now also governs the Scalar x Scalar path, it might be worth
adding a companion to `test_scalar_scalar_returns_scalar` that overflows in
legacy mode and asserts the result comes back as `ScalarValue::Decimal128(None,
p, s)` rather than an array or a raw sentinel. That path routes through
`ScalarValue::try_from_array` after the masking pass, so it seems worth pinning
down.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
