tlfua commented on code in PR #7456:
URL: https://github.com/apache/datafusion-comet/pull/7456#discussion_r4521507130
##########
native/core/src/execution/planner.rs:
##########
@@ -1541,23 +1523,36 @@ impl PhysicalPlanner {
None,
eval_mode,
)?;
- let scalar_expr = Arc::new(ScalarFunctionExpr::new(
- op_str,
- fun_expr,
- vec![left, right],
- Arc::new(Field::new(op_str, data_type, true)),
- Arc::new(ConfigOptions::default()),
- ));
-
- if query_context.is_some() {
- Ok(Arc::new(CheckedBinaryExpr::new(scalar_expr,
query_context)))
- } else {
- Ok(scalar_expr)
- }
+ (
+ Arc::new(ScalarFunctionExpr::new(
+ op_str,
+ fun_expr,
+ vec![left, right],
+ Arc::new(Field::new(op_str, data_type, true)),
+ Arc::new(ConfigOptions::default()),
+ )),
+ true,
+ )
} else {
- Ok(Arc::new(BinaryExpr::new(left, op, right)))
+ (Arc::new(BinaryExpr::new(left, op, right)), false)
}
}
+ };
+ let expr = if options.null_short_circuit {
+ // Spark evaluates the divisor of a division first
+ let order: &[usize] = if op == DataFusionOperator::Divide {
+ &[1, 0]
+ } else {
+ &[0, 1]
+ };
+ NullShortCircuit::wrap_in_order(expr, order, &input_schema)?
Review Comment:
Done in b948e2a05. `Cast::cannot_fail` now accepts a cast from `TINYINT`,
`SMALLINT`, `INT` or `BIGINT` to a `DECIMAL(p, s)` with `s >= 0` and at least
3, 5, 10 or 19 integral digits (`p - s`), which holds every value of the
integer. So the `CAST(a AS DECIMAL(19, 0))` operands of `a DIV b` and of `d +
a` are no longer wrapped.
`test_cannot_fail_casts_succeed_on_extreme_values_in_every_mode` in `cast.rs`
casts the minimum and maximum of each type in every eval mode, and
`test_casts_that_can_fail_are_not_claimed` checks that one digit fewer is not
accepted.
I put it in `cannot_fail` rather than `is_infallible`, because
`is_infallible` also decides which CASE WHEN branches are evaluated over the
whole batch up front, and these casts cost more per row than the ones it holds.
##########
spark/src/main/scala/org/apache/comet/serde/arithmetic.scala:
##########
@@ -208,7 +219,8 @@ object CometAdd extends CometExpressionSerde[Add] with
MathBase {
binding,
expr.dataType,
expr.evalMode,
- (builder, mathExpr) => builder.setAdd(mathExpr))
+ (builder, mathExpr) => builder.setAdd(mathExpr),
+ skippable = Some(expr.right))
Review Comment:
Good catch, thanks. Each balanced node now sets the flag from its right
slice: `createBalancedMathExpr` keeps each operand next to its proto and calls
`canSkipEvaluation` on the operands of the right half. A node skips its right
half where its left half is NULL, which is where some earlier operand is NULL,
so every operand is evaluated on the same rows as in Spark's left-deep chain.
The new `math/arithmetic_legacy_skipped_operands.sql` runs chains of 2 to 5
operands under LEGACY, including one that starts with a `BIGINT` cast, whose
last operand `element_at(arr, i)` fails on the rows Spark skips, and checks
that it still raises `INVALID_INDEX_OF_ZERO` where Spark evaluates it, and
where it comes first.
--
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]
