stantheman0128 commented on PR #5302: URL: https://github.com/apache/datafusion-comet/pull/5302#issuecomment-5749787794
Thanks. I chased the first point further than my September comment did, and it turned up a correction to something I told you plus three things I did not expect to find. **Where the cast actually goes** The first half has not changed. `CometBatchKernelCodegen.isSupportedDataType` matches `case _: StringType | _: BinaryType => true`, and on Spark 4 a collated `StringType` is still a `StringType`, so `canHandle` admits it. A collated cast that this PR marks `Unsupported` does reach the dispatcher. The correction is the second half. I told you the dispatcher never calls `serializeDataType`. That was wrong. `CometScalaUDF.emitJvmCodegenDispatch` declares the return type through it at `CometScalaUDF.scala:152`, and `serializeDataType` flattens every `StringType` to proto id 7 at `QueryPlanSerde.scala:615`. So the dispatcher's output column does arrive on the native side with the collation gone, which is what you suspected. It still does not change an answer on this route, for three reasons. The evaluation never reads the proto type. The kernel compiles the closure-serialized bound expression (`CometScalaUDFCodegen.scala:156-169`), whose `collationId` survives serialization, and runs Spark's own `doGenCode`. Arrow is the byte store for the `UTF8String`s that code produces. The Arrow output field agrees with the proto rather than contradicting it. `lookupOrCompile` builds the field from `boundExpr.dataType` (`CometScalaUDFCodegen.scala:172-175`), which goes through `Utils.toArrowType`'s `case _: StringType => ArrowType.Utf8.INSTANCE` (`Utils.scala:157`). Both sides flatten the same way, so there is no FFI type mismatch hiding in here. And nothing reads the flattened type back. The values are bytes, the kernel produced them, and whether a downstream operator may treat the column collation-blind is decided per operator against the Catalyst `DataType`, which keeps its collation. That decision does not depend on what the dispatcher admitted. So rejecting collated strings in `canHandle` would refuse a route that is already correct and put every collated cast on a full Spark fallback instead. I have written that up on `isSupportedDataType`, with a pointer to it from the guard in `CometCast`. I put it there and not on `canHandle` because `isSupportedDataType` is the line doing the admitting, and `canHandle`'s own doc already points at it. Happy to move it to where you pointed if you would rather have it there. **Three serde-level omissions your question turned up, none of them this PR's** My first draft of that comment claimed the downstream guards were exhaustive. They are not. What I can show you is three places carrying no collation test in the serde. What I cannot show you is a plan that reaches any of them, so please read the list with that limit attached. The limit matters because this repo guards collation systematically, and two of those guards sit upstream of everything below. `CometScanRule` refuses any collated column outright, with the comment that it is a convenient place to force the whole query back to Spark (`CometScanRule.scala:1073-1076`). Both partitionings are guarded as well, range at `CometShuffleExchangeExec.scala:537` and hash at `:432`. Grouping keys (`operators.scala:1931`), join keys in both the broadcast-hash and sort-merge paths (`:2410` and `:2968`) and the sort-merge equal-key type check (`:3029`) each have their own. So a collated column read from a table does not reach a native operator at all, and the shape that does get through is the one this PR's suite already uses: a plain column with `COLLATE` applied above it. Against that background, the three with no test. `supportedSortType` only type-checks single-key sorts. It opens with `if (sortOrder.length == 1)` (`QueryPlanSerde.scala:1287`) and its `else` returns `true` unconditionally (`:1301-1302`), while the single-key branch rejects collation through `supportedScalarSortElementType` at `:1276`. The two branches disagree with each other, which reads like an oversight rather than a decision. A multi-key global `ORDER BY` needs a range shuffle and that is guarded, so the shapes where this could bite are `sortWithinPartitions` and `TakeOrderedAndProject`. I have built neither. `hash()` and `xxhash64()` accept collated children. `CometMurmur3Hash` (`hash.scala:53`) and `CometXxHash64` (`:31`) route `getSupportLevel` to `HashUtils.supportLevelForChildren`, and `unsupportedReasonFor` (`:136-147`) has no collation case. This is the one I would call a plain omission rather than an unproven reachability claim, because the repo has already written down why it is wrong for the neighbouring case: `CometApproxCountDistinct` excludes collated strings at `aggregates.scala:1233-1244`, because Spark hashes them via the collation sort key, and says so in `getUnsupportedReasons` at `:1250`. Nothing in that reasoning is specific to approx_count_distinct. `CometWindowExec.convert` serializes `partitionSpec` and `orderSpec` straight through `exprToProto` (`CometWindowExec.scala:65-67`), which applies the per-expression serde gates but adds no collation gate of its own for a bare attribute key. A window partition needs a hash shuffle, guarded at `CometShuffleExchangeExec.scala:432`, so this carries the same reachability caveat as the sort. The neighbouring operator does have one: `CometWindowGroupLimitExec` filters its ordering through `hasNonDefaultStringCollation` at `:92`. That is what makes the window case look like an omission rather than a decision. None of the three is a confirmed divergence, and none is a confirmed reachable plan. I can file them as serde-level omissions, together or separately, and take the hash one myself if you want it fixed rather than only tracked. **The fallback reason** Changed, with your wording verbatim. `CometCast.nonDefaultCollationReason` holds the string and the guard returns it instead of `unsupported(fromType, toType)`. It is `private[comet]` and the suite reads it from production instead of retyping it, the way `negativeScaleDecimalToStringReason` is already shared with `CometNativeCastSuite`. Two notes on it. The first is a limit. The reason only reaches `EXPLAIN` when the dispatcher declines the expression, because `exprToProtoInternal` offers an `Unsupported` case to `dispatchIfFallback` first and calls `withFallbackReason` only if that returns `None` (`QueryPlanSerde.scala:965-980`). On default config a collated cast is accepted by the dispatcher and nothing is recorded. The user who sees this string is the one who turned `spark.comet.exec.scalaUDF.codegen.enabled` off or hit a `canHandle` refusal, which is also the user whose query really did fall back. So it lands where it matters, just not as widely as it first looks. The second is that it closed a weakness I flagged last round. I said the scalar end-to-end tests were guard-invariant, because `StringType(UTF8_LCASE) -> IntegerType` already exited through the `case _` catch-all with the same reason string. With a dedicated reason that stops being true. I checked by neutralising the guard and rerunning, and the assertion fails: ``` Expected fallback reason 'Cast involving a non-default string collation is not supported (https://github.com/apache/datafusion-comet/issues/4489)' not found in [Cast from StringType(UTF8_LCASE) to IntegerType is not supported, cast: spark.comet.exec.scalaUDF.codegen.enabled=false; expression has no native path so the plan falls back to Spark] ``` `cast.md` is unaffected. `supportedTypes` has no collated entry so the generator never asks for this pair, and `GenerateDocs` renders an `Unsupported` cell as `U` without the note anyway. **The 3.x walk** Already handled. `hasNonDefaultStringCollation` in the 3.x shim is a `false` literal with no match and no recursion (`spark/src/main/spark-3.x/org/apache/comet/shims/CometTypeShim.scala:41`). Nothing recurses on 3.4 or 3.5. On 4.x I do not think the walk is worth short-circuiting. The helper's own first case is the scalar one (`spark/src/main/spark-4.x/org/apache/comet/shims/CometTypeShim.scala:48-54`), so a plain `StringType` answers on that first arm and an `IntegerType` drops straight to the `case _` at `:54`. Neither one recurses. The walk only happens for array, map and struct types, and for exactly those the statement directly below the guard is `if (fromType == toType)`, where `DataType` equality is structural and walks both trees anyway. `canHandle` further along the same path then runs `numOfNestedFields` over the output type and a `collect` over the whole expression tree. The guard adds a constant factor to something already linear in the same quantity, once per cast, at plan time. A short-circuit would only make the guard harder to read for no measurable gain, so I left it out. I will add one if you disagree. **Also in this push** One more end-to-end test. The struct case is the only pair where the guard changes the answer and not just the reason string, and it was covered only with the dispatcher off. It now has the dispatcher-on half too, so both settings are exercised on both the scalar and the struct case. The suite header now carries the reason these are Scala tests rather than `CometSqlFileTestSuite` fixtures. That explanation was in the thread but not in the file, where the next reader would look. The branch is also rebased onto current `main`. The only collision was the `isSupportedDataType` Scaladoc, where #5766 added a paragraph on duplicate struct field names in the same place; both paragraphs are kept. The code change is unchanged at 315 added lines across five files with nothing removed. **Verification** Local, against a debug `libcomet.so` built from this branch after the rebase. - `CometCastCollatedStringSuite`, `spark-4.0`: 21 succeeded, 0 failed. - `CometCastCollatedStringSuite`, `spark-4.1`: 21 succeeded, 0 failed. - `CometCollationSuite` plus `CometNativeCastSuite`, `spark-4.0`: 211 succeeded, 0 failed, 8 ignored. - `CometCollationSuite` plus `CometNativeCastSuite`, `spark-4.1`: 206 succeeded, 0 failed, 8 ignored. - `CometSqlFileTestSuite` in full, `spark-4.1`: 550 fixtures, all passed. That covers all 16 collation fixtures, `expressions/string/collation.sql` among them. - Negative control: guard neutralised, `spark-4.0`, 11 of 21 fail, including the scalar end-to-end test and both struct end-to-end tests. Not run locally: 3.4, 3.5 and 4.2. The guard is inert on the 3.x profiles since the shim is a `false` literal. 4.2 has no `CometCollationSuite` today, which is part of the follow-up we discussed. CI has never executed on this PR. Every workflow run it has produced so far finished without starting a single job: some are still sitting at `action_required`, and the older ones were marked failed once the approval window lapsed. The red mark is that gate expiring rather than a test failure. If you can approve the workflows, that is the fastest way to get a real signal here. **One question still open from last round** `getSupportLevel` returns `Compatible()` for any cast whose child is a `Literal`, before `isSupported` runs, so `CAST('abc' AS STRING COLLATE UTF8_LCASE)` still reaches the native side with the collation stripped. `ConstantFolding` normally removes that cast first, but `CometSqlFileTestSuite` excludes `ConstantFolding` for every fixture it runs, so our own harness can reach it. Closing it means deciding what `CometLiteral` should do with a collated literal, not adding a line to `isSupported`. Do you want it in this PR or in its own issue? I will open the issue if I do not hear otherwise, so it does not sit as an untracked remark. -- 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]
