kazuyukitanimura commented on PR #7025:
URL:
https://github.com/apache/datafusion-comet/pull/7025#issuecomment-6200985486
Thanks for the careful review. @andygrove I've pushed changes for the
code, docs and test points. Two items need a decision or a test run first.
Overlap with #5867. I agree we should settle this before either merges.
The two PRs make different trade-offs:
- #5867 sends ANSI plus a nullable array through the JVM codegen
dispatcher as Incompatible, so the native guard only runs under
allowIncompatible.
- This PR keeps the native path for every item and makes it correct, by
evaluating the item in the ELSE branch. It costs one extra evaluation of the
item on rows whose array is NULL, and none when the item is a literal or column
read.
If #5867 lands first, the native fix is reachable only under
allowIncompatible, and this PR would need a rebase over the rewritten serde. If
this lands first, #5867 would need to drop its ANSI Incompatible routing for
array_append, and its array_append_ansi_null_array.sql repro would pass
natively. I have no strong preference.
Nondeterministic operands and nullability.
- I now decline when the array is nullable and either child is
nondeterministic, so a nondeterministic array on the left is covered too. It is
serialized in the guard and in THEN, so it drifted the same way.
- For a non-nullable array, I changed convert to emit no CASE at all. Each
operand is evaluated once over the whole batch, in order, so this no longer
depends on DataFusion skipping the filter. Unlike #5867, a nondeterministic
item with a non-nullable array therefore stays native. If you'd rather match
#5867 and decline regardless of nullability, that is a one-line change.
Docs. I added getUnsupportedReasons() for the nondeterministic fallback,
and updated the array_append audit note.
Tests. array_append_eager_eval.sql now also covers:
- a nondeterministic item and a nondeterministic array, both falling back;
- a nondeterministic item with a non-nullable array, via expect_native;
- a throwing item on a non-nullable array;
- a table where every array is NULL and one where none is, each with a
throwing and a non-throwing item;
- nested array and map items that go through widenElementInLockstep.
Verification. I'll apply the Spark 3.5 run-* label or run it myself and
post the result here. The new expectations haven't been executed yet, so a few
may need adjusting. The nested-map expect_error case is the one I'm least sure
of.
branch-1.1. Thanks for checking. I agree with your reading of both
behavior changes.
--
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]
