andygrove commented on PR #6716:
URL:
https://github.com/apache/datafusion-comet/pull/6716#issuecomment-6097780759
Thanks for the careful pass. All four are addressed in dd6da4237d, on top of
a merge of `main` (24c02701af). The merge's only conflict was
`CometArrayContains`: #6599's `spark_array_contains` now goes through
`withNullShortCircuit` like the other functions.
**Non-ANSI cost.** `Cast::is_infallible` now accepts a cast from `Utf8` or
`LargeUtf8` to an integer in LEGACY or TRY mode. No LEGACY path raises there.
The LEGACY and TRY parsers in `string.rs` only ever return `Ok(None)` for a
string they can't parse or an out-of-range value, and only the ANSI parser
builds an error. So with ANSI off, `array_contains(ids, CAST(s AS INT))` plans
unwrapped, and `CASE` keeps its eager path for such a cast.
`infallible_expressions` checks all four integer widths in both modes for
`Utf8` and `LargeUtf8`. It also checks that the ANSI cast and a LEGACY cast to
`DOUBLE` stay fallible.
**Planning cost.** `withSharedSubexpressions` now stores a lazy holder, so
`EquivalentExpressions` runs only on the operator's first
`canShortCircuitNulls` call. `canShortCircuitNulls` also skips it when every
later argument is a leaf, since subexpression elimination never shares a column
or a literal. A scan or an operator without one of these functions never runs
it.
**Siblings.** `map_from_arrays` doesn't fail here. `CometMapFromArrays`
already nests `CASE WHEN keys IS NOT NULL` and `CASE WHEN values IS NOT NULL`,
and `CASE` evaluates a fallible branch only on the rows it selects.
`map_null_short_circuit.sql` now runs your query and gets Spark's NULL. It does
evaluate both arrays twice, so moving it to the flag is worth doing, but #5854
rewrites that serde, so I'd rather do it after that lands. For `array_repeat`,
Spark's `eval` reads the count first, but its generated code evaluates the
element first. On 3.4.3 with whole-stage codegen on, `array_repeat(CAST(s AS
INT), cnt)` over `('bad', NULL)` raises `CAST_INVALID_INPUT` in Spark as well
as in Comet, so nothing differs by default. #6850 tracks those two, moving
`element_at` to the flag, the shared-argument case where Spark doesn't
eliminate subexpressions, and dropping `CometArrayContains.eagerEvalReason`,
which the short-circuit makes unnecessary.
**Tests.** `array_null_short_circuit_non_ansi.sql` runs every array shape
with `spark.sql.ansi.enabled=false`. The casts return NULL there, while the
`slice` start of 0 and the `rand` argument still depend on the short-circuit.
`array_null_short_circuit_shuffle.sql` pins `spark.comet.batchSize=8192`.
On Spark 4.1, the `expressions/array/`, `expressions/map/` and
`expressions/conditional/` fixtures plus `CometArrayExpressionSuite` and
`CometMapExpressionSuite` passed 241 tests. The array and map coverage passed
210 on 3.4. All 1,184 `spark-expr` tests pass, clippy is clean on stable and
nightly, and `cargo fmt` is clean.
--
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]
