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: [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]
