Re: [PR] fix: reject casts involving non-default collated strings [datafusion-comet]

["stantheman0128 (via GitHub)" <gi...@apache.org>]

stantheman0128 commented on PR #5302:
URL: https://github.com/apache/datafusion-comet/pull/5302#issuecomment-5272105832

   Thanks, this was a genuinely useful review. I had the dispatch semantics backwards and that wording is now fixed everywhere it was copied.
   
   **`Unsupported` does not mean "falls back to Spark"**
   
   You are right, and I should have caught this from the repo itself. `collation.sql` already spells out the same mechanism for the predicate serdes, and `predicates.scala` carries the comment explaining it. I traced `exprToProtoInternal` again and the `Unsupported` branch really does reach `dispatchIfFallback` before any reason is recorded. Every test name, the suite Scaladoc, and the PR description now say the cast has no native path.
   
   **The guard**
   
   Added, three lines, above the `fromType == toType` shortcut so identity casts are covered. It calls `hasNonDefaultStringCollation` the same way the array, collection, datetime, map, predicate, and string serdes already do. The `lcase -> lcase` `Compatible()` baseline is gone, since the guard makes that pair `Unsupported`. The default-collation baseline stays and I added a nested version of it, so the guard cannot over-block without a test noticing.
   
   **Nested types**
   
   Covered: `ArrayType(lcase) -> ArrayType(unicode)`, the identity array case, a struct with a collated field, a map with a collated key, a map with a collated value, and `ArrayType(lcase) -> StringType` for the recursion at `CometCast.scala:203`.
   
   Working through which pairs the guard actually changes turned up three more that were `Compatible` on main, so I added tests for those too rather than leave the over-block surface undescribed. A struct whose collated field is unchanged while a sibling field is cast came out `Compatible`, because the field zip answered per field and the collated field hit the identity shortcut. `MapType(lcase, IntegerType) -> MapType(lcase, LongType)` did the same through the key. And `ArrayType(NullType) -> ArrayType(lcase)` was `Compatible` through the `elementType == NullType` branch at `CometCast.scala:199`, which runs before any of the rest. So the collated type could ride into the native plan on a sibling's cast in each case.
   
   The remaining nested pairs already returned `Unsupported`. The guard only moved which type the reason string names.
   
   **Scaladoc correction**
   
   Fixed. The `IntegerType -> lcase` explanation now points at `canCastFromInt`'s catch-all, and the test for that pair carries a comment saying why `(_, DataTypes.StringType)` never matches a collated target.
   
   **End-to-end coverage**
   
   Added both, your query and the `=true` counterpart, and the reason surfaces exactly as you predicted.
   
   One thing I would rather flag myself than let read better than it is. Those two are guard-invariant. `(StringType(UTF8_LCASE), IntegerType)` already fell through to the `case _` catch-all on main and produced the same reason string, so both pass with the guard reverted. They pin down the planner's treatment of an `Unsupported` cast, which is the thing your first point corrects, but they do not exercise the new check.
   
   I tried to add one that does, using the identity pair the guard actually changed, and it turns out that hits the case you asked me to document. `CAST(_1 COLLATE utf8_lcase AS STRING COLLATE UTF8_LCASE)` never reaches Comet as a cast, because Spark's `SimplifyCasts` drops a cast whose child already has the target type. The query arrives at the planner as a bare `Collate` and the only fallback reason on the plan is `collate is not supported`, from a different serde:
   
   ```
   Expected fallback reason 'Cast from StringType(UTF8_LCASE) to StringType(UTF8_LCASE) is not
   supported' not found in [collate is not supported]
   ```
   
   So the scalar identity pairs stay pinned at the `isSupported` level, with a comment in the suite explaining why, written in the style of the join tests in `CometCollationSuite`.
   
   A struct turned out to be the way in. When a sibling field changes type the cast survives `SimplifyCasts` and the collated field rides along inside it, so there is now a third end-to-end test on `CAST(struct(_2 AS a, _1 COLLATE utf8_lcase AS s) AS STRUCT<a: STRING, s: STRING COLLATE UTF8_LCASE>)`. That one does fail without the guard, because the old field zip answered `Compatible` and the struct went native with the collation dropped. The reason it produces is:
   
   ```
   Cast from StructType(StructField(a,IntegerType,true),StructField(s,StringType(UTF8_LCASE),true))
   to StructType(StructField(a,StringType,true),StructField(s,StringType(UTF8_LCASE),true))
   is not supported
   ```
   
   Local runs are the `spark-4.0` and `spark-4.1` profiles, against a debug `libcomet.so` built from this branch. 3.4, 3.5, and 4.2 first execute in CI. On the 3.x profiles the guard is inert anyway, since `hasNonDefaultStringCollation` is a `false` literal in the 3.x shim.
   
   **Why these are Scala tests and not SQL fixtures**
   
   The `review-comet-pr` skill checked into this repo says expression tests should use `CometSqlFileTestSuite` where it can express them, so I should say why most of this suite does not.
   
   The bulk of the file asserts on `CometCast.isSupported` over type pairs that SQL cannot construct. `ArrayType(NullType) -> ArrayType(STRING COLLATE UTF8_LCASE)`, and a struct whose collated field is unchanged while a sibling field is cast, are only reachable by building the types in Scala. That is the carve-out the skill already allows for.
   
   The three end-to-end tests were closer to workable as a fixture, but `--Config` and `--ConfigMatrix` are both file scoped, and `ConfigMatrix` reruns every query in the file under every combination. The query that has to assert a fallback reason with `spark.comet.exec.scalaUDF.codegen.enabled=false` and the query that has to assert native execution with it set to true therefore cannot share a file. Expressing them as fixtures means a file per query for what is two lines of SQL each. I am happy to split them out that way if you would rather have them there.
   
   I did run the existing fixtures as part of the blast radius. `CometSqlFileTestSuite` passes on 4.1 with the guard in, including `collation.sql`, which is the fixture most exposed to this change since it casts a default-collation column to a collated target on nearly every query.
   
   **One gap I did not close**
   
   `getSupportLevel` returns `Compatible()` for any cast whose child is a `Literal`, before `isSupported` is consulted, 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 file it runs, so the path is reachable inside our own harness. It is the same class of problem as #4489 but it sits in a different method, and closing it means deciding what `CometLiteral` should do with a collated literal rather than adding a line to `isSupported`. Would you rather I pulled it into this PR or filed it separately?
   
   **Moving `CometCollationSuite` to `spark-4.x`**
   
   I looked at this and I do not think it fits inside this PR. Three things came up.
   
   There is already a second copy at `spark/src/test/spark-4.1/org/apache/spark/sql/CometCollationSuite.scala`, added by #4097. On the 4.1 profile, `src/test/spark-4.x` and `src/test/spark-4.1` are both test source roots (`spark/pom.xml:538-540`), so moving the 4.0 copy up without deleting that one gives two classes with the same fully qualified name.
   
   The two copies differ by exactly the #4051 join block, and Spark 4.1 looks like the reason. `BroadcastHashJoinExec` and `ShuffledHashJoinExec` became `case class ... private` there, with an explicit companion `apply` that runs `HashJoin.normalizeJoinKeys`. That wraps collated keys in `CollationKey`, whose `dataType` is `BinaryType`. So on 4.1 and 4.2 the exec never receives a collated key, Comet's guard has nothing to reject, and the two `result.isEmpty` assertions would not hold. `SortMergeJoinExec` is still a plain public case class on both, so that one test would survive a move.
   
   There is no `spark/src/test/spark-4.2` directory at all, so 4.2 has no `CometCollationSuite` today. A move would run the shuffle and datetime tests there for the first time, which is the real payoff in your suggestion and also the part most likely to surface something new.
   
   So the move is worth doing, but it means reconciling two divergent copies and deciding what happens to the #4051 join tests on 4.1 and later. Happy to file an issue and take it as a follow-up if you agree that is the right shape.
   
   One thing that fell out of the above and may deserve its own issue. If Spark 4.1 normalizes collated join keys to binary before the exec is constructed, is Comet's collated-join guard from #4051 still reachable on 4.1 and later, and could Comet legitimately accept those joins natively there? I did not chase it far enough to be sure, but it did not look like something the current tests would tell us.
   
   For this PR I kept the cast tests in their own `spark-4.x` suite. With two `CometCollationSuite` copies in the tree, folding them in would mean writing the same tests twice.
   


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org