From f2885740cd309878667a96e6165c9fc15aa0a755 Mon Sep 17 00:00:00 2001 From: Andy Grove Date: Wed, 5 Aug 2026 07:33:23 -0600 Subject: [PATCH] test: rename CometCastSuite to CometNativeCastSuite `CometCast` mixes in `CodegenDispatchFallback`, so a cast that `CometCast.isSupported` rejects still runs inside the Comet operator via the Arrow-direct codegen dispatcher. The suite name did not convey that split, and reading a cast pair marked `Unsupported` there is easy to mistake for "not tested at all" rather than "tested through the dispatcher instead". Rename the suite to `CometNativeCastSuite` and add a class-level scaladoc stating the scope explicitly: Spark-parity for the native `Cast` kernel plus the `isSupported` matrix that gates it, with codegen dispatch semantics out of scope and covered by `CometCodegenSuite` and friends. Also annotate the one dispatch-adjacent test to say it only pins that `Unsupported` keeps the operator native. Updates the CI suite lists, contributor-guide examples, SQL fixture headers, and the native-crate comments that cross-reference the suite by name. --- .github/workflows/pr_build_linux.yml | 2 +- .github/workflows/pr_build_macos.yml | 2 +- docs/source/contributor-guide/development.md | 8 +++--- .../spark_configs_support.md | 2 +- .../spark-expr/src/conversion_funcs/string.rs | 4 +-- .../apache/comet/expressions/CometCast.scala | 2 +- .../cast/cast_string_to_date_ansi.sql | 2 +- .../expressions/cast/cast_string_trim.sql | 2 +- ...Suite.scala => CometNativeCastSuite.scala} | 26 ++++++++++++++++++- 9 files changed, 37 insertions(+), 13 deletions(-) rename spark/src/test/scala/org/apache/comet/{CometCastSuite.scala => CometNativeCastSuite.scala} (98%) diff --git a/.github/workflows/pr_build_linux.yml b/.github/workflows/pr_build_linux.yml index 0a71607ead..8fbf82d3f9 100644 --- a/.github/workflows/pr_build_linux.yml +++ b/.github/workflows/pr_build_linux.yml @@ -381,7 +381,7 @@ jobs: org.apache.comet.CometHashExpressionSuite org.apache.comet.CometTemporalExpressionSuite org.apache.comet.CometArrayExpressionSuite - org.apache.comet.CometCastSuite + org.apache.comet.CometNativeCastSuite 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 2335588b70..5b92c53c4a 100644 --- a/.github/workflows/pr_build_macos.yml +++ b/.github/workflows/pr_build_macos.yml @@ -197,7 +197,7 @@ jobs: org.apache.comet.CometHashExpressionSuite org.apache.comet.CometTemporalExpressionSuite org.apache.comet.CometArrayExpressionSuite - org.apache.comet.CometCastSuite + org.apache.comet.CometNativeCastSuite org.apache.comet.CometDateTimeUtilsSuite org.apache.comet.CometMathExpressionSuite org.apache.comet.CometStringExpressionSuite diff --git a/docs/source/contributor-guide/development.md b/docs/source/contributor-guide/development.md index 7e5c20c7ec..26fdbbb404 100644 --- a/docs/source/contributor-guide/development.md +++ b/docs/source/contributor-guide/development.md @@ -269,10 +269,10 @@ and `-Dtest=none`: ./mvnw test -Dtest=none -Dsuites="org.apache.comet.CometArrayExpressionSuite" # Run multiple suites (comma-separated, fully qualified) -./mvnw test -Dtest=none -Dsuites="org.apache.comet.CometCastSuite,org.apache.comet.CometArrayExpressionSuite" +./mvnw test -Dtest=none -Dsuites="org.apache.comet.CometNativeCastSuite,org.apache.comet.CometArrayExpressionSuite" # Run only tests whose name contains "valid" inside one suite -./mvnw test -Dtest=none -Dsuites="org.apache.comet.CometCastSuite valid" +./mvnw test -Dtest=none -Dsuites="org.apache.comet.CometNativeCastSuite valid" ``` `-Dtest=none` tells the Surefire (JUnit) plugin to skip its tests; without it, Surefire still @@ -474,10 +474,10 @@ However if the tests is related to the native side. Please make sure to run `mak Specify which ScalaTest suites to run with the `suites` argument and disable Surefire's JUnit discovery with `-Dtest=none`. For example, to run only the test cases containing _valid_ in -their name from `org.apache.comet.CometCastSuite`: +their name from `org.apache.comet.CometNativeCastSuite`: ```sh -./mvnw test -Dtest=none -Dsuites="org.apache.comet.CometCastSuite valid" +./mvnw test -Dtest=none -Dsuites="org.apache.comet.CometNativeCastSuite valid" ``` See [Running a Specific ScalaTest Suite](#running-a-specific-scalatest-suite) above for the full diff --git a/docs/source/contributor-guide/spark_configs_support.md b/docs/source/contributor-guide/spark_configs_support.md index 4ec8a99edf..b2c55bc146 100644 --- a/docs/source/contributor-guide/spark_configs_support.md +++ b/docs/source/contributor-guide/spark_configs_support.md @@ -104,7 +104,7 @@ across Spark 3.4, 3.5, 4.0, and 4.1. Three expression classes mix in `to_date`, and Spark 4's `try_to_timestamp`) `Cast` between strings and date / timestamp also reads the policy via the default -formatters but is tested separately by `CometCastSuite` and is out of scope here. +formatters but is tested separately by `CometNativeCastSuite` and is out of scope here. **Comet status.** None of the native implementations consult `legacyTimeParserPolicy` directly. Comet instead uses native implementations only for diff --git a/native/spark-expr/src/conversion_funcs/string.rs b/native/spark-expr/src/conversion_funcs/string.rs index 46495ed981..040c5737f7 100644 --- a/native/spark-expr/src/conversion_funcs/string.rs +++ b/native/spark-expr/src/conversion_funcs/string.rs @@ -2201,7 +2201,7 @@ mod tests { /// The codepoint matrix from /// : the ASCII control bytes and /// DELETE, plus the non-ASCII codepoints that are whitespace to Unicode but that no Spark - /// cast trims. `CometCastSuite` runs the same matrix with Spark itself as the oracle. + /// cast trims. `CometNativeCastSuite` runs the same matrix with Spark itself as the oracle. fn trim_pads() -> Vec { let mut pads: Vec = (0x00u8..=0x20).map(|b| String::from(b as char)).collect(); pads.push("\u{7f}".to_string()); @@ -2305,7 +2305,7 @@ mod tests { /// than change behaviour silently. `timestamp_parser` and `timestamp_ntz_parser` still use /// `str::trim`, so they accept the non-ASCII whitespace that Spark's /// `SparkDateTimeUtils.getTrimmedStart` / `getTrimmedEnd` leave in place, where Spark returns - /// NULL. `CometCastSuite` cannot cover this, because Spark is the oracle there and Comet does + /// NULL. `CometNativeCastSuite` cannot cover this, because Spark is the oracle there and Comet does /// not fall back -- it silently returns a value. #[test] fn test_cast_string_to_timestamp_unicode_whitespace_divergence() { 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 df4140078e..1aced4a96e 100644 --- a/spark/src/main/scala/org/apache/comet/expressions/CometCast.scala +++ b/spark/src/main/scala/org/apache/comet/expressions/CometCast.scala @@ -37,7 +37,7 @@ object CometCast with CometTypeShim with CodegenDispatchFallback { - // Shared with CometCastSuite so the asserted reason cannot drift from production. + // Shared with CometNativeCastSuite so the asserted reason cannot drift from production. private[comet] val negativeScaleDecimalToStringReason: String = "Negative-scale decimal requires spark.sql.legacy.allowNegativeScaleOfDecimal=true" diff --git a/spark/src/test/resources/sql-tests/expressions/cast/cast_string_to_date_ansi.sql b/spark/src/test/resources/sql-tests/expressions/cast/cast_string_to_date_ansi.sql index 763ec9705a..4b2a2405dd 100644 --- a/spark/src/test/resources/sql-tests/expressions/cast/cast_string_to_date_ansi.sql +++ b/spark/src/test/resources/sql-tests/expressions/cast/cast_string_to_date_ansi.sql @@ -19,7 +19,7 @@ -- shape of a date but names no real calendar date, rather than returning NULL. -- See https://github.com/apache/datafusion-comet/issues/5012. -- --- Per-string parity with Spark is covered by CometCastSuite "cast StringType to DateType"; +-- Per-string parity with Spark is covered by CometNativeCastSuite "cast StringType to DateType"; -- this fixture covers the plan shapes that suite does not exercise. -- Config: spark.sql.ansi.enabled=true diff --git a/spark/src/test/resources/sql-tests/expressions/cast/cast_string_trim.sql b/spark/src/test/resources/sql-tests/expressions/cast/cast_string_trim.sql index f7edaf315d..c14b893b0c 100644 --- a/spark/src/test/resources/sql-tests/expressions/cast/cast_string_trim.sql +++ b/spark/src/test/resources/sql-tests/expressions/cast/cast_string_trim.sql @@ -22,7 +22,7 @@ -- and NBSP / U+3000 are trimmed for nothing. See conversion_funcs::trim in the native crate and -- https://github.com/apache/datafusion-comet/issues/5149. -- --- Per-codepoint parity across all three eval modes is covered by the CometCastSuite +-- Per-codepoint parity across all three eval modes is covered by the CometNativeCastSuite -- "whitespace trim parity" tests; this fixture covers the same regimes over a Parquet column. statement diff --git a/spark/src/test/scala/org/apache/comet/CometCastSuite.scala b/spark/src/test/scala/org/apache/comet/CometNativeCastSuite.scala similarity index 98% rename from spark/src/test/scala/org/apache/comet/CometCastSuite.scala rename to spark/src/test/scala/org/apache/comet/CometNativeCastSuite.scala index 6e5b03ee35..3ac221c107 100644 --- a/spark/src/test/scala/org/apache/comet/CometCastSuite.scala +++ b/spark/src/test/scala/org/apache/comet/CometNativeCastSuite.scala @@ -40,7 +40,28 @@ import org.apache.comet.expressions.{CometCast, CometEvalMode} import org.apache.comet.rules.CometScanTypeChecker import org.apache.comet.serde.{Compatible, Incompatible, Unsupported} -class CometCastSuite extends CometTestBase with AdaptiveSparkPlanHelper { +/** + * Spark-parity coverage for Comet's **native** `Cast` implementation, and for the + * `CometCast.isSupported` matrix that decides which casts reach it. + * + * Scope: for every `(from, to)` type pair that `CometCast` reports as `Compatible` or + * `Incompatible`, run the cast against Spark as the oracle and assert both that the results match + * and — via `checkSparkAnswerAndOperator` — that the native kernel actually executed. Support + * decisions themselves (`Compatible` / `Incompatible` / `Unsupported`, and the reason strings) + * are asserted directly against `CometCast.isSupported`. + * + * Out of scope: the JVM codegen dispatch path. `CometCast` mixes in `CodegenDispatchFallback`, so + * a cast that `isSupported` rejects still runs inside the Comet operator by way of the + * Arrow-direct codegen dispatcher, evaluating Spark's own `Cast` against Arrow vectors. That path + * is covered by `CometCodegenSuite` and friends. Where this suite touches it at all, it does so + * only to pin that an `Unsupported` cast keeps the enclosing operator native rather than falling + * back to Spark; it does not attempt to cover dispatch semantics. + * + * Because of that split, a cast marked `Unsupported` here is not untested overall — it is tested + * through the dispatcher instead. Adding a native cast implementation therefore means moving a + * pair out of the `Unsupported` assertions and into the parity matrix below. + */ +class CometNativeCastSuite extends CometTestBase with AdaptiveSparkPlanHelper { import testImplicits._ @@ -1853,6 +1874,9 @@ class CometCastSuite extends CometTestBase with AdaptiveSparkPlanHelper { } test("cast ArrayType(DateType) to unsupported ArrayType routes through codegen dispatch") { + // Boundary case rather than dispatch coverage: these pairs have no native cast, so all this + // asserts is that `Unsupported` keeps the operator native via `CodegenDispatchFallback` + // instead of falling back to Spark. Dispatch semantics belong in the codegen suites. val fromType = ArrayType(DateType) val unsupportedElementTypes = Seq(BooleanType, ByteType, ShortType, LongType, FloatType, DoubleType, DecimalType(10, 2))