From 953bca1625d5ad5cf538d2540565c0d66e2ffb75 Mon Sep 17 00:00:00 2001 From: stantheman0128 Date: Fri, 7 Aug 2026 23:11:22 +0800 Subject: [PATCH 1/6] test: pin down CometCast fallback for non-default collated strings CometCast.isSupported matches string casts against DataTypes.StringType, the singleton default-collation instance. A non-default-collation StringType (e.g. STRING COLLATE UTF8_LCASE) correctly fails that equality check today and falls back to Spark, but that was implicit and untested: there was no isStringCollationType guard like the other string-touching serdes use, and no test pinning the fallback down. Adds CometCastCollatedStringSuite under spark-4.x (collation is a Spark 4.0+ feature, shared across every 4.x profile, unlike TimeType in #4490 which is 4.1-only) asserting isSupported returns Unsupported for every collated-string pair across LEGACY/TRY/ANSI, plus two Compatible() sanity baselines (same-collation identity cast, and default-collation identity cast) documenting the boundary this issue is not about: an identity cast is a byte-for-byte no-op regardless of collation, so Compatible() there is correct, not a gap. Closes #4489 --- .github/workflows/pr_build_linux.yml | 1 + .github/workflows/pr_build_macos.yml | 1 + .../comet/CometCastCollatedStringSuite.scala | 96 +++++++++++++++++++ 3 files changed, 98 insertions(+) create mode 100644 spark/src/test/spark-4.x/org/apache/comet/CometCastCollatedStringSuite.scala diff --git a/.github/workflows/pr_build_linux.yml b/.github/workflows/pr_build_linux.yml index cbb73fab985..76761d01120 100644 --- a/.github/workflows/pr_build_linux.yml +++ b/.github/workflows/pr_build_linux.yml @@ -584,6 +584,7 @@ jobs: org.apache.comet.CometTemporalExpressionSuite org.apache.comet.CometArrayExpressionSuite org.apache.comet.CometNativeCastSuite + org.apache.comet.CometCastCollatedStringSuite org.apache.comet.CometDateTimeUtilsSuite org.apache.comet.CometMathExpressionSuite org.apache.comet.CometStringExpressionSuite diff --git a/.github/workflows/pr_build_macos.yml b/.github/workflows/pr_build_macos.yml index bcf73b8a56e..948e375fc3b 100644 --- a/.github/workflows/pr_build_macos.yml +++ b/.github/workflows/pr_build_macos.yml @@ -232,6 +232,7 @@ jobs: org.apache.comet.CometTemporalExpressionSuite org.apache.comet.CometArrayExpressionSuite org.apache.comet.CometNativeCastSuite + org.apache.comet.CometCastCollatedStringSuite org.apache.comet.CometDateTimeUtilsSuite org.apache.comet.CometMathExpressionSuite org.apache.comet.CometStringExpressionSuite diff --git a/spark/src/test/spark-4.x/org/apache/comet/CometCastCollatedStringSuite.scala b/spark/src/test/spark-4.x/org/apache/comet/CometCastCollatedStringSuite.scala new file mode 100644 index 00000000000..5cc45acabce --- /dev/null +++ b/spark/src/test/spark-4.x/org/apache/comet/CometCastCollatedStringSuite.scala @@ -0,0 +1,96 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.comet + +import org.apache.spark.sql.CometTestBase +import org.apache.spark.sql.types.{DataType, DataTypes, IntegerType, StringType} + +import org.apache.comet.expressions.{CometCast, CometEvalMode} +import org.apache.comet.serde.{Compatible, Unsupported} + +/** + * https://github.com/apache/datafusion-comet/issues/4489 + * + * `CometCast.isSupported` matches string casts via `case (DataTypes.StringType, _)` and + * `case (_, DataTypes.StringType)`, where `DataTypes.StringType` is the singleton + * default-collation instance. Scala pattern equality means a non-default-collation `StringType` + * (e.g. `STRING COLLATE UTF8_LCASE`) does not match either case and falls through to the + * `unsupported(...)` catch-all (directly, or via `canCastFromString`/`canCastToString`'s own + * catch-all when only one side is collated), so the cast already falls back to Spark correctly. + * That was previously implicit and untested; this suite pins it down directly against + * `isSupported` rather than relying on Scala's pattern-match semantics never changing. + * + * This lives under `spark-4.x` (shared across every 4.x profile) rather than `spark-4.1+`, + * because collation is a Spark 4.0+ feature (`StringType(collationName)` already resolves on + * 4.0), unlike `TimeType` in #4490 which is 4.1-only. + */ +class CometCastCollatedStringSuite extends CometTestBase { + + private val lcase = StringType("UTF8_LCASE") + private val unicode = StringType("UNICODE") + + private def assertUnsupported(fromType: DataType, toType: DataType): Unit = { + Seq(CometEvalMode.LEGACY, CometEvalMode.TRY, CometEvalMode.ANSI).foreach { evalMode => + CometCast.isSupported(fromType, toType, None, evalMode) match { + case _: Unsupported => // expected + case other => + fail(s"expected Unsupported for $fromType -> $toType under $evalMode, got $other") + } + } + } + + test("cast non-default collated string to IntegerType falls back (Unsupported)") { + assertUnsupported(lcase, IntegerType) + } + + test("cast IntegerType to non-default collated string falls back (Unsupported)") { + assertUnsupported(IntegerType, lcase) + } + + test("cast non-default collated string to default-collation StringType falls back") { + assertUnsupported(lcase, DataTypes.StringType) + } + + test("cast default-collation StringType to non-default collated string falls back") { + assertUnsupported(DataTypes.StringType, lcase) + } + + test("cast between two different non-default collations falls back") { + assertUnsupported(lcase, unicode) + } + + // Boundary case, not a bug: an identity cast between two instances of the SAME collation is a + // byte-for-byte no-op that never touches collation-aware comparison/hashing/sorting semantics, + // so it is correctly Compatible() regardless of which collation it is. This documents that + // boundary so a future reader does not mistake it for the gap this issue describes. + test("cast identical non-default collation to itself is a Compatible no-op") { + Seq(CometEvalMode.LEGACY, CometEvalMode.TRY, CometEvalMode.ANSI).foreach { evalMode => + assert(CometCast.isSupported(lcase, lcase, None, evalMode) == Compatible()) + } + } + + test("cast default-collation StringType to itself is a Compatible no-op (sanity baseline)") { + Seq(CometEvalMode.LEGACY, CometEvalMode.TRY, CometEvalMode.ANSI).foreach { evalMode => + assert( + CometCast.isSupported(DataTypes.StringType, DataTypes.StringType, None, evalMode) == + Compatible()) + } + } +} From 281b5727a39bc680a63ae139b768d92cf9ab9f50 Mon Sep 17 00:00:00 2001 From: stantheman0128 Date: Fri, 7 Aug 2026 23:13:52 +0800 Subject: [PATCH 2/6] style: apply spotless formatting to CometCastCollatedStringSuite Rewraps the Scaladoc comment block to match what 'mvn spotless:apply' (scalafmt) produces. Verified via a real mvn test -Pspark-4.1 run in WSL (spotless:check now passes; 7/7 tests still pass). --- .../comet/CometCastCollatedStringSuite.scala | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/spark/src/test/spark-4.x/org/apache/comet/CometCastCollatedStringSuite.scala b/spark/src/test/spark-4.x/org/apache/comet/CometCastCollatedStringSuite.scala index 5cc45acabce..684c1943ce5 100644 --- a/spark/src/test/spark-4.x/org/apache/comet/CometCastCollatedStringSuite.scala +++ b/spark/src/test/spark-4.x/org/apache/comet/CometCastCollatedStringSuite.scala @@ -28,14 +28,14 @@ import org.apache.comet.serde.{Compatible, Unsupported} /** * https://github.com/apache/datafusion-comet/issues/4489 * - * `CometCast.isSupported` matches string casts via `case (DataTypes.StringType, _)` and - * `case (_, DataTypes.StringType)`, where `DataTypes.StringType` is the singleton - * default-collation instance. Scala pattern equality means a non-default-collation `StringType` - * (e.g. `STRING COLLATE UTF8_LCASE`) does not match either case and falls through to the - * `unsupported(...)` catch-all (directly, or via `canCastFromString`/`canCastToString`'s own - * catch-all when only one side is collated), so the cast already falls back to Spark correctly. - * That was previously implicit and untested; this suite pins it down directly against - * `isSupported` rather than relying on Scala's pattern-match semantics never changing. + * `CometCast.isSupported` matches string casts via `case (DataTypes.StringType, _)` and `case (_, + * DataTypes.StringType)`, where `DataTypes.StringType` is the singleton default-collation + * instance. Scala pattern equality means a non-default-collation `StringType` (e.g. `STRING + * COLLATE UTF8_LCASE`) does not match either case and falls through to the `unsupported(...)` + * catch-all (directly, or via `canCastFromString`/`canCastToString`'s own catch-all when only one + * side is collated), so the cast already falls back to Spark correctly. That was previously + * implicit and untested; this suite pins it down directly against `isSupported` rather than + * relying on Scala's pattern-match semantics never changing. * * This lives under `spark-4.x` (shared across every 4.x profile) rather than `spark-4.1+`, * because collation is a Spark 4.0+ feature (`StringType(collationName)` already resolves on From 1edbe68ec0c618b92df31b6d8e81112dfbe089ae Mon Sep 17 00:00:00 2001 From: stantheman0128 Date: Sat, 8 Aug 2026 03:40:02 +0800 Subject: [PATCH 3/6] fix: reject casts involving non-default collated strings CometCast.isSupported only failed to match a collated StringType because DataTypes.StringType is the default-collation singleton and Scala pattern equality compares the whole instance. The fromType == toType shortcut let identity casts through regardless, including nested ones such as ARRAY, and serializeDataType maps every StringType to one proto type id, so the collation was dropped from the plan with no warning. Reject collated source and target types up front using the existing CometTypeShim.hasNonDefaultStringCollation, the same helper the array, collection, datetime, map, predicate, and string serdes already use. It walks nested element, key, value, and field types, and is stubbed to false on Spark 3.x where collation does not exist. Rework CometCastCollatedStringSuite accordingly. Unsupported means there is no native path rather than a fallback to Spark, since CometCast mixes in CodegenDispatchFallback, so the test names and the Scaladoc now say that. Adds nested coverage for arrays, structs, and maps, over-block checks for default-collation casts, and end-to-end coverage of both settings of spark.comet.exec.scalaUDF.codegen.enabled. Closes #4489 --- .../apache/comet/expressions/CometCast.scala | 13 ++ .../comet/CometCastCollatedStringSuite.scala | 177 ++++++++++++++---- 2 files changed, 154 insertions(+), 36 deletions(-) diff --git a/spark/src/main/scala/org/apache/comet/expressions/CometCast.scala b/spark/src/main/scala/org/apache/comet/expressions/CometCast.scala index d29ef7cd3b7..95418fcb1d7 100644 --- a/spark/src/main/scala/org/apache/comet/expressions/CometCast.scala +++ b/spark/src/main/scala/org/apache/comet/expressions/CometCast.scala @@ -186,6 +186,19 @@ object CometCast return unsupported(fromType, toType) } + // Spark 4.0's collation metadata rides on `StringType`, but `serializeDataType` maps every + // `StringType` to the same proto id, so a non-default collation is dropped on the way into + // the native plan with no warning. Reject the cast outright rather than relying on the + // pattern matching below, which only misses collated types because `DataTypes.StringType` is + // the default-collation singleton and Scala pattern equality happens not to match. This runs + // above the `fromType == toType` shortcut so that an identity cast on a collated type is + // checked too, and `hasNonDefaultStringCollation` walks nested element, key, value, and field + // types. The version-shimmed helper returns false on Spark 3.x, where collation does not + // exist. See https://github.com/apache/datafusion-comet/issues/4489. + if (hasNonDefaultStringCollation(fromType) || hasNonDefaultStringCollation(toType)) { + return unsupported(fromType, toType) + } + if (fromType == toType) { return Compatible() } diff --git a/spark/src/test/spark-4.x/org/apache/comet/CometCastCollatedStringSuite.scala b/spark/src/test/spark-4.x/org/apache/comet/CometCastCollatedStringSuite.scala index 684c1943ce5..aa81710938f 100644 --- a/spark/src/test/spark-4.x/org/apache/comet/CometCastCollatedStringSuite.scala +++ b/spark/src/test/spark-4.x/org/apache/comet/CometCastCollatedStringSuite.scala @@ -20,7 +20,7 @@ package org.apache.comet import org.apache.spark.sql.CometTestBase -import org.apache.spark.sql.types.{DataType, DataTypes, IntegerType, StringType} +import org.apache.spark.sql.types.{ArrayType, DataType, DataTypes, IntegerType, MapType, StringType, StructField, StructType} import org.apache.comet.expressions.{CometCast, CometEvalMode} import org.apache.comet.serde.{Compatible, Unsupported} @@ -28,26 +28,43 @@ import org.apache.comet.serde.{Compatible, Unsupported} /** * https://github.com/apache/datafusion-comet/issues/4489 * - * `CometCast.isSupported` matches string casts via `case (DataTypes.StringType, _)` and `case (_, - * DataTypes.StringType)`, where `DataTypes.StringType` is the singleton default-collation - * instance. Scala pattern equality means a non-default-collation `StringType` (e.g. `STRING - * COLLATE UTF8_LCASE`) does not match either case and falls through to the `unsupported(...)` - * catch-all (directly, or via `canCastFromString`/`canCastToString`'s own catch-all when only one - * side is collated), so the cast already falls back to Spark correctly. That was previously - * implicit and untested; this suite pins it down directly against `isSupported` rather than - * relying on Scala's pattern-match semantics never changing. + * Spark 4.0 carries collation metadata on `StringType`, but `serializeDataType` maps every + * `StringType` to one proto type id, so the collation is dropped on the way into the native plan + * with no warning. Nothing used to stop that from happening. `CometCast.isSupported` matched + * string casts through `case (DataTypes.StringType, _)` and `case (_, DataTypes.StringType)`, and + * those only failed to match a collated `StringType` because `DataTypes.StringType` is the + * default-collation singleton and Scala pattern equality compares the whole instance. The right + * answer fell out of an accident of pattern matching. The `fromType == toType` shortcut let + * identity casts on collated types through regardless, including nested ones such as + * `ARRAY`, which were byte-safe only because the cast was a no-op. * - * This lives under `spark-4.x` (shared across every 4.x profile) rather than `spark-4.1+`, - * because collation is a Spark 4.0+ feature (`StringType(collationName)` already resolves on - * 4.0), unlike `TimeType` in #4490 which is 4.1-only. + * `isSupported` now rejects any source or target type carrying a non-default collation before + * either of those paths runs. This suite pins the resulting matrix down so the behaviour no + * longer depends on pattern-match semantics staying the way they are. + * + * A note on what `Unsupported` means here. It does not mean the query falls back to Spark. + * `CometCast` mixes in `CodegenDispatchFallback`, so `QueryPlanSerde.exprToProtoInternal` offers + * the expression to the JVM codegen dispatcher first, which runs Spark's own `doGenCode` inside + * the Comet pipeline. `spark.comet.exec.scalaUDF.codegen.enabled` defaults to true and + * `CometBatchKernelCodegen` admits `ResolvedCollation`, so under default config a collated cast + * usually stays inside Comet. `Unsupported` means there is no native path. The end-to-end tests + * at the bottom cover both settings of that config. + * + * This lives under `spark-4.x`, shared by every 4.x profile, rather than `spark-4.1+`, because + * collation is a Spark 4.0 feature and `StringType(collationName)` already resolves there. */ class CometCastCollatedStringSuite extends CometTestBase { private val lcase = StringType("UTF8_LCASE") private val unicode = StringType("UNICODE") - private def assertUnsupported(fromType: DataType, toType: DataType): Unit = { - Seq(CometEvalMode.LEGACY, CometEvalMode.TRY, CometEvalMode.ANSI).foreach { evalMode => + private val evalModes = Seq(CometEvalMode.LEGACY, CometEvalMode.TRY, CometEvalMode.ANSI) + + private def structWith(dt: DataType): StructType = StructType(Seq(StructField("s", dt))) + + /** Asserts that Comet reports no native path for this cast under every eval mode. */ + private def assertNoNativePath(fromType: DataType, toType: DataType): Unit = { + evalModes.foreach { evalMode => CometCast.isSupported(fromType, toType, None, evalMode) match { case _: Unsupported => // expected case other => @@ -56,41 +73,129 @@ class CometCastCollatedStringSuite extends CometTestBase { } } - test("cast non-default collated string to IntegerType falls back (Unsupported)") { - assertUnsupported(lcase, IntegerType) + /** Asserts that the collation guard leaves an uncollated cast alone. */ + private def assertCompatible(fromType: DataType, toType: DataType): Unit = { + evalModes.foreach { evalMode => + CometCast.isSupported(fromType, toType, None, evalMode) match { + case _: Compatible => // expected + case other => + fail(s"expected Compatible for $fromType -> $toType under $evalMode, got $other") + } + } + } + + // ---- scalar collated strings ---------------------------------------------------- + + test("cast collated string to IntegerType has no native path") { + assertNoNativePath(lcase, IntegerType) + } + + test("cast IntegerType to collated string has no native path") { + // This pair leaves through `canCastFromInt`'s catch-all rather than `canCastToString`, + // because `case (_, DataTypes.StringType)` does not match a collated target. The guard now + // answers ahead of both. + assertNoNativePath(IntegerType, lcase) + } + + test("cast collated string to default-collation StringType has no native path") { + assertNoNativePath(lcase, DataTypes.StringType) + } + + test("cast default-collation StringType to collated string has no native path") { + assertNoNativePath(DataTypes.StringType, lcase) + } + + test("cast between two different collations has no native path") { + assertNoNativePath(lcase, unicode) + } + + test("cast collated string to the same collation has no native path") { + // The `fromType == toType` shortcut answered `Compatible()` here before the guard existed. + // The cast is a byte-level no-op so results were right, but the plan reached the native side + // with the collation stripped and nothing recording that. This is the implicit behaviour + // #4489 names, so the guard sits above the shortcut. + assertNoNativePath(lcase, lcase) + } + + // ---- nested collated strings ---------------------------------------------------- + + test("cast array of collated strings to another collation has no native path") { + assertNoNativePath(ArrayType(lcase), ArrayType(unicode)) + } + + test("cast array of collated strings to the same collation has no native path") { + // Same identity shortcut as the scalar case, one level down. + assertNoNativePath(ArrayType(lcase), ArrayType(lcase)) + } + + test("cast array of collated strings to StringType has no native path") { + // `case (dt: ArrayType, DataTypes.StringType)` recurses on the element type, so the reason + // used to describe the element rather than the array. The guard answers for the whole type. + assertNoNativePath(ArrayType(lcase), DataTypes.StringType) } - test("cast IntegerType to non-default collated string falls back (Unsupported)") { - assertUnsupported(IntegerType, lcase) + test("cast struct with a collated field has no native path") { + assertNoNativePath(structWith(lcase), structWith(unicode)) + assertNoNativePath(structWith(lcase), structWith(lcase)) } - test("cast non-default collated string to default-collation StringType falls back") { - assertUnsupported(lcase, DataTypes.StringType) + test("cast map with a collated key has no native path") { + assertNoNativePath(MapType(lcase, IntegerType), MapType(unicode, IntegerType)) + assertNoNativePath(MapType(lcase, IntegerType), MapType(lcase, IntegerType)) } - test("cast default-collation StringType to non-default collated string falls back") { - assertUnsupported(DataTypes.StringType, lcase) + test("cast map with a collated value has no native path") { + assertNoNativePath(MapType(IntegerType, lcase), MapType(IntegerType, unicode)) + assertNoNativePath(MapType(IntegerType, lcase), MapType(IntegerType, lcase)) } - test("cast between two different non-default collations falls back") { - assertUnsupported(lcase, unicode) + // ---- the guard must not over-block ---------------------------------------------- + + test("default-collation string casts are untouched by the collation guard") { + assertCompatible(DataTypes.StringType, DataTypes.StringType) + assertCompatible(DataTypes.StringType, IntegerType) + assertCompatible(IntegerType, DataTypes.StringType) } - // Boundary case, not a bug: an identity cast between two instances of the SAME collation is a - // byte-for-byte no-op that never touches collation-aware comparison/hashing/sorting semantics, - // so it is correctly Compatible() regardless of which collation it is. This documents that - // boundary so a future reader does not mistake it for the gap this issue describes. - test("cast identical non-default collation to itself is a Compatible no-op") { - Seq(CometEvalMode.LEGACY, CometEvalMode.TRY, CometEvalMode.ANSI).foreach { evalMode => - assert(CometCast.isSupported(lcase, lcase, None, evalMode) == Compatible()) + test("nested default-collation string casts are untouched by the collation guard") { + assertCompatible(ArrayType(DataTypes.StringType), ArrayType(DataTypes.StringType)) + assertCompatible(structWith(DataTypes.StringType), structWith(DataTypes.StringType)) + assertCompatible( + MapType(DataTypes.StringType, IntegerType), + MapType(DataTypes.StringType, IntegerType)) + } + + // ---- end to end ----------------------------------------------------------------- + // + // The matrix above only exercises `isSupported`. These two run a query and pin down what the + // planner actually does with the answer, which is what #4489 asked for. A plain-string Parquet + // column with `COLLATE` applied on top reaches the cast as a collated child, the same shape + // the datetime tests in `CometCollationSuite` rely on. The cast child has to be a column + // rather than a literal, since `getSupportLevel` folds literal children before `isSupported` + // is consulted. + + private def withCollatedTable(f: => Unit): Unit = + withParquetTable(Seq(("123", 1), ("456", 2)), "collated_cast_tbl")(f) + + private val castFromCollatedReason = + "Cast from StringType(UTF8_LCASE) to IntegerType is not supported" + + test("cast from a collated string falls back to Spark when codegen dispatch is off") { + withCollatedTable { + withSQLConf(CometConf.COMET_SCALA_UDF_CODEGEN_ENABLED.key -> "false") { + checkSparkAnswerAndFallbackReason( + "SELECT CAST(_1 COLLATE utf8_lcase AS INT) FROM collated_cast_tbl", + castFromCollatedReason) + } } } - test("cast default-collation StringType to itself is a Compatible no-op (sanity baseline)") { - Seq(CometEvalMode.LEGACY, CometEvalMode.TRY, CometEvalMode.ANSI).foreach { evalMode => - assert( - CometCast.isSupported(DataTypes.StringType, DataTypes.StringType, None, evalMode) == - Compatible()) + test("cast from a collated string routes through the codegen dispatcher when it is on") { + withCollatedTable { + withSQLConf(CometConf.COMET_SCALA_UDF_CODEGEN_ENABLED.key -> "true") { + checkSparkAnswerAndOperator( + "SELECT CAST(_1 COLLATE utf8_lcase AS INT) FROM collated_cast_tbl") + } } } } From 1b1bc97f43a1f4c42188c3fcbfc8d1022282a8c7 Mon Sep 17 00:00:00 2001 From: stantheman0128 Date: Sat, 8 Aug 2026 05:49:26 +0800 Subject: [PATCH 4/6] test: cover the cast pairs the collation guard newly blocks Working out which pairs actually changed answer turned up three that were Compatible on main and are not identity casts, so the guard's over-block surface was wider than the first revision of this suite described. A struct whose collated field is unchanged while a sibling field is cast answered Compatible, because the field zip answered per field and the collated field matched the fromType == toType shortcut. A map with an unchanged collated key and a cast value did the same through the key. ArrayType(NullType) -> ArrayType(lcase) answered Compatible through the elementType == NullType branch, which runs ahead of everything else. The struct case also gives the suite its first end-to-end test that fails without the guard. The sibling field changes type, so the cast survives SimplifyCasts and the collated field rides along inside it. A scalar identity cast cannot be reached from SQL at all, since SimplifyCasts drops a cast whose child already has the target type and the query arrives at the planner as a bare Collate. There is a comment in the suite recording that, in the style of the unreachable join tests in CometCollationSuite. --- .../comet/CometCastCollatedStringSuite.scala | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/spark/src/test/spark-4.x/org/apache/comet/CometCastCollatedStringSuite.scala b/spark/src/test/spark-4.x/org/apache/comet/CometCastCollatedStringSuite.scala index aa81710938f..91ee15365c3 100644 --- a/spark/src/test/spark-4.x/org/apache/comet/CometCastCollatedStringSuite.scala +++ b/spark/src/test/spark-4.x/org/apache/comet/CometCastCollatedStringSuite.scala @@ -149,6 +149,26 @@ class CometCastCollatedStringSuite extends CometTestBase { assertNoNativePath(MapType(IntegerType, lcase), MapType(IntegerType, lcase)) } + test("cast struct whose collated field is unchanged while a sibling field is cast") { + // The field zip used to answer per field, so the collated field hit the identity shortcut + // and reported Compatible while the sibling carried the cast. That let a collated field ride + // into the native plan on another field's back. The guard answers for the whole struct. + val from = StructType(Seq(StructField("a", IntegerType), StructField("s", lcase))) + val to = StructType(Seq(StructField("a", DataTypes.StringType), StructField("s", lcase))) + assertNoNativePath(from, to) + } + + test("cast map whose collated key is unchanged while the value type is cast") { + assertNoNativePath(MapType(lcase, IntegerType), MapType(lcase, DataTypes.LongType)) + } + + test("cast array of nulls to array of collated strings has no native path") { + // `case (dt: ArrayType, _: ArrayType) if dt.elementType == NullType` returns Compatible ahead + // of every other branch, so this was one more way to reach the native side with a collation + // attached to the target. + assertNoNativePath(ArrayType(DataTypes.NullType), ArrayType(lcase)) + } + // ---- the guard must not over-block ---------------------------------------------- test("default-collation string casts are untouched by the collation guard") { @@ -180,6 +200,15 @@ class CometCastCollatedStringSuite extends CometTestBase { private val castFromCollatedReason = "Cast from StringType(UTF8_LCASE) to IntegerType is not supported" + // A scalar identity pair such as `lcase -> lcase` cannot be reached from SQL. Spark's + // `SimplifyCasts` drops a cast whose child already carries the target type, so + // `CAST(_1 COLLATE utf8_lcase AS STRING COLLATE UTF8_LCASE)` arrives at the planner as a bare + // `Collate` and the only fallback reason on the plan is "collate is not supported", which comes + // from a different serde. Those pairs stay pinned at the `isSupported` level above, in the same + // spirit as the join tests in `CometCollationSuite` that no query can reach. A struct is the + // way in: when a sibling field changes type the cast survives, and the collated field rides + // along inside it. That case is covered end to end below. + test("cast from a collated string falls back to Spark when codegen dispatch is off") { withCollatedTable { withSQLConf(CometConf.COMET_SCALA_UDF_CODEGEN_ENABLED.key -> "false") { @@ -198,4 +227,22 @@ class CometCastCollatedStringSuite extends CometTestBase { } } } + + test("cast of a struct carrying a collated field has no native path end to end") { + // The two tests above hold on either side of the guard, because that pair already reached the + // `case _` catch-all. This one is the guard's own case. The sibling field changes type so the + // cast survives `SimplifyCasts`, and on the old code the field zip answered `Compatible`, + // since the collated field matched the identity shortcut, so the struct went native with the + // collation dropped. Only the target half of the reason is asserted, because the source field + // names come from `struct(...)` and are not worth pinning down across Spark versions. + withCollatedTable { + withSQLConf(CometConf.COMET_SCALA_UDF_CODEGEN_ENABLED.key -> "false") { + checkSparkAnswerAndFallbackReason( + "SELECT CAST(struct(_2 AS a, _1 COLLATE utf8_lcase AS s) AS " + + "STRUCT) FROM collated_cast_tbl", + "to StructType(StructField(a,StringType,true)," + + "StructField(s,StringType(UTF8_LCASE),true)) is not supported") + } + } + } } From aeda8d3643964c1d2e2b9f9834efa5ea52d33331 Mon Sep 17 00:00:00 2001 From: Po-Han Shih Date: Sat, 12 Sep 2026 01:50:18 +0800 Subject: [PATCH 5/6] fix: give the collation cast rejection its own fallback reason The guard returned the generic `Cast from $fromType to $toType is not supported` template. For an identity cast both sides print the same, so EXPLAIN read like a nonsensical refusal of a string-to-string cast with no hint that collation was the cause. Name the reason on `CometCast` and have the suite read it from production so the two cannot drift. Also document why `CometBatchKernelCodegen.isSupportedDataType` admits a collated `StringType`. The dispatcher's declared return type does go through `serializeDataType`, which flattens collation, but every collation-sensitive decision Comet makes reads the Catalyst `DataType` and blocks the operator before a native kernel sees the column. --- .../codegen/CometBatchKernelCodegen.scala | 15 ++++++ .../apache/comet/expressions/CometCast.scala | 17 +++++- .../comet/CometCastCollatedStringSuite.scala | 52 +++++++++++++------ 3 files changed, 68 insertions(+), 16 deletions(-) diff --git a/spark/src/main/scala/org/apache/comet/codegen/CometBatchKernelCodegen.scala b/spark/src/main/scala/org/apache/comet/codegen/CometBatchKernelCodegen.scala index 83fbca6b635..eadda24da70 100644 --- a/spark/src/main/scala/org/apache/comet/codegen/CometBatchKernelCodegen.scala +++ b/spark/src/main/scala/org/apache/comet/codegen/CometBatchKernelCodegen.scala @@ -87,6 +87,21 @@ object CometBatchKernelCodegen extends Logging with CometExprTraitShim with Come * single child and the generated writer NPEs on the missing ordinal-1 vector. * `CometCreateNamedStruct` declines them on the native path for the same reason, but a struct * nested inside a dispatcher-built value (a `CreateMap` value) never reaches that check. + * + * The `StringType` case admits non-default collations (Spark 4+), on purpose. Collation is + * carried by the expression, not by the value: the kernel runs Spark's own `doGenCode` against + * the bound tree, whose `collationId` survives closure serialization, and Arrow is only the + * byte store for the `UTF8String`s that code produces. So a dispatched expression over collated + * input answers exactly as Spark does. + * + * The proto type is a separate matter. `CometScalaUDF.emitJvmCodegenDispatch` declares the + * return type through `QueryPlanSerde.serializeDataType`, which flattens every `StringType` to + * one proto id, so the native plan describes a dispatched collated output as a plain string. + * Nothing on this route reads that back. The values are bytes and the kernel is what produced + * them. Whether a downstream operator may then treat the column collation-blind is decided per + * operator against the Catalyst `DataType`, which keeps its collation, and does not depend on + * what this predicate admits. Rejecting collated strings here would force a full Spark fallback + * for a route that is already correct. */ def isSupportedDataType(dt: DataType): Boolean = dt match { case BooleanType | ByteType | ShortType | IntegerType | LongType => true diff --git a/spark/src/main/scala/org/apache/comet/expressions/CometCast.scala b/spark/src/main/scala/org/apache/comet/expressions/CometCast.scala index 95418fcb1d7..b1799ed1f2c 100644 --- a/spark/src/main/scala/org/apache/comet/expressions/CometCast.scala +++ b/spark/src/main/scala/org/apache/comet/expressions/CometCast.scala @@ -50,6 +50,16 @@ object CometCast private[comet] val legacyCastComplexTypesToStringReason: String = "spark.sql.legacy.castComplexTypesToString.enabled=true is not supported" + // The generic `Cast from $fromType to $toType is not supported` template is unhelpful for a + // collation rejection. An identity cast prints both sides identically, so it reads like a + // nonsensical refusal of a string-to-string cast unless the reader already knows collation is + // the cause. Named here, and shared with `CometCastCollatedStringSuite`, so the asserted reason + // cannot drift from production. Follows the phrasing the other collation reasons use, e.g. + // `CometReverse` in `collectionOperations.scala` and `ComparisonUtils` in `predicates.scala`. + private[comet] val nonDefaultCollationReason: String = + "Cast involving a non-default string collation is not supported " + + "(https://github.com/apache/datafusion-comet/issues/4489)" + private def legacyCastComplexTypesToString: Boolean = SQLConf.get .getConfString("spark.sql.legacy.castComplexTypesToString.enabled", "false") @@ -195,8 +205,13 @@ object CometCast // checked too, and `hasNonDefaultStringCollation` walks nested element, key, value, and field // types. The version-shimmed helper returns false on Spark 3.x, where collation does not // exist. See https://github.com/apache/datafusion-comet/issues/4489. + // + // `Unsupported` here means there is no native path, not that the plan falls back to Spark. + // `CodegenDispatchFallback` offers the cast to the JVM codegen dispatcher first, and that + // route is result-correct: see the note on `isSupportedDataType` in + // `CometBatchKernelCodegen` for why a collated string is safe to admit there. if (hasNonDefaultStringCollation(fromType) || hasNonDefaultStringCollation(toType)) { - return unsupported(fromType, toType) + return Unsupported(Some(nonDefaultCollationReason)) } if (fromType == toType) { diff --git a/spark/src/test/spark-4.x/org/apache/comet/CometCastCollatedStringSuite.scala b/spark/src/test/spark-4.x/org/apache/comet/CometCastCollatedStringSuite.scala index 91ee15365c3..722dbf42baa 100644 --- a/spark/src/test/spark-4.x/org/apache/comet/CometCastCollatedStringSuite.scala +++ b/spark/src/test/spark-4.x/org/apache/comet/CometCastCollatedStringSuite.scala @@ -46,9 +46,18 @@ import org.apache.comet.serde.{Compatible, Unsupported} * `CometCast` mixes in `CodegenDispatchFallback`, so `QueryPlanSerde.exprToProtoInternal` offers * the expression to the JVM codegen dispatcher first, which runs Spark's own `doGenCode` inside * the Comet pipeline. `spark.comet.exec.scalaUDF.codegen.enabled` defaults to true and - * `CometBatchKernelCodegen` admits `ResolvedCollation`, so under default config a collated cast - * usually stays inside Comet. `Unsupported` means there is no native path. The end-to-end tests - * at the bottom cover both settings of that config. + * `CometBatchKernelCodegen.isSupportedDataType` admits every `StringType` regardless of + * collation, so under default config a collated cast usually stays inside Comet. That route is + * result-correct, because the kernel evaluates Spark's own generated code and the `collationId` + * rides along on the expression. `Unsupported` means there is no native path. The end-to-end + * tests at the bottom run both settings of that config on both the scalar and the struct case. + * + * These are Scala tests rather than `CometSqlFileTestSuite` fixtures because most of the file + * asserts on `CometCast.isSupported` over type pairs no SQL can construct, such as + * `ArrayType(NullType) -> ArrayType(STRING COLLATE UTF8_LCASE)`. The end-to-end tests stay here + * with them because `--Config` is file scoped, so the query that has to assert a fallback reason + * with the dispatcher off and the query that has to assert native execution with it on could not + * share a fixture file. * * This lives under `spark-4.x`, shared by every 4.x profile, rather than `spark-4.1+`, because * collation is a Spark 4.0 feature and `StringType(collationName)` already resolves there. @@ -187,7 +196,7 @@ class CometCastCollatedStringSuite extends CometTestBase { // ---- end to end ----------------------------------------------------------------- // - // The matrix above only exercises `isSupported`. These two run a query and pin down what the + // The matrix above only exercises `isSupported`. These three run a query and pin down what the // planner actually does with the answer, which is what #4489 asked for. A plain-string Parquet // column with `COLLATE` applied on top reaches the cast as a collated child, the same shape // the datetime tests in `CometCollationSuite` rely on. The cast child has to be a column @@ -197,8 +206,11 @@ class CometCastCollatedStringSuite extends CometTestBase { private def withCollatedTable(f: => Unit): Unit = withParquetTable(Seq(("123", 1), ("456", 2)), "collated_cast_tbl")(f) - private val castFromCollatedReason = - "Cast from StringType(UTF8_LCASE) to IntegerType is not supported" + // Read from production rather than retyped, so the two cannot drift apart. The guard is the + // only thing that produces this reason: without it `StringType(UTF8_LCASE) -> IntegerType` + // exits through `isSupported`'s `case _` catch-all and reports the generic + // `Cast from ... to ... is not supported` template instead. + private val collationReason = CometCast.nonDefaultCollationReason // A scalar identity pair such as `lcase -> lcase` cannot be reached from SQL. Spark's // `SimplifyCasts` drops a cast whose child already carries the target type, so @@ -214,7 +226,7 @@ class CometCastCollatedStringSuite extends CometTestBase { withSQLConf(CometConf.COMET_SCALA_UDF_CODEGEN_ENABLED.key -> "false") { checkSparkAnswerAndFallbackReason( "SELECT CAST(_1 COLLATE utf8_lcase AS INT) FROM collated_cast_tbl", - castFromCollatedReason) + collationReason) } } } @@ -229,19 +241,29 @@ class CometCastCollatedStringSuite extends CometTestBase { } test("cast of a struct carrying a collated field has no native path end to end") { - // The two tests above hold on either side of the guard, because that pair already reached the - // `case _` catch-all. This one is the guard's own case. The sibling field changes type so the - // cast survives `SimplifyCasts`, and on the old code the field zip answered `Compatible`, - // since the collated field matched the identity shortcut, so the struct went native with the - // collation dropped. Only the target half of the reason is asserted, because the source field - // names come from `struct(...)` and are not worth pinning down across Spark versions. + // A pair the guard changed the answer for, not just the reason string. The sibling field + // changes type so the cast survives `SimplifyCasts`, and on the old code the field zip + // answered `Compatible`, since the collated field matched the identity shortcut, so the + // struct went native with the collation dropped. withCollatedTable { withSQLConf(CometConf.COMET_SCALA_UDF_CODEGEN_ENABLED.key -> "false") { checkSparkAnswerAndFallbackReason( "SELECT CAST(struct(_2 AS a, _1 COLLATE utf8_lcase AS s) AS " + "STRUCT) FROM collated_cast_tbl", - "to StructType(StructField(a,StringType,true)," + - "StructField(s,StringType(UTF8_LCASE),true)) is not supported") + collationReason) + } + } + } + + test("cast of a struct carrying a collated field routes through the codegen dispatcher") { + // The struct is the one case the guard redirects rather than merely relabels, so it needs the + // dispatcher-on half too. `isSupportedDataType` recurses into the fields and accepts them, so + // the cast stays in the Comet pipeline and the kernel runs Spark's own `Cast.doGenCode`. + withCollatedTable { + withSQLConf(CometConf.COMET_SCALA_UDF_CODEGEN_ENABLED.key -> "true") { + checkSparkAnswerAndOperator( + "SELECT CAST(struct(_2 AS a, _1 COLLATE utf8_lcase AS s) AS " + + "STRUCT) FROM collated_cast_tbl") } } } From 9dd2e7f67e7ae2f1218f8b2c7103624376447745 Mon Sep 17 00:00:00 2001 From: Po-Han Shih Date: Sat, 26 Sep 2026 05:28:45 +0800 Subject: [PATCH 6/6] test: describe the struct end-to-end case as a fallback moved into the dispatcher On main the struct cast query already falls back to Spark, because Collate has no serde. The guard moves it into the codegen dispatcher when that is on, and puts the collation reason on the plan when it is off. Also fix the end-to-end test count, and reword three matrix comments (identity cast, unchanged field beside a cast sibling, array of nulls) that implied a collated type reached the native side. They now describe what isSupported used to answer. The array-of-nulls comment also no longer says its branch runs ahead of every other one, since (NullType, _) and the identity shortcut come first. Co-Authored-By: Claude Opus 5.5 --- .../comet/CometCastCollatedStringSuite.scala | 29 ++++++++++--------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/spark/src/test/spark-4.x/org/apache/comet/CometCastCollatedStringSuite.scala b/spark/src/test/spark-4.x/org/apache/comet/CometCastCollatedStringSuite.scala index 722dbf42baa..f547836a7b7 100644 --- a/spark/src/test/spark-4.x/org/apache/comet/CometCastCollatedStringSuite.scala +++ b/spark/src/test/spark-4.x/org/apache/comet/CometCastCollatedStringSuite.scala @@ -120,9 +120,9 @@ class CometCastCollatedStringSuite extends CometTestBase { test("cast collated string to the same collation has no native path") { // The `fromType == toType` shortcut answered `Compatible()` here before the guard existed. - // The cast is a byte-level no-op so results were right, but the plan reached the native side - // with the collation stripped and nothing recording that. This is the implicit behaviour - // #4489 names, so the guard sits above the shortcut. + // The result would be right, since the cast is a byte-level no-op, but `isSupported` was + // clearing a collated type for a proto that cannot carry the collation, and nothing recorded + // that. This is the implicit behaviour #4489 names, so the guard sits above the shortcut. assertNoNativePath(lcase, lcase) } @@ -160,8 +160,8 @@ class CometCastCollatedStringSuite extends CometTestBase { test("cast struct whose collated field is unchanged while a sibling field is cast") { // The field zip used to answer per field, so the collated field hit the identity shortcut - // and reported Compatible while the sibling carried the cast. That let a collated field ride - // into the native plan on another field's back. The guard answers for the whole struct. + // and reported Compatible while the sibling carried the cast. That cleared a collated field + // for the native plan on another field's back. The guard answers for the whole struct. val from = StructType(Seq(StructField("a", IntegerType), StructField("s", lcase))) val to = StructType(Seq(StructField("a", DataTypes.StringType), StructField("s", lcase))) assertNoNativePath(from, to) @@ -172,9 +172,9 @@ class CometCastCollatedStringSuite extends CometTestBase { } test("cast array of nulls to array of collated strings has no native path") { - // `case (dt: ArrayType, _: ArrayType) if dt.elementType == NullType` returns Compatible ahead - // of every other branch, so this was one more way to reach the native side with a collation - // attached to the target. + // `case (dt: ArrayType, _: ArrayType) if dt.elementType == NullType` returns Compatible + // without looking at the target element type, so this was one more pair where + // `isSupported` cleared a collated target type. assertNoNativePath(ArrayType(DataTypes.NullType), ArrayType(lcase)) } @@ -196,7 +196,7 @@ class CometCastCollatedStringSuite extends CometTestBase { // ---- end to end ----------------------------------------------------------------- // - // The matrix above only exercises `isSupported`. These three run a query and pin down what the + // The matrix above only exercises `isSupported`. These four run a query and pin down what the // planner actually does with the answer, which is what #4489 asked for. A plain-string Parquet // column with `COLLATE` applied on top reaches the cast as a collated child, the same shape // the datetime tests in `CometCollationSuite` rely on. The cast child has to be a column @@ -241,10 +241,13 @@ class CometCastCollatedStringSuite extends CometTestBase { } test("cast of a struct carrying a collated field has no native path end to end") { - // A pair the guard changed the answer for, not just the reason string. The sibling field - // changes type so the cast survives `SimplifyCasts`, and on the old code the field zip - // answered `Compatible`, since the collated field matched the identity shortcut, so the - // struct went native with the collation dropped. + // A pair the guard changes the outcome for, not just the reason string. The sibling field + // changes type so the cast survives `SimplifyCasts`. Without the guard the field zip + // answered `Compatible`, since the collated field matched the identity shortcut, but the + // query still fell back to Spark: `Collate` has no serde, so serializing the struct stopped + // at the collated field with "collate is not supported". With the guard the cast itself + // is `Unsupported`: with the dispatcher off the plan carries the collation reason, and with + // it on (next test) the query moves out of that Spark fallback and into the dispatcher. withCollatedTable { withSQLConf(CometConf.COMET_SCALA_UDF_CODEGEN_ENABLED.key -> "false") { checkSparkAnswerAndFallbackReason(