Visorgood commented on PR #5558: URL: https://github.com/apache/datafusion-comet/pull/5558#issuecomment-5512631950
Thanks both, these were good catches. Pushed a revision that addresses all of them. **eager argument evaluation** Confirmed and fixed. I reproduced it before changing anything: with no guard, `SELECT array_join(arr, element_at(delims, idx)) FROM t WHERE arr IS NULL` throws inside Comet rather than returning null, exactly as was described. `convert` now nests the `array_to_string` call inside `IsNull` guards in Spark's own evaluation order (array, then delimiter, then replacement). This works because Comet's `IfExpr` delegates to DataFusion's `CaseExpr`, which evaluates each branch against a filtered `remainder_batch` – so a guarded argument is never evaluated on the short-circuited rows. New fixture `array_join_null_array_guard.sql` covers it; removing the guard makes it fail. Guards are emitted only where they protect something: earlier arguments are guarded only when a *later* one is non-foldable, and non-nullable arguments never are. So `array_join(arr, ',')` and `array_join(arr, ',', 'X')` keep the exact plan they had before this PR. **double evaluation of the replacement** Indeed there is no way to bind a value once in a serialized expression tree. Rather than paper over it, `getSupportLevel` now reports `Incompatible` when any argument that needs a guard is non-deterministic, so those route through the codegen dispatcher where Spark's single evaluation is preserved. The `monotonically_increasing_id() % 2` example takes that path. **stale `withSQLConf`** Dropped. That test now exercises the default path over the 10000-row `dictionaryEnabled` Parquet matrix, plus one added query with a nullable non-foldable delimiter and replacement to cover the guarded shape. **tests cannot distinguish native from the dispatcher** Agreed. I checked, and reverting `Compatible()` to `Incompatible` left every `.sql` fixture green. Added `array_join support level pins the native path`, which asserts the verdict directly, and `array_join emits a null guard only where it is needed`, which asserts the guard is emitted only in the cases above. I verified both fail when the behavior they pin is removed. **extra test cases** Added non-string element types (`array(1, 2, 3)`, `array(1, NULL, 3)` with and without a replacement, plus decimal and boolean) and the empty-string replacement, since `''` versus `NULL` is exactly the distinction the guard draws. Docs: the audit entry and the `expressions.md` note now describe the guards and the non-deterministic carve-out. Verified locally on Spark 3.4.3, 3.5.9, 4.0.4 and 4.1.3 with `CometArrayExpressionSuite` in full alongside the sql-tests, scalastyle and spotless enabled: 52 passing on 4.0/3.5/4.1, 50 plus 2 `isSpark35Plus` cancellations on 3.4. On CI: the earlier `macos-14/Spark 4.0 [scans]` failure looks unrelated to this change. Could someone re-approve the workflows so this gets a clean run? -- 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]
