stantheman0128 commented on PR #5302:
URL:
https://github.com/apache/datafusion-comet/pull/5302#issuecomment-5840764717
Thanks for the approval, and for filing #6158.
You're right about `main` and the struct test. The field zip answers
`Compatible` there, but the query still falls back to Spark because `Collate`
has no serde: `exprToProtoInternal` finds no handler for it and records
`collate is not supported` (`QueryPlanSerde.scala:1034-1040` at `1c25b492`).
The dispatcher never gets a look, because the cast answered `Compatible` and
that arm converts directly. So the guard does change the outcome, by moving the
query from that fallback into the dispatcher, and the comment now says so
(9dd2e7f6). The description says the same now. Two other sentences in it made a
softer version of the same claim ("let a collated type reach the native plan"
and "the plan reached the native side"), so I reworded them to talk about what
`isSupported` answers, along with the matching comments in the test file. While
rereading I also found that the description and the array-of-nulls comment said
the `elementType == NullType` branch runs ahead of everything else. Tha
t was wrong, since `(NullType, _)` and the identity shortcut come first. Both
now say the branch answers without looking at the target element type.
Agreed on hash, after reading `hash.scala` at v4.0.1: `Murmur3Hash` and
`XxHash64` both set `isCollationAware = false`, and the sort-key path only runs
under `COLLATION_AWARE_HASHING_ENABLED`, so at default config raw-byte hashing
gives the same result as Spark. I only read 4.0.1 closely, though the flag is
still in `hash.scala` on Spark's default branch. I had carried the reasoning
over from the `CometApproxCountDistinct` note without reading `hash.scala`
itself.
The window order-spec case I listed has the same shape as the repro in
#6158, so all three omissions are covered: sort and window by #6158, and hash
is not a divergence. #6158 already has an assignee, so I'll stay off it.
I opened #6232 for the collated literal in `CometLiteral` and #6233 for
moving `CometCollationSuite` to `spark-4.x`.
The new commit only touches comments. After the change I reran
`spotless:check` and `CometCastCollatedStringSuite` on `spark-4.1` and the
suite on `spark-4.0`; all 21 tests pass on both.
--
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]
