tlfua opened a new pull request, #7456: URL: https://github.com/apache/datafusion-comet/pull/7456
## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123. --> Part of https://github.com/apache/datafusion-comet/issues/7246. It fixes the arithmetic and `concat` rows of that issue. `OR`, `AND` and `IN` were fixed by https://github.com/apache/datafusion-comet/pull/7326, and the later sort keys of a top-K are left for a separate PR, so the issue stays open. ## Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> With ANSI mode on, native Comet evaluates an operand over the whole batch even on rows where Spark never evaluates it, so a query that Spark answers fails in Comet with `CAST_INVALID_INPUT`: - `+`, `-` and `*` evaluate the right operand only where the left one is not NULL (`BinaryExpression.nullSafeCodeGen`). - `/`, `DIV` and `%` evaluate the divisor first, and the dividend only where the divisor is not NULL (`DivModLike.doGenCode`, the same in Spark 3.4 and 4.1). - `concat` evaluates each argument only while no earlier one is NULL (`Concat.doGenCode`, Spark's default generated code). For example `SELECT n + CAST(txt AS INT) FROM t` returns NULL in Spark wherever `n` is NULL, whatever `txt` holds, but fails in Comet when such a row has a malformed `txt`. ## What changes are included in this PR? <!-- There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR. --> The fix reuses `NullShortCircuit`, which already evaluates later function arguments only where no earlier one is NULL (https://github.com/apache/datafusion-comet/pull/6716). - `NullShortCircuit::wrap_in_order` evaluates the arguments in a given order, so that the divisor of a division comes first. `wrap` keeps the order of the children. - `MathExpr` gains `null_short_circuit`. The serde sets it when the operand that Spark may skip can be skipped (`canSkipEvaluation`), which is the right operand of `+`, `-` and `*` and the left one of `/`, `DIV` and `%`. The planner then wraps the arithmetic, including the checked ANSI functions, decimal arithmetic and the Decimal256 remainder (`create_modulo_expr_with_null_short_circuit`), keeping `CheckedBinaryExpr` outermost so errors still carry the query context. The rebalanced LEGACY add and multiply chains are unchanged. - `CometConcat` wraps the native `concat` with `withNullShortCircuit` when it has more than one argument. - A masked argument that is deterministic is now first evaluated over the whole batch, as it was before this change, and the rows are selected only if that fails, as `LazyLogical` does since https://github.com/apache/datafusion-comet/pull/7326. Without that, `a * (b - c)` under ANSI, as in TPC-H Q1, was up to 2x slower on batches with NULLs, because selecting the rows costs more than the arithmetic. `is_deterministic` moves from private to `pub(super)` for this. - `create_binary_expr` lost its last caller and is removed. Its tests call `create_binary_expr_with_options`. An operand that Spark does evaluate still fails as before. A wrapped operand is NULL only on rows whose result is NULL anyway. The same bug is on `branch-1.2`. That branch has `NullShortCircuit` but not https://github.com/apache/datafusion-comet/pull/7326, which this builds on, so I have not proposed a backport. Maintainers can decide. ## How are these changes tested? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? --> - `spark/src/test/resources/sql-tests/expressions/math/arithmetic_ansi_skipped_operands.sql` and `spark/src/test/resources/sql-tests/expressions/string/concat_ansi_skipped_arguments.sql` run every operator over a batch where the failing operand is malformed only on rows that Spark skips, and over one where every row is skipped. They cover int, bigint, decimal and the Decimal256 paths. `expect_error_class(CAST_INVALID_INPUT)` queries check that an operand Spark evaluates still fails, including the reverse order (`CAST(txt AS INT) + n`, `n DIV CAST(txt AS INT)`). Before the fix, the first `query` in each file fails with `CAST_INVALID_INPUT`. - Rust unit tests in `null_short_circuit.rs` and `modulo_expr.rs`: the divisor is evaluated first, it still fails where the dividend is NULL, `order` must be a permutation, the order survives `with_new_children`, speculative evaluation falls back to selected rows, and the Decimal256 remainder skips the dividend. - `native/spark-expr/benches/null_short_circuit.rs` compares the eager and wrapped forms of `a + CAST(s AS BIGINT)` and `a * (b - c)` at 0, 10 and 50% NULL. All six are within 1-5% (about 0.1-0.5 µs per 8192 rows) of the eager form. `a * (b - c)`, the slowest shape before speculation (µs per 8192-row batch, longer rerun): | NULLs | eager | wrapped | |---|---|---| | 0% | 10.67 | 11.13 | | 10% | 14.19 | 14.27 | | 50% | 10.08 | 10.63 | A nested wrapped expression, such as the `b - c` inside `a * (b - c)`, is not recognized as deterministic by `is_deterministic`, so it is evaluated on the selected rows rather than speculatively. It is still correct, and the numbers above include that cost. Commands run locally (macOS, default Spark profile): - `./mvnw test -Dsuites="org.apache.comet.CometSqlFileTestSuite skipped" -Dtest=none`: passed (both new files). - `./mvnw test -Dsuites="org.apache.comet.CometSqlFileTestSuite arithmetic" -Dtest=none` and `... concat`: passed (existing files). - `cargo test -p datafusion-comet-spark-expr --lib -- null_short_circuit modulo lazy`: 54 passed. - `cargo test -p datafusion-comet --lib -- planner`: 122 passed. - `cargo clippy --all-targets --workspace -- -D warnings`, `cargo fmt --all --check`, `./mvnw spotless:check`, `prettier --check` on the changed doc: passed. - `cargo bench -p datafusion-comet-spark-expr --bench null_short_circuit`: numbers above. I did not run the Spark SQL suite or the other Spark profiles locally. This changes the serde and native expressions, so a maintainer may want to apply `run-spark-4.1-tests`. -- 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]
