tlfua opened a new pull request, #7457: URL: https://github.com/apache/datafusion-comet/pull/7457
## Which issue does this PR close? Part of https://github.com/apache/datafusion-comet/issues/7246 (the top-K case). The `OR`/`AND`/`IN` cases were fixed by https://github.com/apache/datafusion-comet/pull/7326; the arithmetic and `concat` cases are handled in a separate PR. ## Rationale for this change With `spark.sql.ansi.enabled=true`, `SELECT a, b FROM topk ORDER BY a, b + 1 LIMIT 2` over `(1, MaxInt), (2, 5), (3, MaxInt), (4, 7), (5, 9)` returns `(1, 2147483647), (2, 5)` in Spark but fails with `ARITHMETIC_OVERFLOW` in Comet. Spark's `TakeOrderedAndProjectExec` compares rows with `LazilyGeneratedOrdering`, whose generated comparator (`GenerateOrdering.genComparisons`) evaluates sort key *i* for a pair of rows only when keys *0..i-1* tie. Comet's native TopK (DataFusion `SortExec` with `fetch`) evaluates every sort key over the whole batch, so a later key that can fail raises on rows Spark never evaluates it for. Making the native TopK evaluate later keys lazily was considered and rejected for this PR: which row pairs Spark compares depends on Guava's `Ordering.leastOf`, and DataFusion's TopK keeps a heap across batches, so per-batch masking cannot reproduce Spark's evaluation set without a custom operator. A scoped fallback is correct and small, in line with https://github.com/apache/datafusion-comet/issues/6006. ## What changes are included in this PR? - `CometTakeOrderedAndProjectExec.getSupportLevel` returns `Incompatible` when any sort key after the first can fail. The first key is evaluated for every row in Spark too, so it is not restricted. `spark.comet.operator.TakeOrderedAndProjectExec.allowIncompatible=true` still runs it natively. - `QueryPlanSerde.cannotFail(expr)`: a conservative whitelist, mirroring the native `cannot_fail`: columns, literals, comparisons, boolean logic, null checks, `If`/`CaseWhen`/`Coalesce`, float normalization, numeric arithmetic / negation / `abs` outside ANSI mode, and casts that cannot fail. Anything else is treated as able to fail. - `CometCast.cannotFail(cast)`: TRY casts, LEGACY casts between atomic types, and ANSI casts that `keyCastCannotFail` already proves exact. - `docs/source/user-guide/latest/compatibility/operators.md` documents the fallback. Known trade-off: the whitelist is conservative, so a later key built from a function outside it (for example `upper(b)`) also falls back even though it cannot fail. A plain `SortExec` without a limit likely has the same lazy later-key semantics in Spark; that is left for a follow-up. ## How are these changes tested? New SQL file test `spark/src/test/resources/sql-tests/operators/topk_later_key_ansi.sql` (ANSI on, AQE off): - the reproduction from the issue and two more fallible later keys (`expect_fallback`), - later keys that cannot fail, and a fallible *first* key, stay native (`query`), - rows that tie on the first key still raise `ARITHMETIC_OVERFLOW` (`expect_error`), - with ANSI off, the same queries plus `-b`/`abs(b)` stay native. Commands run locally (Spark 4.1 default profile, macOS): - `./mvnw test -Dsuites="org.apache.comet.CometSqlFileTestSuite topk_later_key" -Dtest=none -Dmaven.gitcommitid.skip=true` without the fix: **failed** with `ARITHMETIC_OVERFLOW` on `ORDER BY a, b + 1 LIMIT 2`, as expected. - Same command with the fix: **passed** (1 succeeded, 0 failed). - `./mvnw spotless:apply -Dmaven.gitcommitid.skip=true`: no changes. Broader regression coverage (other SQL file tests, TPC-DS plan stability, Spark SQL tests) is left to CI. Since this touches the serde, the Spark SQL suite should be run via the `run-spark-4.1-tests` label. -- 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]
