From fb9ca2681fe6a8343af39b2f7e0b4b2ea60fe228 Mon Sep 17 00:00:00 2001 From: comphead Date: Thu, 2 Jul 2026 13:25:08 -0700 Subject: [PATCH 01/34] chore: fallback to Spark if legacy sql configurations are set --- .../user-guide/latest/compatibility/index.md | 21 +++++++++++++ .../scala/org/apache/comet/CometConf.scala | 11 +++++++ .../comet/CometSparkSessionExtensions.scala | 18 +++++++++++ .../CometSparkSessionExtensionsSuite.scala | 30 +++++++++++++++++++ 4 files changed, 80 insertions(+) diff --git a/docs/source/user-guide/latest/compatibility/index.md b/docs/source/user-guide/latest/compatibility/index.md index b08d9a3bdb..32c81653de 100644 --- a/docs/source/user-guide/latest/compatibility/index.md +++ b/docs/source/user-guide/latest/compatibility/index.md @@ -102,3 +102,24 @@ string paths is tracked by Separately, Comet's native Parquet scan currently rejects string columns whose stored bytes are not valid UTF-8 rather than reading them like Spark ([#4121](https://github.com/apache/datafusion-comet/issues/4121)). + +## Spark legacy configs + +Spark exposes a family of `spark.sql.legacy.*` configs that opt a query into pre-modern Spark +semantics (for example, `spark.sql.legacy.castComplexTypesToString.enabled` or +`spark.sql.legacy.timeParserPolicy`). Comet implements current Spark semantics and does **not** +reproduce these legacy behaviors. + +By default, when Comet detects that any `spark.sql.legacy.*` config is set to `true`, it disables +itself for that session so the query runs on vanilla Spark and gets the expected legacy results. +The fallback is logged as a warning listing the offending config keys. + +If you would rather keep Comet enabled even when a legacy config is set, set: + +``` +spark.comet.legacyConfFallback.enabled=false +``` + +In that mode Comet will accelerate the query as usual, but it **cannot guarantee Spark +compatibility** — the legacy config will be silently ignored by Comet-executed operators, so +results may diverge from what Spark would produce with the same legacy config. diff --git a/spark/src/main/scala/org/apache/comet/CometConf.scala b/spark/src/main/scala/org/apache/comet/CometConf.scala index 41c99d2724..10e7b1ded6 100644 --- a/spark/src/main/scala/org/apache/comet/CometConf.scala +++ b/spark/src/main/scala/org/apache/comet/CometConf.scala @@ -93,6 +93,17 @@ object CometConf extends ShimCometConf { .booleanConf .createWithEnvVarOrDefault("ENABLE_COMET", true) + val COMET_LEGACY_CONF_FALLBACK_ENABLED: ConfigEntry[Boolean] = + conf("spark.comet.legacyConfFallback.enabled") + .category(CATEGORY_EXEC) + .doc( + "When true (default), Comet falls back to Spark whenever any spark.sql.legacy.* " + + "config is enabled, because Comet does not implement Spark's legacy semantics. Set " + + "to false to keep Comet enabled even when legacy configs are set; Comet cannot " + + "guarantee Spark compatibility in that case.") + .booleanConf + .createWithDefault(true) + val COMET_NATIVE_SCAN_ENABLED: ConfigEntry[Boolean] = conf("spark.comet.scan.enabled") .category(CATEGORY_TESTING) .doc("Whether to enable native scans. Intended for use in Comet's own test suites to " + diff --git a/spark/src/main/scala/org/apache/comet/CometSparkSessionExtensions.scala b/spark/src/main/scala/org/apache/comet/CometSparkSessionExtensions.scala index 202c52b158..0a7d10ef86 100644 --- a/spark/src/main/scala/org/apache/comet/CometSparkSessionExtensions.scala +++ b/spark/src/main/scala/org/apache/comet/CometSparkSessionExtensions.scala @@ -150,6 +150,24 @@ object CometSparkSessionExtensions extends Logging { return false } + // Fall back to Spark when any spark.sql.legacy.* config is enabled, since these opt into + // pre-modern Spark semantics that Comet does not replicate. Users can disable this fallback + // via COMET_LEGACY_CONF_FALLBACK_ENABLED if they accept the loss of Spark compatibility. + if (COMET_LEGACY_CONF_FALLBACK_ENABLED.get(conf)) { + val enabledLegacyConfs = conf.getAllConfs.collect { + case (k, v) if k.startsWith("spark.sql.legacy.") && v.equalsIgnoreCase("true") => k + } + if (enabledLegacyConfs.nonEmpty) { + logWarning( + "Comet extension is disabled because the following spark.sql.legacy.* configs are " + + s"enabled: ${enabledLegacyConfs.toSeq.sorted.mkString(", ")}. " + + "Comet does not support Spark legacy semantics. Set " + + s"${COMET_LEGACY_CONF_FALLBACK_ENABLED.key}=false to keep Comet enabled anyway " + + "(Spark compatibility is not guaranteed in that case).") + return false + } + } + try { // This will load the Comet native lib on demand, and if success, should set // `NativeBase.loaded` to true diff --git a/spark/src/test/scala/org/apache/comet/CometSparkSessionExtensionsSuite.scala b/spark/src/test/scala/org/apache/comet/CometSparkSessionExtensionsSuite.scala index 268fdf94eb..4a6ef0699f 100644 --- a/spark/src/test/scala/org/apache/comet/CometSparkSessionExtensionsSuite.scala +++ b/spark/src/test/scala/org/apache/comet/CometSparkSessionExtensionsSuite.scala @@ -53,6 +53,36 @@ class CometSparkSessionExtensionsSuite extends CometTestBase { NativeBase.setLoaded(true) } + test("isCometLoaded falls back to Spark when spark.sql.legacy.* is enabled") { + val conf = new SQLConf + conf.setConfString(CometConf.COMET_ENABLED.key, "true") + conf.setConfString(CometConf.COMET_EXEC_SHUFFLE_ENABLED.key, "false") + + // Baseline: no legacy configs set, Comet should load. + assert(isCometLoaded(conf)) + + // Any spark.sql.legacy.* set to true should disable Comet. + conf.setConfString("spark.sql.legacy.castComplexTypesToString.enabled", "true") + assert(!isCometLoaded(conf)) + + // Case-insensitive true value is also honored. + conf.setConfString("spark.sql.legacy.castComplexTypesToString.enabled", "TRUE") + assert(!isCometLoaded(conf)) + + // Setting the config to false should re-enable Comet. + conf.setConfString("spark.sql.legacy.castComplexTypesToString.enabled", "false") + assert(isCometLoaded(conf)) + + // Non-legacy spark.sql.* configs must not trigger fallback. + conf.setConfString("spark.sql.shuffle.partitions", "10") + assert(isCometLoaded(conf)) + + // Users can opt out of the legacy-config fallback and keep Comet enabled. + conf.setConfString("spark.sql.legacy.castComplexTypesToString.enabled", "true") + conf.setConfString(CometConf.COMET_LEGACY_CONF_FALLBACK_ENABLED.key, "false") + assert(isCometLoaded(conf)) + } + test("isCometLoaded requires CometShuffleManager when shuffle.enabled=true") { val conf = new SQLConf conf.setConfString(CometConf.COMET_ENABLED.key, "true") From a79ba6a4dad61f87e6d46b90969d3bc7e1a4746c Mon Sep 17 00:00:00 2001 From: comphead Date: Thu, 2 Jul 2026 14:24:46 -0700 Subject: [PATCH 02/34] chore: fallback to Spark if legacy sql configurations are set --- .../apache/comet/expressions/CometCast.scala | 38 ++-------- .../scala/org/apache/comet/serde/arrays.scala | 8 +- .../serde/operator/CometNativeScan.scala | 17 ++--- .../org/apache/comet/CometCastSuite.scala | 73 ------------------- .../apache/comet/CometExpressionSuite.scala | 33 --------- .../comet/exec/CometNativeReaderSuite.scala | 4 - 6 files changed, 16 insertions(+), 157 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 619b69912f..9873809518 100644 --- a/spark/src/main/scala/org/apache/comet/expressions/CometCast.scala +++ b/spark/src/main/scala/org/apache/comet/expressions/CometCast.scala @@ -32,23 +32,6 @@ import org.apache.comet.shims.CometExprShim object CometCast extends CometExpressionSerde[Cast] with CometExprShim { - // Shared with CometCastSuite so the asserted reason cannot drift from production. - private[comet] val negativeScaleDecimalToStringReason: String = - "Negative-scale decimal requires spark.sql.legacy.allowNegativeScaleOfDecimal=true" - - // When `spark.sql.legacy.castComplexTypesToString.enabled` is true, Spark wraps maps and - // structs with `[]` (instead of `{}`) when casting to string, and omits NULL elements of - // structs/maps/arrays (instead of rendering them as the literal "null"). Comet only - // implements the default formatting, so fall back to Spark for any array/map/struct to-string - // cast when the flag is enabled. The flag is internal in Spark 4.0 and defaults to false. - private[comet] val legacyCastComplexTypesToStringReason: String = - "spark.sql.legacy.castComplexTypesToString.enabled=true is not supported" - - private def legacyCastComplexTypesToString: Boolean = - SQLConf.get - .getConfString("spark.sql.legacy.castComplexTypesToString.enabled", "false") - .toBoolean - def supportedTypes: Seq[DataType] = Seq( DataTypes.BooleanType, @@ -163,12 +146,6 @@ object CometCast extends CometExpressionSerde[Cast] with CometExprShim { return Compatible() } - if (toType == DataTypes.StringType && legacyCastComplexTypesToString && (fromType - .isInstanceOf[ArrayType] || fromType.isInstanceOf[StructType] || - fromType.isInstanceOf[MapType])) { - return Unsupported(Some(legacyCastComplexTypesToStringReason)) - } - (fromType, toType) match { case (dt: ArrayType, _: ArrayType) if dt.elementType == NullType => Compatible() case (ArrayType(DataTypes.DateType, _), ArrayType(toElementType, _)) @@ -277,16 +254,11 @@ object CometCast extends CometExpressionSerde[Cast] with CometExprShim { "String formatting can differ for floating-point values near precision limits " + "or when scientific notation is used")) case d: DecimalType if d.scale < 0 => - // Negative-scale decimals require spark.sql.legacy.allowNegativeScaleOfDecimal=true. - // When that config is enabled, Spark formats them using Java BigDecimal.toString() - // which produces scientific notation (e.g. "1.23E+4"). Comet matches this behavior. - // When the config is disabled, negative-scale decimals cannot be created in Spark, - // so we mark this as incompatible to avoid native execution on unexpected inputs. - val allowNegativeScale = SQLConf.get - .getConfString("spark.sql.legacy.allowNegativeScaleOfDecimal", "false") - .toBoolean - if (allowNegativeScale) Compatible() - else Incompatible(Some(negativeScaleDecimalToStringReason)) + // Negative-scale decimals require spark.sql.legacy.allowNegativeScaleOfDecimal=true, + // which the blanket legacy-conf fallback in CometSparkSessionExtensions.isCometLoaded + // already disables Comet for. If a user opts out of that fallback, Spark formats these + // via Java BigDecimal.toString() (scientific notation) and Comet matches that behavior. + Compatible() case _: DecimalType => // Compatible across all eval modes: LEGACY uses cast_decimal128_to_utf8 which // replicates Java BigDecimal.toString() (scientific notation when adj_exp < -6); diff --git a/spark/src/main/scala/org/apache/comet/serde/arrays.scala b/spark/src/main/scala/org/apache/comet/serde/arrays.scala index 8bc639021c..59d092c3f4 100644 --- a/spark/src/main/scala/org/apache/comet/serde/arrays.scala +++ b/spark/src/main/scala/org/apache/comet/serde/arrays.scala @@ -24,7 +24,6 @@ import scala.jdk.CollectionConverters._ import org.apache.spark.sql.catalyst.expressions.{And, ArrayAggregate, ArrayAppend, ArrayContains, ArrayExcept, ArrayExists, ArrayFilter, ArrayForAll, ArrayInsert, ArrayIntersect, ArrayJoin, ArrayMax, ArrayMin, ArrayPosition, ArrayRemove, ArrayRepeat, ArraySort, ArraysOverlap, ArraysZip, ArrayTransform, ArrayUnion, Attribute, Cast, CreateArray, ElementAt, EmptyRow, Expression, Flatten, GetArrayItem, IsNotNull, LambdaFunction, Literal, NamedLambdaVariable, Reverse, Sequence, Size, Slice, SortArray, ZipWith} import org.apache.spark.sql.catalyst.util.GenericArrayData -import org.apache.spark.sql.internal.SQLConf import org.apache.spark.sql.types._ import org.apache.comet.CometConf @@ -451,15 +450,16 @@ object CometArrayInsert extends CometExpressionSerde[ArrayInsert] { val srcExprProto = exprToProtoInternal(expr.children.head, inputs, binding) val posExprProto = exprToProtoInternal(expr.children(1), inputs, binding) val itemExprProto = exprToProtoInternal(expr.children(2), inputs, binding) - val legacyNegativeIndex = - SQLConf.get.getConfString("spark.sql.legacy.negativeIndexInArrayInsert").toBoolean if (srcExprProto.isDefined && posExprProto.isDefined && itemExprProto.isDefined) { val arrayInsertBuilder = ExprOuterClass.ArrayInsert .newBuilder() .setSrcArrayExpr(srcExprProto.get) .setPosExpr(posExprProto.get) .setItemExpr(itemExprProto.get) - .setLegacyNegativeIndex(legacyNegativeIndex) + // spark.sql.legacy.negativeIndexInArrayInsert=true is handled by the blanket + // legacy-conf fallback in CometSparkSessionExtensions.isCometLoaded, so from + // Comet's perspective this always runs with the non-legacy semantics. + .setLegacyNegativeIndex(false) Some( ExprOuterClass.Expr diff --git a/spark/src/main/scala/org/apache/comet/serde/operator/CometNativeScan.scala b/spark/src/main/scala/org/apache/comet/serde/operator/CometNativeScan.scala index 763602bb7d..e56d1c7fa2 100644 --- a/spark/src/main/scala/org/apache/comet/serde/operator/CometNativeScan.scala +++ b/spark/src/main/scala/org/apache/comet/serde/operator/CometNativeScan.scala @@ -190,16 +190,13 @@ object CometNativeScan extends CometOperatorSerde[CometScanExec] with Logging { commonBuilder.setSessionTimezone(scan.conf.getConfString("spark.sql.session.timeZone")) commonBuilder.setCaseSensitive(scan.conf.getConf[Boolean](SQLConf.CASE_SENSITIVE)) - // SPARK-53535 (Spark 4.1+): when reading a struct whose requested fields are all - // missing in the Parquet file, the new default preserves the parent struct's - // nullness from the file (so non-null parents materialize as a struct of all-null - // fields). Pre-4.1 Spark hardcodes the legacy behavior (whole struct null), which - // matches the Comet default we use as fallback. - val returnNullStructConfKey = - "spark.sql.legacy.parquet.returnNullStructIfAllFieldsMissing" - val returnNullStructDefault = if (isSpark41Plus) "false" else "true" - commonBuilder.setReturnNullStructIfAllFieldsMissing( - scan.conf.getConfString(returnNullStructConfKey, returnNullStructDefault).toBoolean) + // SPARK-53535 (Spark 4.1+): reading a struct whose requested fields are all missing in + // the Parquet file preserves the parent struct's nullness. The legacy behavior is toggled + // by spark.sql.legacy.parquet.returnNullStructIfAllFieldsMissing, which is handled by the + // blanket legacy-conf fallback in CometSparkSessionExtensions.isCometLoaded. Comet always + // runs the modern (non-legacy) behavior on Spark 4.1+; on older Spark versions the legacy + // behavior is the hardcoded default, which matches Comet's fallback. + commonBuilder.setReturnNullStructIfAllFieldsMissing(!isSpark41Plus) // Field-ID matching: only ask the native side to do extra work when the conf is on AND // the requested schema actually carries IDs. Spark's ParquetReadSupport applies the same diff --git a/spark/src/test/scala/org/apache/comet/CometCastSuite.scala b/spark/src/test/scala/org/apache/comet/CometCastSuite.scala index 99475a252f..afb2069f40 100644 --- a/spark/src/test/scala/org/apache/comet/CometCastSuite.scala +++ b/spark/src/test/scala/org/apache/comet/CometCastSuite.scala @@ -734,66 +734,6 @@ class CometCastSuite extends CometTestBase with AdaptiveSparkPlanHelper { castTest(generateDecimalsPrecision38Scale18(), DataTypes.StringType) } - test("cast DecimalType with negative scale to StringType") { - // Negative-scale decimals are a legacy Spark feature gated on - // spark.sql.legacy.allowNegativeScaleOfDecimal=true. Spark LEGACY cast uses Java's - // BigDecimal.toString() which produces scientific notation for negative-scale values - // (e.g. 12300 stored as Decimal(7,-2) with unscaled=123 → "1.23E+4"). - // CometCast.canCastToString checks the - // config and returns Incompatible when it is false. - // - // Parquet does not support negative-scale decimals so we use checkSparkAnswer directly - // (no parquet round-trip) to avoid schema coercion. - - // With config enabled, enable localTableScan so Comet can take over the full plan - // and execute the cast natively. Parquet does not support negative-scale decimals so - // the data is kept in-memory; localTableScan.enabled bridges that gap. - withSQLConf( - "spark.sql.legacy.allowNegativeScaleOfDecimal" -> "true", - "spark.comet.exec.localTableScan.enabled" -> "true") { - val dfNeg2 = Seq( - Some(BigDecimal("0")), - Some(BigDecimal("100")), - Some(BigDecimal("12300")), - Some(BigDecimal("-99900")), - Some(BigDecimal("9999900")), - None) - .toDF("b") - .withColumn("a", col("b").cast(DecimalType(7, -2))) - .drop("b") - .select(col("a").cast(DataTypes.StringType).as("result")) - checkSparkAnswerAndOperator(dfNeg2) - - val dfNeg4 = Seq( - Some(BigDecimal("0")), - Some(BigDecimal("10000")), - Some(BigDecimal("120000")), - Some(BigDecimal("-9990000")), - None) - .toDF("b") - .withColumn("a", col("b").cast(DecimalType(7, -4))) - .drop("b") - .select(col("a").cast(DataTypes.StringType).as("result")) - checkSparkAnswerAndOperator(dfNeg4) - } - - // With config disabled (default): the SQL parser rejects negative scale, so - // negative-scale decimals cannot be created through normal SQL paths. - // CometCast.isSupported returns Incompatible for this case, ensuring Comet does - // not attempt native execution if such a value ever reaches the planner. - // Note: DecimalType(7, -2) must be constructed while config=true, because the - // constructor itself checks the config and throws if negative scale is disallowed. - var negScaleType: DecimalType = null - withSQLConf("spark.sql.legacy.allowNegativeScaleOfDecimal" -> "true") { - negScaleType = DecimalType(7, -2) - } - withSQLConf("spark.sql.legacy.allowNegativeScaleOfDecimal" -> "false") { - assert( - CometCast.isSupported(negScaleType, DataTypes.StringType, None, CometEvalMode.LEGACY) == - Incompatible(Some(CometCast.negativeScaleDecimalToStringReason))) - } - } - test("cast DecimalType(10,2) to TimestampType") { castTest(generateDecimalsPrecision10Scale2(), DataTypes.TimestampType) } @@ -1599,19 +1539,6 @@ class CometCastSuite extends CometTestBase with AdaptiveSparkPlanHelper { castTest(generateDecimalsPrecision10Scale2(), DataTypes.createDecimalType(10, 4)) } - test("cast StringType to DecimalType with negative scale (allowNegativeScaleOfDecimal)") { - // With allowNegativeScaleOfDecimal=true, Spark allows DECIMAL(p, s) where s < 0. - // The value is rounded to the nearest 10^|s| — e.g. DECIMAL(10,-4) rounds to - // the nearest 10000. This requires the legacy SQL parser config to be enabled. - withSQLConf("spark.sql.legacy.allowNegativeScaleOfDecimal" -> "true") { - val values = - Seq("12500", "15000", "99990000", "-12500", "0", "0.001", "abc", null).toDF("a") - // testTry=false: try_cast uses SQL string interpolation (toType.sql → "DECIMAL(10,-4)") - // which the SQL parser rejects regardless of allowNegativeScaleOfDecimal. - castTest(values, DataTypes.createDecimalType(10, -4), testTry = false) - } - } - test("cast between decimals with negative precision") { // cast to negative scale checkSparkAnswerMaybeThrows( diff --git a/spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala b/spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala index f3c442ad29..1d99b9a780 100644 --- a/spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala +++ b/spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala @@ -3174,39 +3174,6 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper { } } - test("vectorized reader: missing all struct fields") { - Seq(true, false).foreach { offheapEnabled => - withSQLConf( - SQLConf.USE_V1_SOURCE_LIST.key -> "parquet", - CometConf.COMET_EXEC_ENABLED.key -> "true", - CometConf.COMET_ENABLED.key -> "true", - CometConf.COMET_EXPLAIN_FALLBACK_ENABLED.key -> "false", - SQLConf.PARQUET_VECTORIZED_READER_NESTED_COLUMN_ENABLED.key -> "true", - SQLConf.COLUMN_VECTOR_OFFHEAP_ENABLED.key -> offheapEnabled.toString, - // SPARK-53535 (Spark 4.1+) flipped the default to "false", which preserves the parent - // struct's nullness so non-null parents materialise as Row(Row(null, null)). This test - // asserts the legacy "all missing fields => null struct" answer, so pin the conf to - // "true" to keep the expectation valid on both 3.x/4.0 and 4.1+. The non-legacy - // behaviour is covered separately by `issue #4136` in CometNativeReaderSuite. - "spark.sql.legacy.parquet.returnNullStructIfAllFieldsMissing" -> "true") { - val data = Seq(Tuple1((1, "a")), Tuple1((2, null)), Tuple1(null)) - - val readSchema = new StructType().add( - "_1", - new StructType() - .add("_3", IntegerType, nullable = false) - .add("_4", StringType, nullable = false), - nullable = false) - - withParquetFile(data) { file => - checkAnswer( - spark.read.schema(readSchema).parquet(file), - Row(null) :: Row(null) :: Row(null) :: Nil) - } - } - } - } - test("test length function") { // cast(id as binary) is rejected by Spark 4 ANSI analyzer withSQLConf(SQLConf.ANSI_ENABLED.key -> "false") { diff --git a/spark/src/test/scala/org/apache/comet/exec/CometNativeReaderSuite.scala b/spark/src/test/scala/org/apache/comet/exec/CometNativeReaderSuite.scala index 876565c5e5..86353c1926 100644 --- a/spark/src/test/scala/org/apache/comet/exec/CometNativeReaderSuite.scala +++ b/spark/src/test/scala/org/apache/comet/exec/CometNativeReaderSuite.scala @@ -726,10 +726,8 @@ class CometNativeReaderSuite extends CometTestBase with AdaptiveSparkPlanHelper // reader flag. We've seen CI fail on the off-heap branch when the on-heap branch passes. for { offheapEnabled <- Seq("true", "false") - legacy <- Seq("true", "false") } withSQLConf( "spark.sql.parquet.enableNestedColumnVectorizedReader" -> "true", - "spark.sql.legacy.parquet.returnNullStructIfAllFieldsMissing" -> legacy, "spark.sql.columnVector.offheap.enabled" -> offheapEnabled) { val df = spark.read.schema(readSchema).parquet(path.getCanonicalPath) checkSparkAnswer(df) @@ -773,10 +771,8 @@ class CometNativeReaderSuite extends CometTestBase with AdaptiveSparkPlanHelper for { offheapEnabled <- Seq("true", "false") - legacy <- Seq("true", "false") } withSQLConf( "spark.sql.parquet.enableNestedColumnVectorizedReader" -> "true", - "spark.sql.legacy.parquet.returnNullStructIfAllFieldsMissing" -> legacy, "spark.sql.columnVector.offheap.enabled" -> offheapEnabled) { val df = spark.read.schema(readSchema).parquet(path.getCanonicalPath) checkSparkAnswer(df) From cb6a4265310ca06a603c8a7c71d292931fc5e6ef Mon Sep 17 00:00:00 2001 From: comphead Date: Thu, 2 Jul 2026 17:40:55 -0700 Subject: [PATCH 03/34] chore: fallback to Spark if legacy sql configurations are set --- .../scala/org/apache/comet/CometConf.scala | 11 --- .../comet/CometSparkSessionExtensions.scala | 18 ----- .../apache/comet/expressions/CometCast.scala | 47 ++++++++++-- .../org/apache/comet/serde/aggregates.scala | 22 ++++++ .../scala/org/apache/comet/serde/arrays.scala | 35 +++++++-- .../serde/operator/CometNativeScan.scala | 17 +++-- .../cast_complex_types_to_string_legacy.sql | 22 +++--- .../org/apache/comet/CometCastSuite.scala | 73 +++++++++++++++++++ .../apache/comet/CometExpressionSuite.scala | 33 +++++++++ .../CometSparkSessionExtensionsSuite.scala | 30 -------- .../comet/exec/CometNativeReaderSuite.scala | 4 + 11 files changed, 223 insertions(+), 89 deletions(-) diff --git a/spark/src/main/scala/org/apache/comet/CometConf.scala b/spark/src/main/scala/org/apache/comet/CometConf.scala index 10e7b1ded6..41c99d2724 100644 --- a/spark/src/main/scala/org/apache/comet/CometConf.scala +++ b/spark/src/main/scala/org/apache/comet/CometConf.scala @@ -93,17 +93,6 @@ object CometConf extends ShimCometConf { .booleanConf .createWithEnvVarOrDefault("ENABLE_COMET", true) - val COMET_LEGACY_CONF_FALLBACK_ENABLED: ConfigEntry[Boolean] = - conf("spark.comet.legacyConfFallback.enabled") - .category(CATEGORY_EXEC) - .doc( - "When true (default), Comet falls back to Spark whenever any spark.sql.legacy.* " + - "config is enabled, because Comet does not implement Spark's legacy semantics. Set " + - "to false to keep Comet enabled even when legacy configs are set; Comet cannot " + - "guarantee Spark compatibility in that case.") - .booleanConf - .createWithDefault(true) - val COMET_NATIVE_SCAN_ENABLED: ConfigEntry[Boolean] = conf("spark.comet.scan.enabled") .category(CATEGORY_TESTING) .doc("Whether to enable native scans. Intended for use in Comet's own test suites to " + diff --git a/spark/src/main/scala/org/apache/comet/CometSparkSessionExtensions.scala b/spark/src/main/scala/org/apache/comet/CometSparkSessionExtensions.scala index 0a7d10ef86..202c52b158 100644 --- a/spark/src/main/scala/org/apache/comet/CometSparkSessionExtensions.scala +++ b/spark/src/main/scala/org/apache/comet/CometSparkSessionExtensions.scala @@ -150,24 +150,6 @@ object CometSparkSessionExtensions extends Logging { return false } - // Fall back to Spark when any spark.sql.legacy.* config is enabled, since these opt into - // pre-modern Spark semantics that Comet does not replicate. Users can disable this fallback - // via COMET_LEGACY_CONF_FALLBACK_ENABLED if they accept the loss of Spark compatibility. - if (COMET_LEGACY_CONF_FALLBACK_ENABLED.get(conf)) { - val enabledLegacyConfs = conf.getAllConfs.collect { - case (k, v) if k.startsWith("spark.sql.legacy.") && v.equalsIgnoreCase("true") => k - } - if (enabledLegacyConfs.nonEmpty) { - logWarning( - "Comet extension is disabled because the following spark.sql.legacy.* configs are " + - s"enabled: ${enabledLegacyConfs.toSeq.sorted.mkString(", ")}. " + - "Comet does not support Spark legacy semantics. Set " + - s"${COMET_LEGACY_CONF_FALLBACK_ENABLED.key}=false to keep Comet enabled anyway " + - "(Spark compatibility is not guaranteed in that case).") - return false - } - } - try { // This will load the Comet native lib on demand, and if success, should set // `NativeBase.loaded` to 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 9873809518..6e9767228e 100644 --- a/spark/src/main/scala/org/apache/comet/expressions/CometCast.scala +++ b/spark/src/main/scala/org/apache/comet/expressions/CometCast.scala @@ -25,12 +25,34 @@ import org.apache.spark.sql.types.{ArrayType, DataType, DataTypes, DecimalType, import org.apache.comet.CometConf import org.apache.comet.CometSparkSessionExtensions.{isSpark40Plus, withFallbackReason} -import org.apache.comet.serde.{CometExpressionSerde, Compatible, ExprOuterClass, Incompatible, SupportLevel, Unsupported} +import org.apache.comet.serde.{CodegenDispatchFallback, CometExpressionSerde, Compatible, ExprOuterClass, Incompatible, SupportLevel, Unsupported} import org.apache.comet.serde.ExprOuterClass.Expr import org.apache.comet.serde.QueryPlanSerde.{evalModeToProto, exprToProtoInternal, serializeDataType} import org.apache.comet.shims.CometExprShim -object CometCast extends CometExpressionSerde[Cast] with CometExprShim { +object CometCast + extends CometExpressionSerde[Cast] + with CometExprShim + with CodegenDispatchFallback { + + // Shared with CometCastSuite so the asserted reason cannot drift from production. + private[comet] val negativeScaleDecimalToStringReason: String = + "Negative-scale decimal requires spark.sql.legacy.allowNegativeScaleOfDecimal=true" + + // When `spark.sql.legacy.castComplexTypesToString.enabled` is true, Spark wraps maps and + // structs with `[]` (instead of `{}`) when casting to string, and omits NULL elements of + // structs/maps/arrays (instead of rendering them as the literal "null"). Comet's native cast + // only implements the default formatting, so when the flag is on we mark the cast Incompatible + // and let the [[CodegenDispatchFallback]] trait route it through the JVM codegen dispatcher + // (Spark's own `doGenCode` inside the Comet kernel) so results still match Spark exactly. The + // flag is internal in Spark 4.0 and defaults to false. + private[comet] val legacyCastComplexTypesToStringReason: String = + "spark.sql.legacy.castComplexTypesToString.enabled=true is not supported natively" + + private def legacyCastComplexTypesToString: Boolean = + SQLConf.get + .getConfString("spark.sql.legacy.castComplexTypesToString.enabled", "false") + .toBoolean def supportedTypes: Seq[DataType] = Seq( @@ -146,6 +168,12 @@ object CometCast extends CometExpressionSerde[Cast] with CometExprShim { return Compatible() } + if (toType == DataTypes.StringType && legacyCastComplexTypesToString && (fromType + .isInstanceOf[ArrayType] || fromType.isInstanceOf[StructType] || + fromType.isInstanceOf[MapType])) { + return Incompatible(Some(legacyCastComplexTypesToStringReason)) + } + (fromType, toType) match { case (dt: ArrayType, _: ArrayType) if dt.elementType == NullType => Compatible() case (ArrayType(DataTypes.DateType, _), ArrayType(toElementType, _)) @@ -254,11 +282,16 @@ object CometCast extends CometExpressionSerde[Cast] with CometExprShim { "String formatting can differ for floating-point values near precision limits " + "or when scientific notation is used")) case d: DecimalType if d.scale < 0 => - // Negative-scale decimals require spark.sql.legacy.allowNegativeScaleOfDecimal=true, - // which the blanket legacy-conf fallback in CometSparkSessionExtensions.isCometLoaded - // already disables Comet for. If a user opts out of that fallback, Spark formats these - // via Java BigDecimal.toString() (scientific notation) and Comet matches that behavior. - Compatible() + // Negative-scale decimals require spark.sql.legacy.allowNegativeScaleOfDecimal=true. + // When that config is enabled, Spark formats them using Java BigDecimal.toString() + // which produces scientific notation (e.g. "1.23E+4"). Comet matches this behavior. + // When the config is disabled, negative-scale decimals cannot be created in Spark, + // so we mark this as incompatible to avoid native execution on unexpected inputs. + val allowNegativeScale = SQLConf.get + .getConfString("spark.sql.legacy.allowNegativeScaleOfDecimal", "false") + .toBoolean + if (allowNegativeScale) Compatible() + else Incompatible(Some(negativeScaleDecimalToStringReason)) case _: DecimalType => // Compatible across all eval modes: LEGACY uses cast_decimal128_to_utf8 which // replicates Java BigDecimal.toString() (scientific notation when adj_exp < -6); diff --git a/spark/src/main/scala/org/apache/comet/serde/aggregates.scala b/spark/src/main/scala/org/apache/comet/serde/aggregates.scala index 1fb676366c..1bb932c528 100644 --- a/spark/src/main/scala/org/apache/comet/serde/aggregates.scala +++ b/spark/src/main/scala/org/apache/comet/serde/aggregates.scala @@ -108,6 +108,28 @@ object CometMax extends CometAggregateExpressionSerde[Max] { } object CometCount extends CometAggregateExpressionSerde[Count] { + + // When `spark.sql.legacy.allowParameterlessCount=true`, Spark allows `count()` with no + // arguments and treats it as `count(*)`. Comet's native planner asserts on non-empty children + // and would panic on such an expression, so mark it Unsupported here and let the aggregate fall + // back to Spark. Aggregate serdes have no [[CodegenDispatchFallback]] path (aggregates cannot + // be routed through the JVM codegen dispatcher), so a clean Spark fallback is the appropriate + // outcome. Under the default config value, Spark's analyzer rejects parameterless `count()` so + // this branch is unreachable. + private val legacyAllowParameterlessCountReason: String = + "`spark.sql.legacy.allowParameterlessCount=true` produces `count()` with no children, which " + + "the native planner does not support" + + override def getUnsupportedReasons(): Seq[String] = Seq(legacyAllowParameterlessCountReason) + + override def getSupportLevel(expr: Count): SupportLevel = { + if (expr.children.isEmpty) { + Unsupported(Some(legacyAllowParameterlessCountReason)) + } else { + Compatible() + } + } + override def convert( aggExpr: AggregateExpression, expr: Count, diff --git a/spark/src/main/scala/org/apache/comet/serde/arrays.scala b/spark/src/main/scala/org/apache/comet/serde/arrays.scala index 59d092c3f4..da46388ace 100644 --- a/spark/src/main/scala/org/apache/comet/serde/arrays.scala +++ b/spark/src/main/scala/org/apache/comet/serde/arrays.scala @@ -24,6 +24,7 @@ import scala.jdk.CollectionConverters._ import org.apache.spark.sql.catalyst.expressions.{And, ArrayAggregate, ArrayAppend, ArrayContains, ArrayExcept, ArrayExists, ArrayFilter, ArrayForAll, ArrayInsert, ArrayIntersect, ArrayJoin, ArrayMax, ArrayMin, ArrayPosition, ArrayRemove, ArrayRepeat, ArraySort, ArraysOverlap, ArraysZip, ArrayTransform, ArrayUnion, Attribute, Cast, CreateArray, ElementAt, EmptyRow, Expression, Flatten, GetArrayItem, IsNotNull, LambdaFunction, Literal, NamedLambdaVariable, Reverse, Sequence, Size, Slice, SortArray, ZipWith} import org.apache.spark.sql.catalyst.util.GenericArrayData +import org.apache.spark.sql.internal.SQLConf import org.apache.spark.sql.types._ import org.apache.comet.CometConf @@ -439,9 +440,28 @@ object CometArrayJoin } } -object CometArrayInsert extends CometExpressionSerde[ArrayInsert] { +object CometArrayInsert extends CometExpressionSerde[ArrayInsert] with CodegenDispatchFallback { - override def getSupportLevel(expr: ArrayInsert): SupportLevel = Compatible() + // Spark's `spark.sql.legacy.negativeIndexInArrayInsert=true` changes how a 0-based/negative + // position is interpreted. Rather than maintain a parallel native code path for the legacy + // semantics, mark `array_insert` Incompatible when the flag is on so + // [[CodegenDispatchFallback]] routes the expression through the JVM codegen dispatcher + // (Spark's own `doGenCode` inside the Comet kernel) — that gives Spark-exact results + // without duplicating the legacy branch natively. + private val legacyNegativeIndexConfig = "spark.sql.legacy.negativeIndexInArrayInsert" + + private val legacyNegativeIndexReason = + s"`$legacyNegativeIndexConfig=true` legacy negative-index semantics are not implemented natively" + + override def getIncompatibleReasons(): Seq[String] = Seq(legacyNegativeIndexReason) + + override def getSupportLevel(expr: ArrayInsert): SupportLevel = { + if (SQLConf.get.getConfString(legacyNegativeIndexConfig, "false").toBoolean) { + Incompatible(Some(legacyNegativeIndexReason)) + } else { + Compatible() + } + } override def convert( expr: ArrayInsert, @@ -450,16 +470,19 @@ object CometArrayInsert extends CometExpressionSerde[ArrayInsert] { val srcExprProto = exprToProtoInternal(expr.children.head, inputs, binding) val posExprProto = exprToProtoInternal(expr.children(1), inputs, binding) val itemExprProto = exprToProtoInternal(expr.children(2), inputs, binding) + // Reached in two cases: + // 1. Legacy conf is false → getSupportLevel returned Compatible → run native. + // 2. Legacy conf is true AND user set allowIncompatible=true → opt in to native. + // In case (2) the native impl honors the legacy semantics directly so we forward the flag. + val legacyNegativeIndex = + SQLConf.get.getConfString(legacyNegativeIndexConfig, "false").toBoolean if (srcExprProto.isDefined && posExprProto.isDefined && itemExprProto.isDefined) { val arrayInsertBuilder = ExprOuterClass.ArrayInsert .newBuilder() .setSrcArrayExpr(srcExprProto.get) .setPosExpr(posExprProto.get) .setItemExpr(itemExprProto.get) - // spark.sql.legacy.negativeIndexInArrayInsert=true is handled by the blanket - // legacy-conf fallback in CometSparkSessionExtensions.isCometLoaded, so from - // Comet's perspective this always runs with the non-legacy semantics. - .setLegacyNegativeIndex(false) + .setLegacyNegativeIndex(legacyNegativeIndex) Some( ExprOuterClass.Expr diff --git a/spark/src/main/scala/org/apache/comet/serde/operator/CometNativeScan.scala b/spark/src/main/scala/org/apache/comet/serde/operator/CometNativeScan.scala index e56d1c7fa2..763602bb7d 100644 --- a/spark/src/main/scala/org/apache/comet/serde/operator/CometNativeScan.scala +++ b/spark/src/main/scala/org/apache/comet/serde/operator/CometNativeScan.scala @@ -190,13 +190,16 @@ object CometNativeScan extends CometOperatorSerde[CometScanExec] with Logging { commonBuilder.setSessionTimezone(scan.conf.getConfString("spark.sql.session.timeZone")) commonBuilder.setCaseSensitive(scan.conf.getConf[Boolean](SQLConf.CASE_SENSITIVE)) - // SPARK-53535 (Spark 4.1+): reading a struct whose requested fields are all missing in - // the Parquet file preserves the parent struct's nullness. The legacy behavior is toggled - // by spark.sql.legacy.parquet.returnNullStructIfAllFieldsMissing, which is handled by the - // blanket legacy-conf fallback in CometSparkSessionExtensions.isCometLoaded. Comet always - // runs the modern (non-legacy) behavior on Spark 4.1+; on older Spark versions the legacy - // behavior is the hardcoded default, which matches Comet's fallback. - commonBuilder.setReturnNullStructIfAllFieldsMissing(!isSpark41Plus) + // SPARK-53535 (Spark 4.1+): when reading a struct whose requested fields are all + // missing in the Parquet file, the new default preserves the parent struct's + // nullness from the file (so non-null parents materialize as a struct of all-null + // fields). Pre-4.1 Spark hardcodes the legacy behavior (whole struct null), which + // matches the Comet default we use as fallback. + val returnNullStructConfKey = + "spark.sql.legacy.parquet.returnNullStructIfAllFieldsMissing" + val returnNullStructDefault = if (isSpark41Plus) "false" else "true" + commonBuilder.setReturnNullStructIfAllFieldsMissing( + scan.conf.getConfString(returnNullStructConfKey, returnNullStructDefault).toBoolean) // Field-ID matching: only ask the native side to do extra work when the conf is on AND // the requested schema actually carries IDs. Spark's ParquetReadSupport applies the same diff --git a/spark/src/test/resources/sql-tests/expressions/cast/cast_complex_types_to_string_legacy.sql b/spark/src/test/resources/sql-tests/expressions/cast/cast_complex_types_to_string_legacy.sql index 2c0bc19b3b..92010a7c89 100644 --- a/spark/src/test/resources/sql-tests/expressions/cast/cast_complex_types_to_string_legacy.sql +++ b/spark/src/test/resources/sql-tests/expressions/cast/cast_complex_types_to_string_legacy.sql @@ -17,24 +17,26 @@ -- When `spark.sql.legacy.castComplexTypesToString.enabled` is true Spark wraps maps and -- structs with `[...]` (instead of `{...}`) and omits NULL elements of structs/maps/arrays --- (instead of rendering them as the literal "null"). Comet only implements the default --- formatting, so any array/map/struct → string cast must fall back to Spark. +-- (instead of rendering them as the literal "null"). Comet's native cast does not implement +-- the legacy formatting; the [[CodegenDispatchFallback]] mixin on `CometCast` routes these +-- casts through the JVM codegen dispatcher (Spark's own `doGenCode` inside the Comet kernel) +-- so results match Spark exactly without a Spark fallback. -- The flag is internal in Spark 4.0 and defaults to false. -- Config: spark.sql.legacy.castComplexTypesToString.enabled=true --- Struct → string falls back. -query expect_fallback(spark.sql.legacy.castComplexTypesToString.enabled=true is not supported) +-- Struct → string routed through the codegen dispatcher. +query SELECT CAST(struct(1, 2, null) AS STRING) --- Array → string falls back (NULL elements rendered differently between modes). -query expect_fallback(spark.sql.legacy.castComplexTypesToString.enabled=true is not supported) +-- Array → string routed through the codegen dispatcher. +query SELECT CAST(array(1, 2, null) AS STRING) --- Map → string falls back (`[]` vs `{}` wrapping differs between modes). -query expect_fallback(spark.sql.legacy.castComplexTypesToString.enabled=true is not supported) +-- Map → string routed through the codegen dispatcher. +query SELECT CAST(map('a', 1, 'b', null) AS STRING) --- Nested complex types still fall back through the outer type. -query expect_fallback(spark.sql.legacy.castComplexTypesToString.enabled=true is not supported) +-- Nested complex types also routed through the codegen dispatcher via the outer type. +query SELECT CAST(struct(array(1, null), map('k', null)) AS STRING) diff --git a/spark/src/test/scala/org/apache/comet/CometCastSuite.scala b/spark/src/test/scala/org/apache/comet/CometCastSuite.scala index afb2069f40..99475a252f 100644 --- a/spark/src/test/scala/org/apache/comet/CometCastSuite.scala +++ b/spark/src/test/scala/org/apache/comet/CometCastSuite.scala @@ -734,6 +734,66 @@ class CometCastSuite extends CometTestBase with AdaptiveSparkPlanHelper { castTest(generateDecimalsPrecision38Scale18(), DataTypes.StringType) } + test("cast DecimalType with negative scale to StringType") { + // Negative-scale decimals are a legacy Spark feature gated on + // spark.sql.legacy.allowNegativeScaleOfDecimal=true. Spark LEGACY cast uses Java's + // BigDecimal.toString() which produces scientific notation for negative-scale values + // (e.g. 12300 stored as Decimal(7,-2) with unscaled=123 → "1.23E+4"). + // CometCast.canCastToString checks the + // config and returns Incompatible when it is false. + // + // Parquet does not support negative-scale decimals so we use checkSparkAnswer directly + // (no parquet round-trip) to avoid schema coercion. + + // With config enabled, enable localTableScan so Comet can take over the full plan + // and execute the cast natively. Parquet does not support negative-scale decimals so + // the data is kept in-memory; localTableScan.enabled bridges that gap. + withSQLConf( + "spark.sql.legacy.allowNegativeScaleOfDecimal" -> "true", + "spark.comet.exec.localTableScan.enabled" -> "true") { + val dfNeg2 = Seq( + Some(BigDecimal("0")), + Some(BigDecimal("100")), + Some(BigDecimal("12300")), + Some(BigDecimal("-99900")), + Some(BigDecimal("9999900")), + None) + .toDF("b") + .withColumn("a", col("b").cast(DecimalType(7, -2))) + .drop("b") + .select(col("a").cast(DataTypes.StringType).as("result")) + checkSparkAnswerAndOperator(dfNeg2) + + val dfNeg4 = Seq( + Some(BigDecimal("0")), + Some(BigDecimal("10000")), + Some(BigDecimal("120000")), + Some(BigDecimal("-9990000")), + None) + .toDF("b") + .withColumn("a", col("b").cast(DecimalType(7, -4))) + .drop("b") + .select(col("a").cast(DataTypes.StringType).as("result")) + checkSparkAnswerAndOperator(dfNeg4) + } + + // With config disabled (default): the SQL parser rejects negative scale, so + // negative-scale decimals cannot be created through normal SQL paths. + // CometCast.isSupported returns Incompatible for this case, ensuring Comet does + // not attempt native execution if such a value ever reaches the planner. + // Note: DecimalType(7, -2) must be constructed while config=true, because the + // constructor itself checks the config and throws if negative scale is disallowed. + var negScaleType: DecimalType = null + withSQLConf("spark.sql.legacy.allowNegativeScaleOfDecimal" -> "true") { + negScaleType = DecimalType(7, -2) + } + withSQLConf("spark.sql.legacy.allowNegativeScaleOfDecimal" -> "false") { + assert( + CometCast.isSupported(negScaleType, DataTypes.StringType, None, CometEvalMode.LEGACY) == + Incompatible(Some(CometCast.negativeScaleDecimalToStringReason))) + } + } + test("cast DecimalType(10,2) to TimestampType") { castTest(generateDecimalsPrecision10Scale2(), DataTypes.TimestampType) } @@ -1539,6 +1599,19 @@ class CometCastSuite extends CometTestBase with AdaptiveSparkPlanHelper { castTest(generateDecimalsPrecision10Scale2(), DataTypes.createDecimalType(10, 4)) } + test("cast StringType to DecimalType with negative scale (allowNegativeScaleOfDecimal)") { + // With allowNegativeScaleOfDecimal=true, Spark allows DECIMAL(p, s) where s < 0. + // The value is rounded to the nearest 10^|s| — e.g. DECIMAL(10,-4) rounds to + // the nearest 10000. This requires the legacy SQL parser config to be enabled. + withSQLConf("spark.sql.legacy.allowNegativeScaleOfDecimal" -> "true") { + val values = + Seq("12500", "15000", "99990000", "-12500", "0", "0.001", "abc", null).toDF("a") + // testTry=false: try_cast uses SQL string interpolation (toType.sql → "DECIMAL(10,-4)") + // which the SQL parser rejects regardless of allowNegativeScaleOfDecimal. + castTest(values, DataTypes.createDecimalType(10, -4), testTry = false) + } + } + test("cast between decimals with negative precision") { // cast to negative scale checkSparkAnswerMaybeThrows( diff --git a/spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala b/spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala index 1d99b9a780..f3c442ad29 100644 --- a/spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala +++ b/spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala @@ -3174,6 +3174,39 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper { } } + test("vectorized reader: missing all struct fields") { + Seq(true, false).foreach { offheapEnabled => + withSQLConf( + SQLConf.USE_V1_SOURCE_LIST.key -> "parquet", + CometConf.COMET_EXEC_ENABLED.key -> "true", + CometConf.COMET_ENABLED.key -> "true", + CometConf.COMET_EXPLAIN_FALLBACK_ENABLED.key -> "false", + SQLConf.PARQUET_VECTORIZED_READER_NESTED_COLUMN_ENABLED.key -> "true", + SQLConf.COLUMN_VECTOR_OFFHEAP_ENABLED.key -> offheapEnabled.toString, + // SPARK-53535 (Spark 4.1+) flipped the default to "false", which preserves the parent + // struct's nullness so non-null parents materialise as Row(Row(null, null)). This test + // asserts the legacy "all missing fields => null struct" answer, so pin the conf to + // "true" to keep the expectation valid on both 3.x/4.0 and 4.1+. The non-legacy + // behaviour is covered separately by `issue #4136` in CometNativeReaderSuite. + "spark.sql.legacy.parquet.returnNullStructIfAllFieldsMissing" -> "true") { + val data = Seq(Tuple1((1, "a")), Tuple1((2, null)), Tuple1(null)) + + val readSchema = new StructType().add( + "_1", + new StructType() + .add("_3", IntegerType, nullable = false) + .add("_4", StringType, nullable = false), + nullable = false) + + withParquetFile(data) { file => + checkAnswer( + spark.read.schema(readSchema).parquet(file), + Row(null) :: Row(null) :: Row(null) :: Nil) + } + } + } + } + test("test length function") { // cast(id as binary) is rejected by Spark 4 ANSI analyzer withSQLConf(SQLConf.ANSI_ENABLED.key -> "false") { diff --git a/spark/src/test/scala/org/apache/comet/CometSparkSessionExtensionsSuite.scala b/spark/src/test/scala/org/apache/comet/CometSparkSessionExtensionsSuite.scala index 4a6ef0699f..268fdf94eb 100644 --- a/spark/src/test/scala/org/apache/comet/CometSparkSessionExtensionsSuite.scala +++ b/spark/src/test/scala/org/apache/comet/CometSparkSessionExtensionsSuite.scala @@ -53,36 +53,6 @@ class CometSparkSessionExtensionsSuite extends CometTestBase { NativeBase.setLoaded(true) } - test("isCometLoaded falls back to Spark when spark.sql.legacy.* is enabled") { - val conf = new SQLConf - conf.setConfString(CometConf.COMET_ENABLED.key, "true") - conf.setConfString(CometConf.COMET_EXEC_SHUFFLE_ENABLED.key, "false") - - // Baseline: no legacy configs set, Comet should load. - assert(isCometLoaded(conf)) - - // Any spark.sql.legacy.* set to true should disable Comet. - conf.setConfString("spark.sql.legacy.castComplexTypesToString.enabled", "true") - assert(!isCometLoaded(conf)) - - // Case-insensitive true value is also honored. - conf.setConfString("spark.sql.legacy.castComplexTypesToString.enabled", "TRUE") - assert(!isCometLoaded(conf)) - - // Setting the config to false should re-enable Comet. - conf.setConfString("spark.sql.legacy.castComplexTypesToString.enabled", "false") - assert(isCometLoaded(conf)) - - // Non-legacy spark.sql.* configs must not trigger fallback. - conf.setConfString("spark.sql.shuffle.partitions", "10") - assert(isCometLoaded(conf)) - - // Users can opt out of the legacy-config fallback and keep Comet enabled. - conf.setConfString("spark.sql.legacy.castComplexTypesToString.enabled", "true") - conf.setConfString(CometConf.COMET_LEGACY_CONF_FALLBACK_ENABLED.key, "false") - assert(isCometLoaded(conf)) - } - test("isCometLoaded requires CometShuffleManager when shuffle.enabled=true") { val conf = new SQLConf conf.setConfString(CometConf.COMET_ENABLED.key, "true") diff --git a/spark/src/test/scala/org/apache/comet/exec/CometNativeReaderSuite.scala b/spark/src/test/scala/org/apache/comet/exec/CometNativeReaderSuite.scala index 86353c1926..876565c5e5 100644 --- a/spark/src/test/scala/org/apache/comet/exec/CometNativeReaderSuite.scala +++ b/spark/src/test/scala/org/apache/comet/exec/CometNativeReaderSuite.scala @@ -726,8 +726,10 @@ class CometNativeReaderSuite extends CometTestBase with AdaptiveSparkPlanHelper // reader flag. We've seen CI fail on the off-heap branch when the on-heap branch passes. for { offheapEnabled <- Seq("true", "false") + legacy <- Seq("true", "false") } withSQLConf( "spark.sql.parquet.enableNestedColumnVectorizedReader" -> "true", + "spark.sql.legacy.parquet.returnNullStructIfAllFieldsMissing" -> legacy, "spark.sql.columnVector.offheap.enabled" -> offheapEnabled) { val df = spark.read.schema(readSchema).parquet(path.getCanonicalPath) checkSparkAnswer(df) @@ -771,8 +773,10 @@ class CometNativeReaderSuite extends CometTestBase with AdaptiveSparkPlanHelper for { offheapEnabled <- Seq("true", "false") + legacy <- Seq("true", "false") } withSQLConf( "spark.sql.parquet.enableNestedColumnVectorizedReader" -> "true", + "spark.sql.legacy.parquet.returnNullStructIfAllFieldsMissing" -> legacy, "spark.sql.columnVector.offheap.enabled" -> offheapEnabled) { val df = spark.read.schema(readSchema).parquet(path.getCanonicalPath) checkSparkAnswer(df) From e84b42e2eb9a751c725c7abefee7988a4932863b Mon Sep 17 00:00:00 2001 From: comphead Date: Thu, 2 Jul 2026 17:42:09 -0700 Subject: [PATCH 04/34] chore: fallback to Spark if legacy sql configurations are set --- .../aggregate/count_parameterless_legacy.sql | 45 ++++++++++++++ .../array/array_insert_legacy_dispatch.sql | 60 +++++++++++++++++++ 2 files changed, 105 insertions(+) create mode 100644 spark/src/test/resources/sql-tests/expressions/aggregate/count_parameterless_legacy.sql create mode 100644 spark/src/test/resources/sql-tests/expressions/array/array_insert_legacy_dispatch.sql diff --git a/spark/src/test/resources/sql-tests/expressions/aggregate/count_parameterless_legacy.sql b/spark/src/test/resources/sql-tests/expressions/aggregate/count_parameterless_legacy.sql new file mode 100644 index 0000000000..b47476b0f5 --- /dev/null +++ b/spark/src/test/resources/sql-tests/expressions/aggregate/count_parameterless_legacy.sql @@ -0,0 +1,45 @@ +-- 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. + +-- When `spark.sql.legacy.allowParameterlessCount=true`, Spark accepts `count()` (no arguments) +-- and treats it as `count(*)`. Comet's native planner asserts non-empty children on Count, so +-- `CometCount.getSupportLevel` marks parameterless Count `Unsupported` and lets the aggregate +-- fall back to Spark. Aggregate serdes do not have a JVM codegen dispatcher path, so the +-- Spark fallback is the correct outcome. + +-- Config: spark.sql.legacy.allowParameterlessCount=true + +statement +CREATE TABLE test_count_parameterless(i int, grp string) USING parquet + +statement +INSERT INTO test_count_parameterless VALUES (1, 'x'), (2, 'x'), (NULL, 'y'), (3, 'y'), (NULL, 'y') + +-- Parameterless count() falls back to Spark; the aggregate result must still match Spark. +query expect_fallback(spark.sql.legacy.allowParameterlessCount=true) +SELECT count() FROM test_count_parameterless + +-- Parameterless count() with GROUP BY. +query expect_fallback(spark.sql.legacy.allowParameterlessCount=true) +SELECT grp, count() FROM test_count_parameterless GROUP BY grp ORDER BY grp + +-- Parameterless count() on empty table. +statement +CREATE TABLE test_count_empty(i int) USING parquet + +query expect_fallback(spark.sql.legacy.allowParameterlessCount=true) +SELECT count() FROM test_count_empty diff --git a/spark/src/test/resources/sql-tests/expressions/array/array_insert_legacy_dispatch.sql b/spark/src/test/resources/sql-tests/expressions/array/array_insert_legacy_dispatch.sql new file mode 100644 index 0000000000..4229312c3e --- /dev/null +++ b/spark/src/test/resources/sql-tests/expressions/array/array_insert_legacy_dispatch.sql @@ -0,0 +1,60 @@ +-- 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. + +-- Tests array_insert with legacy negative index mode enabled but without opting into the +-- native (incompatible) path. `CometArrayInsert` mixes in [[CodegenDispatchFallback]] so with +-- spark.sql.legacy.negativeIndexInArrayInsert=true the expression is routed through the JVM +-- codegen dispatcher (Spark's own `doGenCode` inside the Comet kernel), producing Spark-exact +-- results without a Spark fallback and without touching the native legacy branch. +-- The companion file array_insert_legacy.sql covers the allowIncompatible=true opt-in path. + +-- ConfigMatrix: parquet.enable.dictionary=false,true +-- Config: spark.sql.legacy.negativeIndexInArrayInsert=true + +-- -1 inserts before last element in legacy mode +query +SELECT array_insert(array(1, 2, 3), -1, 10) + +-- -2 inserts before second-to-last +query +SELECT array_insert(array(1, 2, 3), -2, 10) + +-- -3 inserts before first element +query +SELECT array_insert(array(1, 2, 3), -3, 10) + +-- negative beyond start with null padding (legacy mode pads differently) +query +SELECT array_insert(array(1, 2, 3), -5, 10) + +-- far negative beyond start +query +SELECT array_insert(array(1, 3, 4), -2, 2) + +-- column-based test +statement +CREATE TABLE test_ai_legacy_dispatch(arr array, pos int, val int) USING parquet + +statement +INSERT INTO test_ai_legacy_dispatch VALUES + (array(1, 2, 3), -1, 10), + (array(4, 5), -1, 20), + (array(1, 2, 3), -4, 10), + (NULL, -1, 10) + +query +SELECT array_insert(arr, pos, val) FROM test_ai_legacy_dispatch From 8f103de31891d7e9acbde7bfd1a839badebdca12 Mon Sep 17 00:00:00 2001 From: comphead Date: Mon, 6 Jul 2026 09:11:42 -0700 Subject: [PATCH 05/34] chore: fallback to Spark if legacy sql configurations are set --- .../user-guide/latest/compatibility/index.md | 112 +++++++++++++++--- .../scala/org/apache/comet/CometConf.scala | 14 +++ .../comet/CometSparkSessionExtensions.scala | 21 ++++ .../org/apache/comet/LegacyConfFallback.scala | 85 +++++++++++++ .../scala/org/apache/comet/serde/arrays.scala | 9 +- .../org/apache/comet/serde/predicates.scala | 35 +++++- .../org/apache/comet/serde/strings.scala | 13 +- .../org/apache/comet/serde/structs.scala | 17 ++- .../CometSparkSessionExtensionsSuite.scala | 35 ++++++ 9 files changed, 314 insertions(+), 27 deletions(-) create mode 100644 spark/src/main/scala/org/apache/comet/LegacyConfFallback.scala diff --git a/docs/source/user-guide/latest/compatibility/index.md b/docs/source/user-guide/latest/compatibility/index.md index 32c81653de..2ba7a9cf2c 100644 --- a/docs/source/user-guide/latest/compatibility/index.md +++ b/docs/source/user-guide/latest/compatibility/index.md @@ -106,20 +106,98 @@ valid UTF-8 rather than reading them like Spark ## Spark legacy configs Spark exposes a family of `spark.sql.legacy.*` configs that opt a query into pre-modern Spark -semantics (for example, `spark.sql.legacy.castComplexTypesToString.enabled` or -`spark.sql.legacy.timeParserPolicy`). Comet implements current Spark semantics and does **not** -reproduce these legacy behaviors. - -By default, when Comet detects that any `spark.sql.legacy.*` config is set to `true`, it disables -itself for that session so the query runs on vanilla Spark and gets the expected legacy results. -The fallback is logged as a warning listing the offending config keys. - -If you would rather keep Comet enabled even when a legacy config is set, set: - -``` -spark.comet.legacyConfFallback.enabled=false -``` - -In that mode Comet will accelerate the query as usual, but it **cannot guarantee Spark -compatibility** — the legacy config will be silently ignored by Comet-executed operators, so -results may diverge from what Spark would produce with the same legacy config. +semantics. Comet handles these in two ways: + +- **Per-expression**: when a legacy config affects a specific Spark expression that Comet + supports (for example `spark.sql.legacy.castComplexTypesToString.enabled` for `Cast`, + `spark.sql.legacy.negativeIndexInArrayInsert` for `array_insert`, + `spark.sql.legacy.nullInEmptyListBehavior` for `IN`), Comet's serde routes the expression + through the JVM codegen dispatcher (Spark's own `doGenCode` inside the Comet kernel) or + through a native code path that honors the flag. No session-wide fallback is triggered. +- **Session-wide execution fallback**: when a legacy config affects execution semantics but + is consumed by an analyzer/optimizer rule, a data-source reader/writer, or a type-system + utility (rather than a specific Comet-supported expression), Comet cannot fix the divergence + in a single serde. Instead, when + [`spark.comet.legacyConfFallback.enabled`](../configs.md) is `true` (default) and any config + in the curated list is set to a non-default value, Comet disables itself for the session so + Spark's own execution provides the legacy semantics. The warning names the offending config + keys. + +### Curated legacy configs that trigger the session-wide fallback + +The list below is the exact set checked by +`spark.comet.legacyConfFallback.enabled`. Each entry names the Spark config key and the value +Comet compares against. The comparison is case-insensitive, and the fallback only fires when +the key is explicitly set in the session AND its value differs from the recorded default. Keys +absent from the session conf never trigger the fallback, regardless of their runtime resolution +in Spark. The defaults recorded here are Spark 4.0's static defaults; when a Spark 4.0 default +depends on another config (for example ANSI mode), the value used is what Spark 4.0 itself +resolves to under its own defaults. + +**Decimal type-system / analyzer rules** + +| Config key | Comet-expected default | +| --- | --- | +| `spark.sql.legacy.allowNegativeScaleOfDecimal` | `false` | +| `spark.sql.legacy.decimal.retainFractionDigitsOnTruncate` | `false` | +| `spark.sql.legacy.literal.pickMinimumPrecision` | `true` | + +**Char/varchar padding and analyzer-inserted write-side validation** + +| Config key | Comet-expected default | +| --- | --- | +| `spark.sql.legacy.charVarcharAsString` | `false` | + +**Type coercion and upcast rules** + +| Config key | Comet-expected default | +| --- | --- | +| `spark.sql.legacy.doLooseUpcast` | `false` | +| `spark.sql.legacy.typeCoercion.datetimeToString.enabled` | `false` | + +**Optimizer rules that reshape plans handed to Comet** + +| Config key | Comet-expected default | +| --- | --- | +| `spark.sql.legacy.duplicateBetweenInput` | `false` | +| `spark.sql.legacy.inSubqueryNullability` | `false` | +| `spark.sql.legacy.scalarSubqueryCountBugBehavior` | `false` | +| `spark.sql.legacy.disableMapKeyNormalization` | `false` | +| `spark.sql.legacy.setopsPrecedence.enabled` | `false` | + +**View resolution (Cast vs. UpCast injection)** + +| Config key | Comet-expected default | +| --- | --- | +| `spark.sql.legacy.viewSchemaCompensation` | `true` | + +**Datetime parser policy** + +| Config key | Comet-expected default | +| --- | --- | +| `spark.sql.legacy.timeParserPolicy` | `CORRECTED` | + +**Parquet reader/writer semantics** + +| Config key | Comet-expected default | +| --- | --- | +| `spark.sql.legacy.parquet.datetimeRebaseModeInRead` | `CORRECTED` | +| `spark.sql.legacy.parquet.datetimeRebaseModeInWrite` | `CORRECTED` | +| `spark.sql.legacy.parquet.int96RebaseModeInRead` | `CORRECTED` | +| `spark.sql.legacy.parquet.int96RebaseModeInWrite` | `CORRECTED` | +| `spark.sql.legacy.parquet.nanosAsLong` | `false` | + +**Cached-plan behavior on file-source scans** + +| Config key | Comet-expected default | +| --- | --- | +| `spark.sql.legacy.readFileSourceTableCacheIgnoreOptions` | `false` | + +### Opting out of the session-wide fallback + +The fallback is on by default (`spark.comet.legacyConfFallback.enabled=true`). To keep Comet +enabled even when one of the configs above is set to a non-default value, set +`spark.comet.legacyConfFallback.enabled=false`. In that mode Comet's native operators do not +implement the legacy semantics the flag requests: results may silently diverge from Spark for +queries that touch the affected code paths. Spark compatibility is not guaranteed while the +opt-out is in effect. diff --git a/spark/src/main/scala/org/apache/comet/CometConf.scala b/spark/src/main/scala/org/apache/comet/CometConf.scala index 41c99d2724..3f57c4bd6d 100644 --- a/spark/src/main/scala/org/apache/comet/CometConf.scala +++ b/spark/src/main/scala/org/apache/comet/CometConf.scala @@ -93,6 +93,20 @@ object CometConf extends ShimCometConf { .booleanConf .createWithEnvVarOrDefault("ENABLE_COMET", true) + val COMET_LEGACY_CONF_FALLBACK_ENABLED: ConfigEntry[Boolean] = + conf("spark.comet.legacyConfFallback.enabled") + .category(CATEGORY_EXEC) + .doc( + "When true (default), Comet disables itself for the session if any spark.sql.legacy.* " + + "config that Comet does NOT already handle per-expression is set to a non-default " + + "value. Legacy configs consumed by specific Spark expressions are already routed " + + "through the JVM codegen dispatcher (or an explicit incompat check) inside Comet " + + "and do not trigger this fallback. Set this config to false to keep Comet enabled " + + "when other legacy configs are set; Spark compatibility is not guaranteed in that " + + "case.") + .booleanConf + .createWithDefault(true) + val COMET_NATIVE_SCAN_ENABLED: ConfigEntry[Boolean] = conf("spark.comet.scan.enabled") .category(CATEGORY_TESTING) .doc("Whether to enable native scans. Intended for use in Comet's own test suites to " + diff --git a/spark/src/main/scala/org/apache/comet/CometSparkSessionExtensions.scala b/spark/src/main/scala/org/apache/comet/CometSparkSessionExtensions.scala index 202c52b158..07be830ec0 100644 --- a/spark/src/main/scala/org/apache/comet/CometSparkSessionExtensions.scala +++ b/spark/src/main/scala/org/apache/comet/CometSparkSessionExtensions.scala @@ -150,6 +150,27 @@ object CometSparkSessionExtensions extends Logging { return false } + // Some spark.sql.legacy.* configs affect execution semantics for queries Comet accelerates + // but are not tied to a specific expression that Comet's serdes can gate on (parquet + // datetime rebase modes, decimal-precision analyzer rules, type-coercion policies, etc.). + // When any such config is set to a non-default value we disable Comet for the session so + // Spark's own execution provides the legacy semantics. The list is intentionally narrow -- + // legacy configs whose consumers ARE Comet-supported expressions (Cast, ArrayInsert, In, + // etc.) are handled per-expression via [[CodegenDispatchFallback]] and are NOT in this set. + if (COMET_LEGACY_CONF_FALLBACK_ENABLED.get(conf)) { + val triggered = LegacyConfFallback.triggeredConfigs(conf) + if (triggered.nonEmpty) { + val keys = triggered.toSeq.sorted.mkString(", ") + logWarning( + "Comet extension is disabled because the following execution-affecting " + + s"spark.sql.legacy.* configs are set to non-default values: $keys. Comet does not " + + "implement these legacy execution semantics. To keep Comet enabled anyway, set " + + s"${COMET_LEGACY_CONF_FALLBACK_ENABLED.key}=false (Spark compatibility is not " + + "guaranteed in that case).") + return false + } + } + try { // This will load the Comet native lib on demand, and if success, should set // `NativeBase.loaded` to true diff --git a/spark/src/main/scala/org/apache/comet/LegacyConfFallback.scala b/spark/src/main/scala/org/apache/comet/LegacyConfFallback.scala new file mode 100644 index 0000000000..1e92213179 --- /dev/null +++ b/spark/src/main/scala/org/apache/comet/LegacyConfFallback.scala @@ -0,0 +1,85 @@ +/* + * 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.internal.SQLConf + +/** + * Curated set of Spark `spark.sql.legacy.*` configs whose behavior is NOT tied to a specific + * Comet-supported expression (built-in functions with a legacy dependency are handled + * per-expression via [[org.apache.comet.serde.CodegenDispatchFallback]] or a native passthrough + * in the serde). The keys in this list are consumed by analyzer/optimizer rules, data-source + * readers/writers, or type-system utilities, and Comet's native execution does not replicate + * their legacy semantics. + * + * When [[CometConf.COMET_LEGACY_CONF_FALLBACK_ENABLED]] is true (default), Comet disables itself + * for the session if any of these keys is set to its non-default value, so Spark's own execution + * path is used instead. Users can set `spark.comet.legacyConfFallback.enabled=false` to override + * the fallback and keep Comet enabled (Spark compatibility is not guaranteed in that case). + */ +private[comet] object LegacyConfFallback { + + /** + * Map of legacy config key -> case-insensitive Spark default value. A config triggers the + * fallback when it is present in the session conf AND its value is not equal (case-insensitive) + * to the default recorded here. + */ + val executionAffectingDefaults: Map[String, String] = Map( + // Decimal type-system / analyzer rules that reshape plans reaching Comet. + "spark.sql.legacy.allowNegativeScaleOfDecimal" -> "false", + "spark.sql.legacy.decimal.retainFractionDigitsOnTruncate" -> "false", + "spark.sql.legacy.literal.pickMinimumPrecision" -> "false", + // Char/varchar padding + write-side validation inserted by the analyzer. + "spark.sql.legacy.charVarcharAsString" -> "false", + // Type-coercion / upcast rules. + "spark.sql.legacy.doLooseUpcast" -> "false", + "spark.sql.legacy.typeCoercion.datetimeToString.enabled" -> "false", + // Optimizer rules that reshape plans (subqueries, Between, empty-list IN nullability). + "spark.sql.legacy.duplicateBetweenInput" -> "false", + "spark.sql.legacy.inSubqueryNullability" -> "false", + "spark.sql.legacy.scalarSubqueryCountBugBehavior" -> "false", + // Map-key normalization used by CreateMap and friends inside ArrayBasedMapBuilder. + "spark.sql.legacy.disableMapKeyNormalization" -> "false", + // Set-op precedence changes the plan topology handed to Comet operators. + "spark.sql.legacy.setopsPrecedence.enabled" -> "false", + // View schema compensation controls whether Cast (Comet-supported) or UpCast (Comet + // unsupported) is injected during view resolution. + "spark.sql.legacy.viewSchemaCompensation" -> "true", + // Datetime parser policy affects CSV/JSON scan options and datetime formatters. + "spark.sql.legacy.timeParserPolicy" -> "CORRECTED", + // Datasource readers/writers Comet may accelerate. + "spark.sql.legacy.parquet.datetimeRebaseModeInRead" -> "CORRECTED", + "spark.sql.legacy.parquet.datetimeRebaseModeInWrite" -> "CORRECTED", + "spark.sql.legacy.parquet.int96RebaseModeInRead" -> "CORRECTED", + "spark.sql.legacy.parquet.int96RebaseModeInWrite" -> "CORRECTED", + "spark.sql.legacy.parquet.nanosAsLong" -> "false", + // Cached-plan behavior that leaves stale options on a Comet-accelerated file scan. + "spark.sql.legacy.readFileSourceTableCacheIgnoreOptions" -> "false") + + /** Keys in [[executionAffectingDefaults]] that are set to a non-default value on `conf`. */ + def triggeredConfigs(conf: SQLConf): Iterable[String] = { + executionAffectingDefaults.iterator.collect { + case (key, safeDefault) + if conf.contains(key) && + !conf.getConfString(key).equalsIgnoreCase(safeDefault) => + key + }.toSeq + } +} diff --git a/spark/src/main/scala/org/apache/comet/serde/arrays.scala b/spark/src/main/scala/org/apache/comet/serde/arrays.scala index da46388ace..1b91406c49 100644 --- a/spark/src/main/scala/org/apache/comet/serde/arrays.scala +++ b/spark/src/main/scala/org/apache/comet/serde/arrays.scala @@ -446,12 +446,13 @@ object CometArrayInsert extends CometExpressionSerde[ArrayInsert] with CodegenDi // position is interpreted. Rather than maintain a parallel native code path for the legacy // semantics, mark `array_insert` Incompatible when the flag is on so // [[CodegenDispatchFallback]] routes the expression through the JVM codegen dispatcher - // (Spark's own `doGenCode` inside the Comet kernel) — that gives Spark-exact results + // (Spark's own `doGenCode` inside the Comet kernel) -- that gives Spark-exact results // without duplicating the legacy branch natively. private val legacyNegativeIndexConfig = "spark.sql.legacy.negativeIndexInArrayInsert" private val legacyNegativeIndexReason = - s"`$legacyNegativeIndexConfig=true` legacy negative-index semantics are not implemented natively" + s"`$legacyNegativeIndexConfig=true` legacy negative-index semantics are not implemented" + + " natively" override def getIncompatibleReasons(): Seq[String] = Seq(legacyNegativeIndexReason) @@ -471,8 +472,8 @@ object CometArrayInsert extends CometExpressionSerde[ArrayInsert] with CodegenDi val posExprProto = exprToProtoInternal(expr.children(1), inputs, binding) val itemExprProto = exprToProtoInternal(expr.children(2), inputs, binding) // Reached in two cases: - // 1. Legacy conf is false → getSupportLevel returned Compatible → run native. - // 2. Legacy conf is true AND user set allowIncompatible=true → opt in to native. + // 1. Legacy conf is false -> getSupportLevel returned Compatible -> run native. + // 2. Legacy conf is true AND user set allowIncompatible=true -> opt in to native. // In case (2) the native impl honors the legacy semantics directly so we forward the flag. val legacyNegativeIndex = SQLConf.get.getConfString(legacyNegativeIndexConfig, "false").toBoolean diff --git a/spark/src/main/scala/org/apache/comet/serde/predicates.scala b/spark/src/main/scala/org/apache/comet/serde/predicates.scala index c9a70a830f..4d26975f7d 100644 --- a/spark/src/main/scala/org/apache/comet/serde/predicates.scala +++ b/spark/src/main/scala/org/apache/comet/serde/predicates.scala @@ -243,8 +243,14 @@ object CometIsNaN extends CometExpressionSerde[IsNaN] { object CometIn extends CometExpressionSerde[In] with CodegenDispatchFallback { + override def getIncompatibleReasons(): Seq[String] = Seq(LegacyConfHelpers.nullInEmptyListReason) + override def getSupportLevel(expr: In): SupportLevel = - ComparisonUtils.collationSupportLevel("In", (expr.value +: expr.list): _*) + if (expr.list.isEmpty && LegacyConfHelpers.legacyNullInEmptyBehavior) { + Incompatible(Some(LegacyConfHelpers.nullInEmptyListReason)) + } else { + ComparisonUtils.collationSupportLevel("In", (expr.value +: expr.list): _*) + } override def getUnsupportedReasons(): Seq[String] = Seq(ComparisonUtils.nonDefaultCollationDocReason) @@ -259,8 +265,14 @@ object CometIn extends CometExpressionSerde[In] with CodegenDispatchFallback { object CometInSet extends CometExpressionSerde[InSet] with CodegenDispatchFallback { + override def getIncompatibleReasons(): Seq[String] = Seq(LegacyConfHelpers.nullInEmptyListReason) + override def getSupportLevel(expr: InSet): SupportLevel = - ComparisonUtils.collationSupportLevel("InSet", expr.child) + if (expr.hset.isEmpty && LegacyConfHelpers.legacyNullInEmptyBehavior) { + Incompatible(Some(LegacyConfHelpers.nullInEmptyListReason)) + } else { + ComparisonUtils.collationSupportLevel("InSet", expr.child) + } override def getUnsupportedReasons(): Seq[String] = Seq(ComparisonUtils.nonDefaultCollationDocReason) @@ -296,6 +308,25 @@ trait CollationAwareBinaryPredicate[T <: BinaryExpression] Seq(ComparisonUtils.nonDefaultCollationDocReason) } +private[serde] object LegacyConfHelpers { + + // Reason string shared with CometIn/CometInSet for the `null IN (empty)` divergence. + val nullInEmptyListReason: String = + "`spark.sql.legacy.nullInEmptyListBehavior=true` (or its effective default `!ansiEnabled`)" + + " changes `null IN (empty list)` from false to null; the native in-list path only" + + " implements the non-legacy semantics." + + // Resolve `spark.sql.legacy.nullInEmptyListBehavior` the same way Spark does: use the explicit + // value if set, otherwise fall back to `!ansiEnabled`. Read by string key to stay compatible + // with Spark versions where the accessor is not available. + def legacyNullInEmptyBehavior: Boolean = { + val conf = SQLConf.get + Option(conf.getConfString("spark.sql.legacy.nullInEmptyListBehavior", null)) + .map(_.equalsIgnoreCase("true")) + .getOrElse(!conf.ansiEnabled) + } +} + object ComparisonUtils { // Comet's native equality/ordering/hashing compare raw bytes, so any predicate operand carrying diff --git a/spark/src/main/scala/org/apache/comet/serde/strings.scala b/spark/src/main/scala/org/apache/comet/serde/strings.scala index 1e124dbd42..48ead53ad9 100644 --- a/spark/src/main/scala/org/apache/comet/serde/strings.scala +++ b/spark/src/main/scala/org/apache/comet/serde/strings.scala @@ -380,12 +380,15 @@ object CometRLike extends CometExpressionSerde[RLike] with NativeOptInAvailable private object PadReasons { val literalStrReason = "Scalar values are not supported for the `str` argument." val nonLiteralPadReason = "Only scalar values are supported for the `pad` argument." + val binaryStrReason: String = + "`spark.sql.legacy.lpadRpadAlwaysReturnString=true` allows lpad/rpad to run with a" + + " BinaryType `str` argument; Comet's native `lpad`/`rpad` only support string inputs." } object CometStringRPad extends CometExpressionSerde[StringRPad] { override def getUnsupportedReasons(): Seq[String] = - Seq(PadReasons.literalStrReason, PadReasons.nonLiteralPadReason) + Seq(PadReasons.literalStrReason, PadReasons.nonLiteralPadReason, PadReasons.binaryStrReason) override def getSupportLevel(expr: StringRPad): SupportLevel = { if (expr.str.isInstanceOf[Literal]) { @@ -394,6 +397,9 @@ object CometStringRPad extends CometExpressionSerde[StringRPad] { if (!expr.pad.isInstanceOf[Literal]) { return Unsupported(Some(PadReasons.nonLiteralPadReason)) } + if (expr.str.dataType == BinaryType) { + return Unsupported(Some(PadReasons.binaryStrReason)) + } Compatible() } @@ -413,7 +419,7 @@ object CometStringRPad extends CometExpressionSerde[StringRPad] { object CometStringLPad extends CometExpressionSerde[StringLPad] { override def getUnsupportedReasons(): Seq[String] = - Seq(PadReasons.literalStrReason, PadReasons.nonLiteralPadReason) + Seq(PadReasons.literalStrReason, PadReasons.nonLiteralPadReason, PadReasons.binaryStrReason) override def getSupportLevel(expr: StringLPad): SupportLevel = { if (expr.str.isInstanceOf[Literal]) { @@ -422,6 +428,9 @@ object CometStringLPad extends CometExpressionSerde[StringLPad] { if (!expr.pad.isInstanceOf[Literal]) { return Unsupported(Some(PadReasons.nonLiteralPadReason)) } + if (expr.str.dataType == BinaryType) { + return Unsupported(Some(PadReasons.binaryStrReason)) + } Compatible() } diff --git a/spark/src/main/scala/org/apache/comet/serde/structs.scala b/spark/src/main/scala/org/apache/comet/serde/structs.scala index 409ef38b4f..c30fe1d67a 100644 --- a/spark/src/main/scala/org/apache/comet/serde/structs.scala +++ b/spark/src/main/scala/org/apache/comet/serde/structs.scala @@ -259,13 +259,23 @@ object CometJsonToStructs extends CometCodegenDispatch[JsonToStructs] with Nativ } } -object CometStructsToCsv extends CometExpressionSerde[StructsToCsv] { +object CometStructsToCsv extends CometExpressionSerde[StructsToCsv] with CodegenDispatchFallback { private val incompatibleDataTypes = Seq(DateType, TimestampType, TimestampNTZType, BinaryType) + // When true, Spark's UnivocityGenerator wraps null values as quoted empty strings; Comet's + // native to_csv writer emits unquoted empty strings. Mark Incompatible so the + // CodegenDispatchFallback trait routes the expression through the JVM codegen dispatcher. + private val legacyNullValueConfKey = + "spark.sql.legacy.nullValueWrittenAsQuotedEmptyStringCsv" + private val legacyNullValueReason = + s"`$legacyNullValueConfKey=true` quotes NULLs as an empty quoted string in the CSV output;" + + " Comet's native `to_csv` writer does not implement that legacy behavior." + override def getIncompatibleReasons(): Seq[String] = Seq( "Date, Timestamp, TimestampNTZ, and Binary data types may produce different results" + - " (https://github.com/apache/datafusion-comet/issues/3232)") + " (https://github.com/apache/datafusion-comet/issues/3232)", + legacyNullValueReason) override def getUnsupportedReasons(): Seq[String] = Seq( "Complex types (arrays, maps, structs) in the schema are not supported") @@ -285,6 +295,9 @@ object CometStructsToCsv extends CometExpressionSerde[StructsToCsv] { s"The schema ${expr.inputSchema} is not supported because " + s"it includes a incompatible data types: $incompatibleDataTypes")) } + if (SQLConf.get.getConfString(legacyNullValueConfKey, "false").toBoolean) { + return Incompatible(Some(legacyNullValueReason)) + } // https://github.com/apache/datafusion-comet/issues/3232 Incompatible() } diff --git a/spark/src/test/scala/org/apache/comet/CometSparkSessionExtensionsSuite.scala b/spark/src/test/scala/org/apache/comet/CometSparkSessionExtensionsSuite.scala index 268fdf94eb..e3358c6f77 100644 --- a/spark/src/test/scala/org/apache/comet/CometSparkSessionExtensionsSuite.scala +++ b/spark/src/test/scala/org/apache/comet/CometSparkSessionExtensionsSuite.scala @@ -53,6 +53,41 @@ class CometSparkSessionExtensionsSuite extends CometTestBase { NativeBase.setLoaded(true) } + test("isCometLoaded falls back when execution-affecting spark.sql.legacy.* config is set") { + val conf = new SQLConf + conf.setConfString(CometConf.COMET_ENABLED.key, "true") + conf.setConfString(CometConf.COMET_EXEC_SHUFFLE_ENABLED.key, "false") + + // Baseline: no legacy configs set, Comet should load. + assert(isCometLoaded(conf)) + + // A single boolean-false-default execution-affecting legacy config triggers the fallback. + conf.setConfString("spark.sql.legacy.allowNegativeScaleOfDecimal", "true") + assert(!isCometLoaded(conf)) + + // Setting the config back to its Spark default (case-insensitive) clears the trigger. + conf.setConfString("spark.sql.legacy.allowNegativeScaleOfDecimal", "FALSE") + assert(isCometLoaded(conf)) + + // Enum-default configs also trigger when set to a non-default value. + conf.setConfString("spark.sql.legacy.timeParserPolicy", "LEGACY") + assert(!isCometLoaded(conf)) + conf.setConfString("spark.sql.legacy.timeParserPolicy", "CORRECTED") + assert(isCometLoaded(conf)) + + // Legacy configs handled per-expression (e.g. castComplexTypesToString) are NOT part of the + // fallback set and must not disable Comet on their own. + conf.setConfString("spark.sql.legacy.castComplexTypesToString.enabled", "true") + assert(isCometLoaded(conf)) + conf.unsetConf("spark.sql.legacy.castComplexTypesToString.enabled") + + // Opt-out: users can keep Comet enabled by disabling the fallback (compatibility not + // guaranteed). + conf.setConfString("spark.sql.legacy.allowNegativeScaleOfDecimal", "true") + conf.setConfString(CometConf.COMET_LEGACY_CONF_FALLBACK_ENABLED.key, "false") + assert(isCometLoaded(conf)) + } + test("isCometLoaded requires CometShuffleManager when shuffle.enabled=true") { val conf = new SQLConf conf.setConfString(CometConf.COMET_ENABLED.key, "true") From 2c88b045c9613ba51d9d4db0b0b56df2c50fac9e Mon Sep 17 00:00:00 2001 From: comphead Date: Mon, 6 Jul 2026 09:43:32 -0700 Subject: [PATCH 06/34] chore: fallback to Spark if legacy sql configurations are set --- .../user-guide/latest/compatibility/index.md | 70 +++++++++---------- 1 file changed, 35 insertions(+), 35 deletions(-) diff --git a/docs/source/user-guide/latest/compatibility/index.md b/docs/source/user-guide/latest/compatibility/index.md index 2ba7a9cf2c..4d09948fc0 100644 --- a/docs/source/user-guide/latest/compatibility/index.md +++ b/docs/source/user-guide/latest/compatibility/index.md @@ -136,62 +136,62 @@ resolves to under its own defaults. **Decimal type-system / analyzer rules** -| Config key | Comet-expected default | -| --- | --- | -| `spark.sql.legacy.allowNegativeScaleOfDecimal` | `false` | -| `spark.sql.legacy.decimal.retainFractionDigitsOnTruncate` | `false` | -| `spark.sql.legacy.literal.pickMinimumPrecision` | `true` | +| Config key | Comet-expected default | +| --------------------------------------------------------- | ---------------------- | +| `spark.sql.legacy.allowNegativeScaleOfDecimal` | `false` | +| `spark.sql.legacy.decimal.retainFractionDigitsOnTruncate` | `false` | +| `spark.sql.legacy.literal.pickMinimumPrecision` | `true` | **Char/varchar padding and analyzer-inserted write-side validation** -| Config key | Comet-expected default | -| --- | --- | -| `spark.sql.legacy.charVarcharAsString` | `false` | +| Config key | Comet-expected default | +| -------------------------------------- | ---------------------- | +| `spark.sql.legacy.charVarcharAsString` | `false` | **Type coercion and upcast rules** -| Config key | Comet-expected default | -| --- | --- | -| `spark.sql.legacy.doLooseUpcast` | `false` | -| `spark.sql.legacy.typeCoercion.datetimeToString.enabled` | `false` | +| Config key | Comet-expected default | +| -------------------------------------------------------- | ---------------------- | +| `spark.sql.legacy.doLooseUpcast` | `false` | +| `spark.sql.legacy.typeCoercion.datetimeToString.enabled` | `false` | **Optimizer rules that reshape plans handed to Comet** -| Config key | Comet-expected default | -| --- | --- | -| `spark.sql.legacy.duplicateBetweenInput` | `false` | -| `spark.sql.legacy.inSubqueryNullability` | `false` | -| `spark.sql.legacy.scalarSubqueryCountBugBehavior` | `false` | -| `spark.sql.legacy.disableMapKeyNormalization` | `false` | -| `spark.sql.legacy.setopsPrecedence.enabled` | `false` | +| Config key | Comet-expected default | +| ------------------------------------------------- | ---------------------- | +| `spark.sql.legacy.duplicateBetweenInput` | `false` | +| `spark.sql.legacy.inSubqueryNullability` | `false` | +| `spark.sql.legacy.scalarSubqueryCountBugBehavior` | `false` | +| `spark.sql.legacy.disableMapKeyNormalization` | `false` | +| `spark.sql.legacy.setopsPrecedence.enabled` | `false` | **View resolution (Cast vs. UpCast injection)** -| Config key | Comet-expected default | -| --- | --- | -| `spark.sql.legacy.viewSchemaCompensation` | `true` | +| Config key | Comet-expected default | +| ----------------------------------------- | ---------------------- | +| `spark.sql.legacy.viewSchemaCompensation` | `true` | **Datetime parser policy** -| Config key | Comet-expected default | -| --- | --- | -| `spark.sql.legacy.timeParserPolicy` | `CORRECTED` | +| Config key | Comet-expected default | +| ----------------------------------- | ---------------------- | +| `spark.sql.legacy.timeParserPolicy` | `CORRECTED` | **Parquet reader/writer semantics** -| Config key | Comet-expected default | -| --- | --- | -| `spark.sql.legacy.parquet.datetimeRebaseModeInRead` | `CORRECTED` | -| `spark.sql.legacy.parquet.datetimeRebaseModeInWrite` | `CORRECTED` | -| `spark.sql.legacy.parquet.int96RebaseModeInRead` | `CORRECTED` | -| `spark.sql.legacy.parquet.int96RebaseModeInWrite` | `CORRECTED` | -| `spark.sql.legacy.parquet.nanosAsLong` | `false` | +| Config key | Comet-expected default | +| ---------------------------------------------------- | ---------------------- | +| `spark.sql.legacy.parquet.datetimeRebaseModeInRead` | `CORRECTED` | +| `spark.sql.legacy.parquet.datetimeRebaseModeInWrite` | `CORRECTED` | +| `spark.sql.legacy.parquet.int96RebaseModeInRead` | `CORRECTED` | +| `spark.sql.legacy.parquet.int96RebaseModeInWrite` | `CORRECTED` | +| `spark.sql.legacy.parquet.nanosAsLong` | `false` | **Cached-plan behavior on file-source scans** -| Config key | Comet-expected default | -| --- | --- | -| `spark.sql.legacy.readFileSourceTableCacheIgnoreOptions` | `false` | +| Config key | Comet-expected default | +| -------------------------------------------------------- | ---------------------- | +| `spark.sql.legacy.readFileSourceTableCacheIgnoreOptions` | `false` | ### Opting out of the session-wide fallback From fa489630932c86e0212069198e098a0b55d5c173 Mon Sep 17 00:00:00 2001 From: comphead Date: Mon, 6 Jul 2026 10:13:45 -0700 Subject: [PATCH 07/34] chore: fallback to Spark if legacy sql configurations are set --- .../src/main/scala/org/apache/comet/serde/predicates.scala | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/spark/src/main/scala/org/apache/comet/serde/predicates.scala b/spark/src/main/scala/org/apache/comet/serde/predicates.scala index 4d26975f7d..79e4a0c64b 100644 --- a/spark/src/main/scala/org/apache/comet/serde/predicates.scala +++ b/spark/src/main/scala/org/apache/comet/serde/predicates.scala @@ -243,7 +243,8 @@ object CometIsNaN extends CometExpressionSerde[IsNaN] { object CometIn extends CometExpressionSerde[In] with CodegenDispatchFallback { - override def getIncompatibleReasons(): Seq[String] = Seq(LegacyConfHelpers.nullInEmptyListReason) + override def getIncompatibleReasons(): Seq[String] = Seq( + LegacyConfHelpers.nullInEmptyListReason) override def getSupportLevel(expr: In): SupportLevel = if (expr.list.isEmpty && LegacyConfHelpers.legacyNullInEmptyBehavior) { @@ -265,7 +266,8 @@ object CometIn extends CometExpressionSerde[In] with CodegenDispatchFallback { object CometInSet extends CometExpressionSerde[InSet] with CodegenDispatchFallback { - override def getIncompatibleReasons(): Seq[String] = Seq(LegacyConfHelpers.nullInEmptyListReason) + override def getIncompatibleReasons(): Seq[String] = Seq( + LegacyConfHelpers.nullInEmptyListReason) override def getSupportLevel(expr: InSet): SupportLevel = if (expr.hset.isEmpty && LegacyConfHelpers.legacyNullInEmptyBehavior) { From 97a89b36de0b0a8b3dcaf925ec6ccde6b310d636 Mon Sep 17 00:00:00 2001 From: comphead Date: Mon, 6 Jul 2026 12:51:13 -0700 Subject: [PATCH 08/34] chore: fallback to Spark if legacy sql configurations are set --- docs/source/user-guide/latest/compatibility/index.md | 1 + .../main/scala/org/apache/comet/LegacyConfFallback.scala | 3 +++ .../expressions/aggregate/count_parameterless_legacy.sql | 6 +++--- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/docs/source/user-guide/latest/compatibility/index.md b/docs/source/user-guide/latest/compatibility/index.md index 4d09948fc0..ffdf94f54e 100644 --- a/docs/source/user-guide/latest/compatibility/index.md +++ b/docs/source/user-guide/latest/compatibility/index.md @@ -159,6 +159,7 @@ resolves to under its own defaults. | Config key | Comet-expected default | | ------------------------------------------------- | ---------------------- | +| `spark.sql.legacy.allowParameterlessCount` | `false` | | `spark.sql.legacy.duplicateBetweenInput` | `false` | | `spark.sql.legacy.inSubqueryNullability` | `false` | | `spark.sql.legacy.scalarSubqueryCountBugBehavior` | `false` | diff --git a/spark/src/main/scala/org/apache/comet/LegacyConfFallback.scala b/spark/src/main/scala/org/apache/comet/LegacyConfFallback.scala index 1e92213179..a8c3962311 100644 --- a/spark/src/main/scala/org/apache/comet/LegacyConfFallback.scala +++ b/spark/src/main/scala/org/apache/comet/LegacyConfFallback.scala @@ -48,6 +48,9 @@ private[comet] object LegacyConfFallback { "spark.sql.legacy.literal.pickMinimumPrecision" -> "false", // Char/varchar padding + write-side validation inserted by the analyzer. "spark.sql.legacy.charVarcharAsString" -> "false", + // Analyzer rule that lets `count()` be treated as `count(*)`; the native planner has no + // representation for a Count with zero children. + "spark.sql.legacy.allowParameterlessCount" -> "false", // Type-coercion / upcast rules. "spark.sql.legacy.doLooseUpcast" -> "false", "spark.sql.legacy.typeCoercion.datetimeToString.enabled" -> "false", diff --git a/spark/src/test/resources/sql-tests/expressions/aggregate/count_parameterless_legacy.sql b/spark/src/test/resources/sql-tests/expressions/aggregate/count_parameterless_legacy.sql index b47476b0f5..018e190754 100644 --- a/spark/src/test/resources/sql-tests/expressions/aggregate/count_parameterless_legacy.sql +++ b/spark/src/test/resources/sql-tests/expressions/aggregate/count_parameterless_legacy.sql @@ -30,16 +30,16 @@ statement INSERT INTO test_count_parameterless VALUES (1, 'x'), (2, 'x'), (NULL, 'y'), (3, 'y'), (NULL, 'y') -- Parameterless count() falls back to Spark; the aggregate result must still match Spark. -query expect_fallback(spark.sql.legacy.allowParameterlessCount=true) +query spark_answer_only SELECT count() FROM test_count_parameterless -- Parameterless count() with GROUP BY. -query expect_fallback(spark.sql.legacy.allowParameterlessCount=true) +query spark_answer_only SELECT grp, count() FROM test_count_parameterless GROUP BY grp ORDER BY grp -- Parameterless count() on empty table. statement CREATE TABLE test_count_empty(i int) USING parquet -query expect_fallback(spark.sql.legacy.allowParameterlessCount=true) +query spark_answer_only SELECT count() FROM test_count_empty From 964a75d3d0314fdc581008647fae33386f4563be Mon Sep 17 00:00:00 2001 From: comphead Date: Mon, 6 Jul 2026 13:30:41 -0700 Subject: [PATCH 09/34] chore: fallback to Spark if legacy sql configurations are set --- .../org/apache/comet/LegacyConfFallback.scala | 44 +++------- .../org/apache/comet/serde/aggregates.scala | 21 ----- .../comet/shims/ShimLegacyConfFallback.scala | 54 +++++++++++++ .../comet/shims/ShimLegacyConfFallback.scala | 80 +++++++++++++++++++ .../aggregate/count_parameterless_legacy.sql | 45 ----------- 5 files changed, 143 insertions(+), 101 deletions(-) create mode 100644 spark/src/main/spark-3.x/org/apache/comet/shims/ShimLegacyConfFallback.scala create mode 100644 spark/src/main/spark-4.x/org/apache/comet/shims/ShimLegacyConfFallback.scala delete mode 100644 spark/src/test/resources/sql-tests/expressions/aggregate/count_parameterless_legacy.sql diff --git a/spark/src/main/scala/org/apache/comet/LegacyConfFallback.scala b/spark/src/main/scala/org/apache/comet/LegacyConfFallback.scala index a8c3962311..9c2788c271 100644 --- a/spark/src/main/scala/org/apache/comet/LegacyConfFallback.scala +++ b/spark/src/main/scala/org/apache/comet/LegacyConfFallback.scala @@ -21,6 +21,8 @@ package org.apache.comet import org.apache.spark.sql.internal.SQLConf +import org.apache.comet.shims.ShimLegacyConfFallback + /** * Curated set of Spark `spark.sql.legacy.*` configs whose behavior is NOT tied to a specific * Comet-supported expression (built-in functions with a legacy dependency are handled @@ -33,48 +35,20 @@ import org.apache.spark.sql.internal.SQLConf * for the session if any of these keys is set to its non-default value, so Spark's own execution * path is used instead. Users can set `spark.comet.legacyConfFallback.enabled=false` to override * the fallback and keep Comet enabled (Spark compatibility is not guaranteed in that case). + * + * The map of legacy key -> Spark 4 default value comes from [[ShimLegacyConfFallback]]. The 4.x + * shim derives defaults from live [[org.apache.spark.internal.config.ConfigEntry]] references so + * additions/removals in Spark 4 are picked up automatically; the 3.x shim hardcodes the same + * defaults because several of these keys do not exist as ConfigEntry instances in Spark 3. */ -private[comet] object LegacyConfFallback { +private[comet] object LegacyConfFallback extends ShimLegacyConfFallback { /** * Map of legacy config key -> case-insensitive Spark default value. A config triggers the * fallback when it is present in the session conf AND its value is not equal (case-insensitive) * to the default recorded here. */ - val executionAffectingDefaults: Map[String, String] = Map( - // Decimal type-system / analyzer rules that reshape plans reaching Comet. - "spark.sql.legacy.allowNegativeScaleOfDecimal" -> "false", - "spark.sql.legacy.decimal.retainFractionDigitsOnTruncate" -> "false", - "spark.sql.legacy.literal.pickMinimumPrecision" -> "false", - // Char/varchar padding + write-side validation inserted by the analyzer. - "spark.sql.legacy.charVarcharAsString" -> "false", - // Analyzer rule that lets `count()` be treated as `count(*)`; the native planner has no - // representation for a Count with zero children. - "spark.sql.legacy.allowParameterlessCount" -> "false", - // Type-coercion / upcast rules. - "spark.sql.legacy.doLooseUpcast" -> "false", - "spark.sql.legacy.typeCoercion.datetimeToString.enabled" -> "false", - // Optimizer rules that reshape plans (subqueries, Between, empty-list IN nullability). - "spark.sql.legacy.duplicateBetweenInput" -> "false", - "spark.sql.legacy.inSubqueryNullability" -> "false", - "spark.sql.legacy.scalarSubqueryCountBugBehavior" -> "false", - // Map-key normalization used by CreateMap and friends inside ArrayBasedMapBuilder. - "spark.sql.legacy.disableMapKeyNormalization" -> "false", - // Set-op precedence changes the plan topology handed to Comet operators. - "spark.sql.legacy.setopsPrecedence.enabled" -> "false", - // View schema compensation controls whether Cast (Comet-supported) or UpCast (Comet - // unsupported) is injected during view resolution. - "spark.sql.legacy.viewSchemaCompensation" -> "true", - // Datetime parser policy affects CSV/JSON scan options and datetime formatters. - "spark.sql.legacy.timeParserPolicy" -> "CORRECTED", - // Datasource readers/writers Comet may accelerate. - "spark.sql.legacy.parquet.datetimeRebaseModeInRead" -> "CORRECTED", - "spark.sql.legacy.parquet.datetimeRebaseModeInWrite" -> "CORRECTED", - "spark.sql.legacy.parquet.int96RebaseModeInRead" -> "CORRECTED", - "spark.sql.legacy.parquet.int96RebaseModeInWrite" -> "CORRECTED", - "spark.sql.legacy.parquet.nanosAsLong" -> "false", - // Cached-plan behavior that leaves stale options on a Comet-accelerated file scan. - "spark.sql.legacy.readFileSourceTableCacheIgnoreOptions" -> "false") + val executionAffectingDefaults: Map[String, String] = legacyConfDefaults /** Keys in [[executionAffectingDefaults]] that are set to a non-default value on `conf`. */ def triggeredConfigs(conf: SQLConf): Iterable[String] = { diff --git a/spark/src/main/scala/org/apache/comet/serde/aggregates.scala b/spark/src/main/scala/org/apache/comet/serde/aggregates.scala index 1bb932c528..7381c65acb 100644 --- a/spark/src/main/scala/org/apache/comet/serde/aggregates.scala +++ b/spark/src/main/scala/org/apache/comet/serde/aggregates.scala @@ -109,27 +109,6 @@ object CometMax extends CometAggregateExpressionSerde[Max] { object CometCount extends CometAggregateExpressionSerde[Count] { - // When `spark.sql.legacy.allowParameterlessCount=true`, Spark allows `count()` with no - // arguments and treats it as `count(*)`. Comet's native planner asserts on non-empty children - // and would panic on such an expression, so mark it Unsupported here and let the aggregate fall - // back to Spark. Aggregate serdes have no [[CodegenDispatchFallback]] path (aggregates cannot - // be routed through the JVM codegen dispatcher), so a clean Spark fallback is the appropriate - // outcome. Under the default config value, Spark's analyzer rejects parameterless `count()` so - // this branch is unreachable. - private val legacyAllowParameterlessCountReason: String = - "`spark.sql.legacy.allowParameterlessCount=true` produces `count()` with no children, which " + - "the native planner does not support" - - override def getUnsupportedReasons(): Seq[String] = Seq(legacyAllowParameterlessCountReason) - - override def getSupportLevel(expr: Count): SupportLevel = { - if (expr.children.isEmpty) { - Unsupported(Some(legacyAllowParameterlessCountReason)) - } else { - Compatible() - } - } - override def convert( aggExpr: AggregateExpression, expr: Count, diff --git a/spark/src/main/spark-3.x/org/apache/comet/shims/ShimLegacyConfFallback.scala b/spark/src/main/spark-3.x/org/apache/comet/shims/ShimLegacyConfFallback.scala new file mode 100644 index 0000000000..f67c53294b --- /dev/null +++ b/spark/src/main/spark-3.x/org/apache/comet/shims/ShimLegacyConfFallback.scala @@ -0,0 +1,54 @@ +/* + * 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.shims + +/** + * Spark 3.x variant: several entries in this map (view-schema compensation, decimal truncate, + * duplicate-between, scalar-subquery count-bug, disable-map-key-normalization, cache-ignore- + * options) do not exist as [[org.apache.spark.internal.config.ConfigEntry]] instances in Spark + * 3.5 or earlier, so the defaults are hardcoded. They match the Spark 4 defaults on purpose: + * the fallback rule is "when a legacy key is set to something other than the Spark 4 default, + * disable Comet". This keeps 3.x and 4.x behaviourally aligned even though 3.x can't derive + * the defaults live. See the 4.x shim for the reference-derived variant. + */ +trait ShimLegacyConfFallback { + + protected def legacyConfDefaults: Map[String, String] = Map( + "spark.sql.legacy.allowNegativeScaleOfDecimal" -> "false", + "spark.sql.legacy.decimal.retainFractionDigitsOnTruncate" -> "false", + "spark.sql.legacy.literal.pickMinimumPrecision" -> "true", + "spark.sql.legacy.charVarcharAsString" -> "false", + "spark.sql.legacy.allowParameterlessCount" -> "false", + "spark.sql.legacy.doLooseUpcast" -> "false", + "spark.sql.legacy.typeCoercion.datetimeToString.enabled" -> "false", + "spark.sql.legacy.duplicateBetweenInput" -> "false", + "spark.sql.legacy.inSubqueryNullability" -> "false", + "spark.sql.legacy.scalarSubqueryCountBugBehavior" -> "false", + "spark.sql.legacy.disableMapKeyNormalization" -> "false", + "spark.sql.legacy.setopsPrecedence.enabled" -> "false", + "spark.sql.legacy.viewSchemaCompensation" -> "true", + "spark.sql.legacy.timeParserPolicy" -> "CORRECTED", + "spark.sql.legacy.parquet.datetimeRebaseModeInRead" -> "CORRECTED", + "spark.sql.legacy.parquet.datetimeRebaseModeInWrite" -> "CORRECTED", + "spark.sql.legacy.parquet.int96RebaseModeInRead" -> "CORRECTED", + "spark.sql.legacy.parquet.int96RebaseModeInWrite" -> "CORRECTED", + "spark.sql.legacy.parquet.nanosAsLong" -> "false", + "spark.sql.legacy.readFileSourceTableCacheIgnoreOptions" -> "false") +} diff --git a/spark/src/main/spark-4.x/org/apache/comet/shims/ShimLegacyConfFallback.scala b/spark/src/main/spark-4.x/org/apache/comet/shims/ShimLegacyConfFallback.scala new file mode 100644 index 0000000000..ec74b94ac3 --- /dev/null +++ b/spark/src/main/spark-4.x/org/apache/comet/shims/ShimLegacyConfFallback.scala @@ -0,0 +1,80 @@ +/* + * 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.shims + +import org.apache.spark.internal.config.ConfigEntry +import org.apache.spark.sql.internal.SQLConf + +/** + * Spark 4.x variant: defaults are read from live `ConfigEntry` references so this file stays in + * sync with Spark 4 without duplicated string literals. See [[ShimLegacyConfFallback]] on 3.x for + * the hardcoded counterpart. + * + * Every key returned here uses the `spark.sql.legacy.*` name that the check compares against. + * When a legacy key was removed in Spark 4 (the four parquet rebase aliases), we still ship the + * legacy key -> Spark 4 non-legacy default; on Spark 4 the check is effectively inert because + * `SQLConf.set` rejects removed keys, but the entry is kept so 3.x behaviour is consistent. + */ +trait ShimLegacyConfFallback { + + private def entryDefault(entry: ConfigEntry[_]): String = entry.defaultValueString + + protected def legacyConfDefaults: Map[String, String] = Map( + "spark.sql.legacy.allowNegativeScaleOfDecimal" -> + entryDefault(SQLConf.LEGACY_ALLOW_NEGATIVE_SCALE_OF_DECIMAL_ENABLED), + "spark.sql.legacy.decimal.retainFractionDigitsOnTruncate" -> + entryDefault(SQLConf.LEGACY_RETAIN_FRACTION_DIGITS_FIRST), + "spark.sql.legacy.literal.pickMinimumPrecision" -> + entryDefault(SQLConf.LITERAL_PICK_MINIMUM_PRECISION), + "spark.sql.legacy.charVarcharAsString" -> + entryDefault(SQLConf.LEGACY_CHAR_VARCHAR_AS_STRING), + "spark.sql.legacy.allowParameterlessCount" -> + entryDefault(SQLConf.ALLOW_PARAMETERLESS_COUNT), + "spark.sql.legacy.doLooseUpcast" -> + entryDefault(SQLConf.LEGACY_LOOSE_UPCAST), + "spark.sql.legacy.typeCoercion.datetimeToString.enabled" -> + entryDefault(SQLConf.LEGACY_CAST_DATETIME_TO_STRING), + "spark.sql.legacy.duplicateBetweenInput" -> + entryDefault(SQLConf.LEGACY_DUPLICATE_BETWEEN_INPUT), + "spark.sql.legacy.inSubqueryNullability" -> + entryDefault(SQLConf.LEGACY_IN_SUBQUERY_NULLABILITY), + "spark.sql.legacy.scalarSubqueryCountBugBehavior" -> + entryDefault(SQLConf.LEGACY_SCALAR_SUBQUERY_COUNT_BUG_HANDLING), + "spark.sql.legacy.disableMapKeyNormalization" -> + entryDefault(SQLConf.DISABLE_MAP_KEY_NORMALIZATION), + "spark.sql.legacy.setopsPrecedence.enabled" -> + entryDefault(SQLConf.LEGACY_SETOPS_PRECEDENCE_ENABLED), + "spark.sql.legacy.viewSchemaCompensation" -> + entryDefault(SQLConf.VIEW_SCHEMA_COMPENSATION), + "spark.sql.legacy.timeParserPolicy" -> + entryDefault(SQLConf.LEGACY_TIME_PARSER_POLICY), + "spark.sql.legacy.parquet.datetimeRebaseModeInRead" -> + entryDefault(SQLConf.PARQUET_REBASE_MODE_IN_READ), + "spark.sql.legacy.parquet.datetimeRebaseModeInWrite" -> + entryDefault(SQLConf.PARQUET_REBASE_MODE_IN_WRITE), + "spark.sql.legacy.parquet.int96RebaseModeInRead" -> + entryDefault(SQLConf.PARQUET_INT96_REBASE_MODE_IN_READ), + "spark.sql.legacy.parquet.int96RebaseModeInWrite" -> + entryDefault(SQLConf.PARQUET_INT96_REBASE_MODE_IN_WRITE), + "spark.sql.legacy.parquet.nanosAsLong" -> + entryDefault(SQLConf.LEGACY_PARQUET_NANOS_AS_LONG), + "spark.sql.legacy.readFileSourceTableCacheIgnoreOptions" -> + entryDefault(SQLConf.READ_FILE_SOURCE_TABLE_CACHE_IGNORE_OPTIONS)) +} diff --git a/spark/src/test/resources/sql-tests/expressions/aggregate/count_parameterless_legacy.sql b/spark/src/test/resources/sql-tests/expressions/aggregate/count_parameterless_legacy.sql deleted file mode 100644 index 018e190754..0000000000 --- a/spark/src/test/resources/sql-tests/expressions/aggregate/count_parameterless_legacy.sql +++ /dev/null @@ -1,45 +0,0 @@ --- 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. - --- When `spark.sql.legacy.allowParameterlessCount=true`, Spark accepts `count()` (no arguments) --- and treats it as `count(*)`. Comet's native planner asserts non-empty children on Count, so --- `CometCount.getSupportLevel` marks parameterless Count `Unsupported` and lets the aggregate --- fall back to Spark. Aggregate serdes do not have a JVM codegen dispatcher path, so the --- Spark fallback is the correct outcome. - --- Config: spark.sql.legacy.allowParameterlessCount=true - -statement -CREATE TABLE test_count_parameterless(i int, grp string) USING parquet - -statement -INSERT INTO test_count_parameterless VALUES (1, 'x'), (2, 'x'), (NULL, 'y'), (3, 'y'), (NULL, 'y') - --- Parameterless count() falls back to Spark; the aggregate result must still match Spark. -query spark_answer_only -SELECT count() FROM test_count_parameterless - --- Parameterless count() with GROUP BY. -query spark_answer_only -SELECT grp, count() FROM test_count_parameterless GROUP BY grp ORDER BY grp - --- Parameterless count() on empty table. -statement -CREATE TABLE test_count_empty(i int) USING parquet - -query spark_answer_only -SELECT count() FROM test_count_empty From db91685e93bccce12e0155c831022b8c2606c736 Mon Sep 17 00:00:00 2001 From: comphead Date: Mon, 6 Jul 2026 13:41:25 -0700 Subject: [PATCH 10/34] chore: fallback to Spark if legacy sql configurations are set --- .../org/apache/comet/shims/ShimLegacyConfFallback.scala | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spark/src/main/spark-3.x/org/apache/comet/shims/ShimLegacyConfFallback.scala b/spark/src/main/spark-3.x/org/apache/comet/shims/ShimLegacyConfFallback.scala index f67c53294b..5cc466389b 100644 --- a/spark/src/main/spark-3.x/org/apache/comet/shims/ShimLegacyConfFallback.scala +++ b/spark/src/main/spark-3.x/org/apache/comet/shims/ShimLegacyConfFallback.scala @@ -23,10 +23,10 @@ package org.apache.comet.shims * Spark 3.x variant: several entries in this map (view-schema compensation, decimal truncate, * duplicate-between, scalar-subquery count-bug, disable-map-key-normalization, cache-ignore- * options) do not exist as [[org.apache.spark.internal.config.ConfigEntry]] instances in Spark - * 3.5 or earlier, so the defaults are hardcoded. They match the Spark 4 defaults on purpose: - * the fallback rule is "when a legacy key is set to something other than the Spark 4 default, - * disable Comet". This keeps 3.x and 4.x behaviourally aligned even though 3.x can't derive - * the defaults live. See the 4.x shim for the reference-derived variant. + * 3.5 or earlier, so the defaults are hardcoded. They match the Spark 4 defaults on purpose: the + * fallback rule is "when a legacy key is set to something other than the Spark 4 default, disable + * Comet". This keeps 3.x and 4.x behaviourally aligned even though 3.x can't derive the defaults + * live. See the 4.x shim for the reference-derived variant. */ trait ShimLegacyConfFallback { From 4b2cde58ec0f2443bff98e581f4c3724d96f4a15 Mon Sep 17 00:00:00 2001 From: comphead Date: Mon, 6 Jul 2026 15:24:44 -0700 Subject: [PATCH 11/34] chore: fallback to Spark if legacy sql configurations are set --- .../comet/shims/ShimLegacyConfFallback.scala | 47 ++++++++++--------- 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/spark/src/main/spark-4.x/org/apache/comet/shims/ShimLegacyConfFallback.scala b/spark/src/main/spark-4.x/org/apache/comet/shims/ShimLegacyConfFallback.scala index ec74b94ac3..6fbf91d070 100644 --- a/spark/src/main/spark-4.x/org/apache/comet/shims/ShimLegacyConfFallback.scala +++ b/spark/src/main/spark-4.x/org/apache/comet/shims/ShimLegacyConfFallback.scala @@ -19,7 +19,6 @@ package org.apache.comet.shims -import org.apache.spark.internal.config.ConfigEntry import org.apache.spark.sql.internal.SQLConf /** @@ -31,50 +30,52 @@ import org.apache.spark.sql.internal.SQLConf * When a legacy key was removed in Spark 4 (the four parquet rebase aliases), we still ship the * legacy key -> Spark 4 non-legacy default; on Spark 4 the check is effectively inert because * `SQLConf.set` rejects removed keys, but the entry is kept so 3.x behaviour is consistent. + * + * Note: `ConfigEntry` itself is `private[spark]`, so we never spell the type — every value below + * is a direct method call on a `SQLConf` val, and the compiler resolves `defaultValueString` + * without exposing the type name to this compilation unit. */ trait ShimLegacyConfFallback { - private def entryDefault(entry: ConfigEntry[_]): String = entry.defaultValueString - protected def legacyConfDefaults: Map[String, String] = Map( "spark.sql.legacy.allowNegativeScaleOfDecimal" -> - entryDefault(SQLConf.LEGACY_ALLOW_NEGATIVE_SCALE_OF_DECIMAL_ENABLED), + SQLConf.LEGACY_ALLOW_NEGATIVE_SCALE_OF_DECIMAL_ENABLED.defaultValueString, "spark.sql.legacy.decimal.retainFractionDigitsOnTruncate" -> - entryDefault(SQLConf.LEGACY_RETAIN_FRACTION_DIGITS_FIRST), + SQLConf.LEGACY_RETAIN_FRACTION_DIGITS_FIRST.defaultValueString, "spark.sql.legacy.literal.pickMinimumPrecision" -> - entryDefault(SQLConf.LITERAL_PICK_MINIMUM_PRECISION), + SQLConf.LITERAL_PICK_MINIMUM_PRECISION.defaultValueString, "spark.sql.legacy.charVarcharAsString" -> - entryDefault(SQLConf.LEGACY_CHAR_VARCHAR_AS_STRING), + SQLConf.LEGACY_CHAR_VARCHAR_AS_STRING.defaultValueString, "spark.sql.legacy.allowParameterlessCount" -> - entryDefault(SQLConf.ALLOW_PARAMETERLESS_COUNT), + SQLConf.ALLOW_PARAMETERLESS_COUNT.defaultValueString, "spark.sql.legacy.doLooseUpcast" -> - entryDefault(SQLConf.LEGACY_LOOSE_UPCAST), + SQLConf.LEGACY_LOOSE_UPCAST.defaultValueString, "spark.sql.legacy.typeCoercion.datetimeToString.enabled" -> - entryDefault(SQLConf.LEGACY_CAST_DATETIME_TO_STRING), + SQLConf.LEGACY_CAST_DATETIME_TO_STRING.defaultValueString, "spark.sql.legacy.duplicateBetweenInput" -> - entryDefault(SQLConf.LEGACY_DUPLICATE_BETWEEN_INPUT), + SQLConf.LEGACY_DUPLICATE_BETWEEN_INPUT.defaultValueString, "spark.sql.legacy.inSubqueryNullability" -> - entryDefault(SQLConf.LEGACY_IN_SUBQUERY_NULLABILITY), + SQLConf.LEGACY_IN_SUBQUERY_NULLABILITY.defaultValueString, "spark.sql.legacy.scalarSubqueryCountBugBehavior" -> - entryDefault(SQLConf.LEGACY_SCALAR_SUBQUERY_COUNT_BUG_HANDLING), + SQLConf.LEGACY_SCALAR_SUBQUERY_COUNT_BUG_HANDLING.defaultValueString, "spark.sql.legacy.disableMapKeyNormalization" -> - entryDefault(SQLConf.DISABLE_MAP_KEY_NORMALIZATION), + SQLConf.DISABLE_MAP_KEY_NORMALIZATION.defaultValueString, "spark.sql.legacy.setopsPrecedence.enabled" -> - entryDefault(SQLConf.LEGACY_SETOPS_PRECEDENCE_ENABLED), + SQLConf.LEGACY_SETOPS_PRECEDENCE_ENABLED.defaultValueString, "spark.sql.legacy.viewSchemaCompensation" -> - entryDefault(SQLConf.VIEW_SCHEMA_COMPENSATION), + SQLConf.VIEW_SCHEMA_COMPENSATION.defaultValueString, "spark.sql.legacy.timeParserPolicy" -> - entryDefault(SQLConf.LEGACY_TIME_PARSER_POLICY), + SQLConf.LEGACY_TIME_PARSER_POLICY.defaultValueString, "spark.sql.legacy.parquet.datetimeRebaseModeInRead" -> - entryDefault(SQLConf.PARQUET_REBASE_MODE_IN_READ), + SQLConf.PARQUET_REBASE_MODE_IN_READ.defaultValueString, "spark.sql.legacy.parquet.datetimeRebaseModeInWrite" -> - entryDefault(SQLConf.PARQUET_REBASE_MODE_IN_WRITE), + SQLConf.PARQUET_REBASE_MODE_IN_WRITE.defaultValueString, "spark.sql.legacy.parquet.int96RebaseModeInRead" -> - entryDefault(SQLConf.PARQUET_INT96_REBASE_MODE_IN_READ), + SQLConf.PARQUET_INT96_REBASE_MODE_IN_READ.defaultValueString, "spark.sql.legacy.parquet.int96RebaseModeInWrite" -> - entryDefault(SQLConf.PARQUET_INT96_REBASE_MODE_IN_WRITE), + SQLConf.PARQUET_INT96_REBASE_MODE_IN_WRITE.defaultValueString, "spark.sql.legacy.parquet.nanosAsLong" -> - entryDefault(SQLConf.LEGACY_PARQUET_NANOS_AS_LONG), + SQLConf.LEGACY_PARQUET_NANOS_AS_LONG.defaultValueString, "spark.sql.legacy.readFileSourceTableCacheIgnoreOptions" -> - entryDefault(SQLConf.READ_FILE_SOURCE_TABLE_CACHE_IGNORE_OPTIONS)) + SQLConf.READ_FILE_SOURCE_TABLE_CACHE_IGNORE_OPTIONS.defaultValueString) } From d348f3f678b627ea25a9a78542e887aec5786bc0 Mon Sep 17 00:00:00 2001 From: comphead Date: Tue, 7 Jul 2026 08:01:03 -0700 Subject: [PATCH 12/34] chore: fallback to Spark if legacy sql configurations are set --- .../user-guide/latest/compatibility/index.md | 6 +++++- .../comet/shims/ShimLegacyConfFallback.scala | 1 - .../comet/shims/ShimLegacyConfFallback.scala | 2 -- .../scala/org/apache/comet/CometCastSuite.scala | 15 +-------------- .../comet/CometSparkSessionExtensionsSuite.scala | 14 +++++++++----- 5 files changed, 15 insertions(+), 23 deletions(-) diff --git a/docs/source/user-guide/latest/compatibility/index.md b/docs/source/user-guide/latest/compatibility/index.md index ffdf94f54e..ca028167f0 100644 --- a/docs/source/user-guide/latest/compatibility/index.md +++ b/docs/source/user-guide/latest/compatibility/index.md @@ -138,10 +138,14 @@ resolves to under its own defaults. | Config key | Comet-expected default | | --------------------------------------------------------- | ---------------------- | -| `spark.sql.legacy.allowNegativeScaleOfDecimal` | `false` | | `spark.sql.legacy.decimal.retainFractionDigitsOnTruncate` | `false` | | `spark.sql.legacy.literal.pickMinimumPrecision` | `true` | +Note: `spark.sql.legacy.allowNegativeScaleOfDecimal` is intentionally NOT in this list. +Negative-scale decimals are handled per-expression — `CometCast.isSupported` returns +`Incompatible` when the flag is `false` and `Compatible` when the user opts in — so enabling +the legacy flag does not disable Comet for the whole session. + **Char/varchar padding and analyzer-inserted write-side validation** | Config key | Comet-expected default | diff --git a/spark/src/main/spark-3.x/org/apache/comet/shims/ShimLegacyConfFallback.scala b/spark/src/main/spark-3.x/org/apache/comet/shims/ShimLegacyConfFallback.scala index 5cc466389b..c675f8405f 100644 --- a/spark/src/main/spark-3.x/org/apache/comet/shims/ShimLegacyConfFallback.scala +++ b/spark/src/main/spark-3.x/org/apache/comet/shims/ShimLegacyConfFallback.scala @@ -31,7 +31,6 @@ package org.apache.comet.shims trait ShimLegacyConfFallback { protected def legacyConfDefaults: Map[String, String] = Map( - "spark.sql.legacy.allowNegativeScaleOfDecimal" -> "false", "spark.sql.legacy.decimal.retainFractionDigitsOnTruncate" -> "false", "spark.sql.legacy.literal.pickMinimumPrecision" -> "true", "spark.sql.legacy.charVarcharAsString" -> "false", diff --git a/spark/src/main/spark-4.x/org/apache/comet/shims/ShimLegacyConfFallback.scala b/spark/src/main/spark-4.x/org/apache/comet/shims/ShimLegacyConfFallback.scala index 6fbf91d070..25a49cd11f 100644 --- a/spark/src/main/spark-4.x/org/apache/comet/shims/ShimLegacyConfFallback.scala +++ b/spark/src/main/spark-4.x/org/apache/comet/shims/ShimLegacyConfFallback.scala @@ -38,8 +38,6 @@ import org.apache.spark.sql.internal.SQLConf trait ShimLegacyConfFallback { protected def legacyConfDefaults: Map[String, String] = Map( - "spark.sql.legacy.allowNegativeScaleOfDecimal" -> - SQLConf.LEGACY_ALLOW_NEGATIVE_SCALE_OF_DECIMAL_ENABLED.defaultValueString, "spark.sql.legacy.decimal.retainFractionDigitsOnTruncate" -> SQLConf.LEGACY_RETAIN_FRACTION_DIGITS_FIRST.defaultValueString, "spark.sql.legacy.literal.pickMinimumPrecision" -> diff --git a/spark/src/test/scala/org/apache/comet/CometCastSuite.scala b/spark/src/test/scala/org/apache/comet/CometCastSuite.scala index 99475a252f..0f46499025 100644 --- a/spark/src/test/scala/org/apache/comet/CometCastSuite.scala +++ b/spark/src/test/scala/org/apache/comet/CometCastSuite.scala @@ -735,21 +735,8 @@ class CometCastSuite extends CometTestBase with AdaptiveSparkPlanHelper { } test("cast DecimalType with negative scale to StringType") { - // Negative-scale decimals are a legacy Spark feature gated on - // spark.sql.legacy.allowNegativeScaleOfDecimal=true. Spark LEGACY cast uses Java's - // BigDecimal.toString() which produces scientific notation for negative-scale values - // (e.g. 12300 stored as Decimal(7,-2) with unscaled=123 → "1.23E+4"). - // CometCast.canCastToString checks the - // config and returns Incompatible when it is false. - // - // Parquet does not support negative-scale decimals so we use checkSparkAnswer directly - // (no parquet round-trip) to avoid schema coercion. - - // With config enabled, enable localTableScan so Comet can take over the full plan - // and execute the cast natively. Parquet does not support negative-scale decimals so - // the data is kept in-memory; localTableScan.enabled bridges that gap. withSQLConf( - "spark.sql.legacy.allowNegativeScaleOfDecimal" -> "true", + "spark.sql.legacy.allowNegativeScaleOfDecimal" -> "false", "spark.comet.exec.localTableScan.enabled" -> "true") { val dfNeg2 = Seq( Some(BigDecimal("0")), diff --git a/spark/src/test/scala/org/apache/comet/CometSparkSessionExtensionsSuite.scala b/spark/src/test/scala/org/apache/comet/CometSparkSessionExtensionsSuite.scala index e3358c6f77..e65b86569f 100644 --- a/spark/src/test/scala/org/apache/comet/CometSparkSessionExtensionsSuite.scala +++ b/spark/src/test/scala/org/apache/comet/CometSparkSessionExtensionsSuite.scala @@ -62,11 +62,11 @@ class CometSparkSessionExtensionsSuite extends CometTestBase { assert(isCometLoaded(conf)) // A single boolean-false-default execution-affecting legacy config triggers the fallback. - conf.setConfString("spark.sql.legacy.allowNegativeScaleOfDecimal", "true") + conf.setConfString("spark.sql.legacy.charVarcharAsString", "true") assert(!isCometLoaded(conf)) // Setting the config back to its Spark default (case-insensitive) clears the trigger. - conf.setConfString("spark.sql.legacy.allowNegativeScaleOfDecimal", "FALSE") + conf.setConfString("spark.sql.legacy.charVarcharAsString", "FALSE") assert(isCometLoaded(conf)) // Enum-default configs also trigger when set to a non-default value. @@ -75,15 +75,19 @@ class CometSparkSessionExtensionsSuite extends CometTestBase { conf.setConfString("spark.sql.legacy.timeParserPolicy", "CORRECTED") assert(isCometLoaded(conf)) - // Legacy configs handled per-expression (e.g. castComplexTypesToString) are NOT part of the - // fallback set and must not disable Comet on their own. + // Legacy configs handled per-expression (e.g. castComplexTypesToString, + // allowNegativeScaleOfDecimal) are NOT part of the fallback set and must not disable Comet + // on their own. conf.setConfString("spark.sql.legacy.castComplexTypesToString.enabled", "true") assert(isCometLoaded(conf)) conf.unsetConf("spark.sql.legacy.castComplexTypesToString.enabled") + conf.setConfString("spark.sql.legacy.allowNegativeScaleOfDecimal", "true") + assert(isCometLoaded(conf)) + conf.unsetConf("spark.sql.legacy.allowNegativeScaleOfDecimal") // Opt-out: users can keep Comet enabled by disabling the fallback (compatibility not // guaranteed). - conf.setConfString("spark.sql.legacy.allowNegativeScaleOfDecimal", "true") + conf.setConfString("spark.sql.legacy.charVarcharAsString", "true") conf.setConfString(CometConf.COMET_LEGACY_CONF_FALLBACK_ENABLED.key, "false") assert(isCometLoaded(conf)) } From c2a8d622ad5cbc0ef00f32b16ee789953b550c91 Mon Sep 17 00:00:00 2001 From: comphead Date: Tue, 7 Jul 2026 11:03:12 -0700 Subject: [PATCH 13/34] chore: fallback to Spark if legacy sql configurations are set --- spark/src/test/scala/org/apache/comet/CometCastSuite.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spark/src/test/scala/org/apache/comet/CometCastSuite.scala b/spark/src/test/scala/org/apache/comet/CometCastSuite.scala index 0f46499025..35b1fbbabe 100644 --- a/spark/src/test/scala/org/apache/comet/CometCastSuite.scala +++ b/spark/src/test/scala/org/apache/comet/CometCastSuite.scala @@ -736,7 +736,7 @@ class CometCastSuite extends CometTestBase with AdaptiveSparkPlanHelper { test("cast DecimalType with negative scale to StringType") { withSQLConf( - "spark.sql.legacy.allowNegativeScaleOfDecimal" -> "false", + "spark.sql.legacy.allowNegativeScaleOfDecimal" -> "true", "spark.comet.exec.localTableScan.enabled" -> "true") { val dfNeg2 = Seq( Some(BigDecimal("0")), From 2e1de6d1de10eb830f1579e560fbddcc3e1989c7 Mon Sep 17 00:00:00 2001 From: comphead Date: Thu, 9 Jul 2026 17:27:14 -0700 Subject: [PATCH 14/34] chore: fallback to Spark if legacy sql configurations are set --- .../org/apache/comet/expressions/CometCast.scala | 13 +++---------- .../cast/cast_complex_types_to_string.sql | 3 +-- .../org/apache/comet/CometExpressionSuite.scala | 7 +------ 3 files changed, 5 insertions(+), 18 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 6e9767228e..ef1cd2d753 100644 --- a/spark/src/main/scala/org/apache/comet/expressions/CometCast.scala +++ b/spark/src/main/scala/org/apache/comet/expressions/CometCast.scala @@ -25,6 +25,7 @@ import org.apache.spark.sql.types.{ArrayType, DataType, DataTypes, DecimalType, import org.apache.comet.CometConf import org.apache.comet.CometSparkSessionExtensions.{isSpark40Plus, withFallbackReason} +import org.apache.comet.DataTypeSupport.isComplexType import org.apache.comet.serde.{CodegenDispatchFallback, CometExpressionSerde, Compatible, ExprOuterClass, Incompatible, SupportLevel, Unsupported} import org.apache.comet.serde.ExprOuterClass.Expr import org.apache.comet.serde.QueryPlanSerde.{evalModeToProto, exprToProtoInternal, serializeDataType} @@ -39,13 +40,6 @@ object CometCast private[comet] val negativeScaleDecimalToStringReason: String = "Negative-scale decimal requires spark.sql.legacy.allowNegativeScaleOfDecimal=true" - // When `spark.sql.legacy.castComplexTypesToString.enabled` is true, Spark wraps maps and - // structs with `[]` (instead of `{}`) when casting to string, and omits NULL elements of - // structs/maps/arrays (instead of rendering them as the literal "null"). Comet's native cast - // only implements the default formatting, so when the flag is on we mark the cast Incompatible - // and let the [[CodegenDispatchFallback]] trait route it through the JVM codegen dispatcher - // (Spark's own `doGenCode` inside the Comet kernel) so results still match Spark exactly. The - // flag is internal in Spark 4.0 and defaults to false. private[comet] val legacyCastComplexTypesToStringReason: String = "spark.sql.legacy.castComplexTypesToString.enabled=true is not supported natively" @@ -168,9 +162,8 @@ object CometCast return Compatible() } - if (toType == DataTypes.StringType && legacyCastComplexTypesToString && (fromType - .isInstanceOf[ArrayType] || fromType.isInstanceOf[StructType] || - fromType.isInstanceOf[MapType])) { + if (toType == DataTypes.StringType && legacyCastComplexTypesToString && isComplexType( + fromType)) { return Incompatible(Some(legacyCastComplexTypesToStringReason)) } diff --git a/spark/src/test/resources/sql-tests/expressions/cast/cast_complex_types_to_string.sql b/spark/src/test/resources/sql-tests/expressions/cast/cast_complex_types_to_string.sql index 8b1d989ae7..09d3bb19ec 100644 --- a/spark/src/test/resources/sql-tests/expressions/cast/cast_complex_types_to_string.sql +++ b/spark/src/test/resources/sql-tests/expressions/cast/cast_complex_types_to_string.sql @@ -149,8 +149,7 @@ SELECT cast(named_struct('a', named_struct('b', named_struct('c', 1, 'd', 'leaf' query SELECT cast(named_struct('s1', '', 's2', ' ', 's3', cast(null as string)) as string) --- Map-valued field: not supported, falls back to Spark. -query expect_fallback(to StringType is not supported) +query SELECT cast(named_struct('m', map('k', 1)) as string) -- ---------------------------------------------------------------------------- diff --git a/spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala b/spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala index f3c442ad29..d335d5bbda 100644 --- a/spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala +++ b/spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala @@ -2047,9 +2047,6 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper { sql(s"insert into $table values(0, 1, 100.000001)") Seq( - ( - s"SELECT cast(make_interval(c0, c1, c0, c1, c0, c0, c2) as string) as C from $table", - Set("Cast from CalendarIntervalType to StringType is not supported")), ( "SELECT " + "date_part('YEAR', make_interval(c0, c1, c0, c1, c0, c0, c2))" @@ -2067,9 +2064,7 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper { + s"(SELECT c1, sum(c0) as sum_c0, sum(c2) as sum_c2 from $table group by c1) as A, " + s"(SELECT c1, cast(make_interval(c0, c1, c0, c1, c0, c0, c2) as string) as casted from $table) as B " + "where A.c1 = B.c1 ", - Set( - "Cast from CalendarIntervalType to StringType is not supported", - "Comet shuffle is not enabled: spark.comet.exec.shuffle.enabled is not enabled")), + Set("Comet shuffle is not enabled: spark.comet.exec.shuffle.enabled is not enabled")), (s"select * from $table LIMIT 10 OFFSET 3", Set("Comet shuffle is not enabled"))) .foreach(test => { val qry = test._1 From 71fd8b0f768595794f0529a7733b8d25db5c708d Mon Sep 17 00:00:00 2001 From: comphead Date: Fri, 10 Jul 2026 10:50:30 -0700 Subject: [PATCH 15/34] chore: fallback to Spark if legacy sql configurations are set --- .../cast/cast_complex_types_to_string.sql | 32 +++++++++---------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/spark/src/test/resources/sql-tests/expressions/cast/cast_complex_types_to_string.sql b/spark/src/test/resources/sql-tests/expressions/cast/cast_complex_types_to_string.sql index 09d3bb19ec..ac05b67675 100644 --- a/spark/src/test/resources/sql-tests/expressions/cast/cast_complex_types_to_string.sql +++ b/spark/src/test/resources/sql-tests/expressions/cast/cast_complex_types_to_string.sql @@ -269,8 +269,6 @@ SELECT cast(array(cast(1.5 as double), cast('NaN' as double), cast('-Infinity' a query SELECT cast(array(array(array(1, 2), array(3)), array(array(cast(null as int)))) as string) --- Array of map: not supported, falls back to Spark. -query expect_fallback(to StringType is not supported) SELECT cast(array(map('k', 1)) as string) -- ---------------------------------------------------------------------------- @@ -281,57 +279,57 @@ SELECT cast(array(map('k', 1)) as string) -- tests use literal maps directly rather than reading from a parquet table. -- Map with string keys, int values. -query expect_fallback(Cast from MapType) +query SELECT cast(map('a', 1, 'b', 2, 'c', 3) as string) -- Map with NULL values rendered as "null". -query expect_fallback(Cast from MapType) +query SELECT cast(map('a', 1, 'b', cast(null as int), 'c', 3) as string) -- Map with int keys, string values. -query expect_fallback(Cast from MapType) +query SELECT cast(map(1, 'one', 2, 'two', 3, 'three') as string) -- Map with boolean values. -query expect_fallback(Cast from MapType) +query SELECT cast(map('t', true, 'f', false, 'n', cast(null as boolean)) as string) -- Map with bigint values at min/max. -query expect_fallback(Cast from MapType) +query SELECT cast(map('max', 9223372036854775807, 'min', -9223372036854775808, 'zero', cast(0 as bigint)) as string) -- Map with decimal values. -query expect_fallback(Cast from MapType) +query SELECT cast(map('pos', cast('1.234567890123456789' as decimal(38, 18)), 'neg', cast('-1.234567890123456789' as decimal(38, 18)), 'null', cast(null as decimal(38, 18))) as string) -- Map with date and timestamp values. -query expect_fallback(Cast from MapType) +query SELECT cast(map('a', date '2024-01-15', 'b', date '1970-01-01', 'c', cast(null as date)) as string) -query expect_fallback(Cast from MapType) +query SELECT cast(map('a', timestamp '2024-01-15 10:30:45', 'b', cast(null as timestamp)) as string) -- Map with binary values. -query expect_fallback(Cast from MapType) +query SELECT cast(map('a', X'616263', 'b', X'', 'c', cast(null as binary)) as string) -- Map with float / double values: NaN / ±0 / ±Infinity / NULL. -query expect_fallback(Cast from MapType) +query SELECT cast(map('nan', cast('NaN' as float), 'neg0', cast(-0.0 as float), 'null', cast(null as float)) as string) -query expect_fallback(Cast from MapType) +query SELECT cast(map('nan', cast('NaN' as double), 'inf', cast('Infinity' as double), 'ninf', cast('-Infinity' as double), 'null', cast(null as double)) as string) -- Map with struct values: each value rendered as `{f1, f2, ...}`. -query expect_fallback(Cast from MapType) +query SELECT cast(map('a', named_struct('x', 1, 'y', 'first'), 'b', cast(null as struct)) as string) -- Map with array values. -query expect_fallback(Cast from MapType) +query SELECT cast(map('a', array(1, 2, 3), 'b', array(cast(null as int)), 'c', cast(null as array)) as string) -- Empty map. -query expect_fallback(Cast from MapType) +query SELECT cast(map() as string) -- NULL map: Spark constant-folds this to a literal NULL, so the cast never reaches Comet @@ -340,5 +338,5 @@ query SELECT cast(cast(null as map) as string) -- Map of map. -query expect_fallback(Cast from MapType) +query SELECT cast(map('outer', map('inner', 1)) as string) From 8d9200ce16fc1508c7b9f15e54f7fda543e48aa9 Mon Sep 17 00:00:00 2001 From: comphead Date: Sat, 11 Jul 2026 09:55:52 -0700 Subject: [PATCH 16/34] chore: fallback to Spark if legacy sql configurations are set --- spark/src/test/scala/org/apache/comet/CometCastSuite.scala | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/spark/src/test/scala/org/apache/comet/CometCastSuite.scala b/spark/src/test/scala/org/apache/comet/CometCastSuite.scala index 35b1fbbabe..d3d8472dd7 100644 --- a/spark/src/test/scala/org/apache/comet/CometCastSuite.scala +++ b/spark/src/test/scala/org/apache/comet/CometCastSuite.scala @@ -1790,9 +1790,7 @@ class CometCastSuite extends CometTestBase with AdaptiveSparkPlanHelper { assert( CometCast.isSupported(fromType, toType, None, CometEvalMode.LEGACY) == Unsupported(Some(expectedMessage))) - checkSparkAnswerAndFallbackReason( - data.select(col("a").cast(toType).as("converted")), - expectedMessage) + checkSparkAnswerAndOperator(data.select(col("a").cast(toType).as("converted"))) } } } From a6e71ffd80a2210ffa558473bc8f49a885adedcd Mon Sep 17 00:00:00 2001 From: comphead Date: Sun, 12 Jul 2026 15:50:23 -0700 Subject: [PATCH 17/34] chore: fallback to Spark if legacy sql configurations are set --- .../apache/comet/expressions/CometCast.scala | 20 ++++++++++++++++++- .../apache/comet/shims/CometTypeShim.scala | 3 +++ .../apache/comet/shims/CometTypeShim.scala | 8 +++++++- 3 files changed, 29 insertions(+), 2 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 ef1cd2d753..57db38a2e2 100644 --- a/spark/src/main/scala/org/apache/comet/expressions/CometCast.scala +++ b/spark/src/main/scala/org/apache/comet/expressions/CometCast.scala @@ -29,11 +29,12 @@ import org.apache.comet.DataTypeSupport.isComplexType import org.apache.comet.serde.{CodegenDispatchFallback, CometExpressionSerde, Compatible, ExprOuterClass, Incompatible, SupportLevel, Unsupported} import org.apache.comet.serde.ExprOuterClass.Expr import org.apache.comet.serde.QueryPlanSerde.{evalModeToProto, exprToProtoInternal, serializeDataType} -import org.apache.comet.shims.CometExprShim +import org.apache.comet.shims.{CometExprShim, CometTypeShim} object CometCast extends CometExpressionSerde[Cast] with CometExprShim + with CometTypeShim with CodegenDispatchFallback { // Shared with CometCastSuite so the asserted reason cannot drift from production. @@ -71,6 +72,14 @@ object CometCast // would only duplicate the matrix and risk drifting from it. override def getSupportLevel(cast: Cast): SupportLevel = { + // Reject `VariantType` before the Literal short-circuit below. Folding a Cast whose child or + // target is `VariantType` produces a `Literal[VariantType]` that no downstream Comet serde + // can serialize, and relying on `CometLiteral` to reject it after the fact leaves a native + // path that assumes the produced literal is safe. Guarding here (in addition to the + // recursive check in `isSupported`) forces Spark fallback for every VariantType cast shape. + if (isVariantType(cast.child.dataType) || isVariantType(cast.dataType)) { + return unsupported(cast.child.dataType, cast.dataType) + } if (cast.child.isInstanceOf[Literal]) { // A cast whose child is a literal is folded by Spark at planning time via `cast.eval()` // (see `convert`), so the cast never executes natively and the result matches Spark by @@ -158,6 +167,15 @@ object CometCast timeZoneId: Option[String], evalMode: CometEvalMode.Value): SupportLevel = { + // Spark 4's `VariantType` (SPARK-45827) has no native counterpart in Comet: serializing it + // into the DataFusion plan would fail in `serializeDataType`, and the codegen dispatcher + // cannot compile Variant read/write kernels either. Detect it via the version-shimmed + // `isVariantType` (which returns false on Spark 3.x where the class does not exist) and + // report `Unsupported` so the enclosing operator falls back to Spark. + if (isVariantType(fromType) || isVariantType(toType)) { + return unsupported(fromType, toType) + } + if (fromType == toType) { return Compatible() } diff --git a/spark/src/main/spark-3.x/org/apache/comet/shims/CometTypeShim.scala b/spark/src/main/spark-3.x/org/apache/comet/shims/CometTypeShim.scala index 97320be9e7..b71476c3dd 100644 --- a/spark/src/main/spark-3.x/org/apache/comet/shims/CometTypeShim.scala +++ b/spark/src/main/spark-3.x/org/apache/comet/shims/CometTypeShim.scala @@ -36,6 +36,9 @@ trait CometTypeShim { @nowarn // Spark 4 feature; Variant shredding doesn't exist in Spark 3.x. def isVariantStruct(s: StructType): Boolean = false + @nowarn // Spark 4 feature; VariantType doesn't exist in Spark 3.x. + def isVariantType(dt: DataType): Boolean = false + @nowarn // Spark 4.1 feature; TimeType doesn't exist in Spark 3.x. def isTimeType(dt: DataType): Boolean = false } diff --git a/spark/src/main/spark-4.x/org/apache/comet/shims/CometTypeShim.scala b/spark/src/main/spark-4.x/org/apache/comet/shims/CometTypeShim.scala index 1d4a9f601e..f48955a7da 100644 --- a/spark/src/main/spark-4.x/org/apache/comet/shims/CometTypeShim.scala +++ b/spark/src/main/spark-4.x/org/apache/comet/shims/CometTypeShim.scala @@ -20,7 +20,7 @@ package org.apache.comet.shims import org.apache.spark.sql.execution.datasources.VariantMetadata -import org.apache.spark.sql.types.{ArrayType, DataType, MapType, StringType, StructType} +import org.apache.spark.sql.types.{ArrayType, DataType, MapType, StringType, StructType, VariantType} trait CometTypeShim { // A `StringType` carries collation metadata in Spark 4.0. Only non-default (non-UTF8_BINARY) @@ -54,6 +54,12 @@ trait CometTypeShim { // and force scan fallback. def isVariantStruct(s: StructType): Boolean = VariantMetadata.isVariantStruct(s) + // Comet has no native execution path for Spark 4's `VariantType` (introduced in + // SPARK-45827). Serdes call this to route casts/expressions touching the type back to Spark + // rather than serializing an unsupported datatype into the native plan. Stubbed to `false` in + // Spark 3.x where `VariantType` does not exist. + def isVariantType(dt: DataType): Boolean = dt.isInstanceOf[VariantType] + def isTimeType(dt: DataType): Boolean = dt.getClass.getSimpleName.startsWith("TimeType") From 0ebe80b6daff318bc6ebf87eb709b0d0715e2ce5 Mon Sep 17 00:00:00 2001 From: comphead Date: Mon, 13 Jul 2026 08:18:45 -0700 Subject: [PATCH 18/34] chore: fallback to Spark if legacy sql configurations are set --- dev/diffs/4.0.2.diff | 22 ++++++++++++++++++++++ dev/diffs/4.1.2.diff | 22 ++++++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/dev/diffs/4.0.2.diff b/dev/diffs/4.0.2.diff index d8e506b631..4edaae8cfe 100644 --- a/dev/diffs/4.0.2.diff +++ b/dev/diffs/4.0.2.diff @@ -1374,6 +1374,28 @@ index 2e33f6505ab..be967565303 100644 } withTable("t1", "t2") { +diff --git a/sql/core/src/test/scala/org/apache/spark/sql/VariantEndToEndSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/VariantEndToEndSuite.scala +index a40e34d94d0..d3b3c0801bb 100644 +--- a/sql/core/src/test/scala/org/apache/spark/sql/VariantEndToEndSuite.scala ++++ b/sql/core/src/test/scala/org/apache/spark/sql/VariantEndToEndSuite.scala +@@ -17,6 +17,7 @@ + package org.apache.spark.sql + + import org.apache.spark.{SparkException, SparkRuntimeException} ++import org.apache.spark.sql.IgnoreComet + import org.apache.spark.sql.QueryTest.sameRows + import org.apache.spark.sql.catalyst.InternalRow + import org.apache.spark.sql.catalyst.expressions.{Cast, Literal} +@@ -340,7 +341,8 @@ class VariantEndToEndSuite extends QueryTest with SharedSparkSession { + dataVector.close() + } + +- test("cast to variant/to_variant_object with scan input") { ++ test("cast to variant/to_variant_object with scan input", ++ IgnoreComet("\"VariantType\" not supported")) { + Seq("NO_CODEGEN", "CODEGEN_ONLY").foreach { codegenMode => + withSQLConf(SQLConf.CODEGEN_FACTORY_MODE.key -> codegenMode) { + withTempPath { dir => diff --git a/sql/core/src/test/scala/org/apache/spark/sql/collation/CollationSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/collation/CollationSuite.scala index 11e9547dfc5..637411056ae 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/collation/CollationSuite.scala diff --git a/dev/diffs/4.1.2.diff b/dev/diffs/4.1.2.diff index 9a1b30b010..86448fed06 100644 --- a/dev/diffs/4.1.2.diff +++ b/dev/diffs/4.1.2.diff @@ -1473,6 +1473,28 @@ index 3ba48da0e32..0313aaa9ec6 100644 } withTable("t1", "t2") { +diff --git a/sql/core/src/test/scala/org/apache/spark/sql/VariantEndToEndSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/VariantEndToEndSuite.scala +index 8a0e2c29653..b4c436a88df 100644 +--- a/sql/core/src/test/scala/org/apache/spark/sql/VariantEndToEndSuite.scala ++++ b/sql/core/src/test/scala/org/apache/spark/sql/VariantEndToEndSuite.scala +@@ -17,6 +17,7 @@ + package org.apache.spark.sql + + import org.apache.spark.{SparkException, SparkRuntimeException} ++import org.apache.spark.sql.IgnoreComet + import org.apache.spark.sql.QueryTest.sameRows + import org.apache.spark.sql.catalyst.InternalRow + import org.apache.spark.sql.catalyst.expressions.{Cast, Literal} +@@ -340,7 +341,8 @@ class VariantEndToEndSuite extends QueryTest with SharedSparkSession { + dataVector.close() + } + +- test("cast to variant/to_variant_object with scan input") { ++ test("cast to variant/to_variant_object with scan input", ++ IgnoreComet("\"VariantType\" not supported")) { + Seq("NO_CODEGEN", "CODEGEN_ONLY").foreach { codegenMode => + withSQLConf(SQLConf.CODEGEN_FACTORY_MODE.key -> codegenMode) { + withTempPath { dir => diff --git a/sql/core/src/test/scala/org/apache/spark/sql/VariantShreddingSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/VariantShreddingSuite.scala index fee375db10a..8c2c24e2c5f 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/VariantShreddingSuite.scala From 38fcb201e423499f6051cf324aa627b5a292158d Mon Sep 17 00:00:00 2001 From: comphead Date: Mon, 13 Jul 2026 11:06:32 -0700 Subject: [PATCH 19/34] chore: fallback to Spark if legacy sql configurations are set --- .../spark/sql/CometCollationSuite.scala | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/spark/src/test/spark-4.1/org/apache/spark/sql/CometCollationSuite.scala b/spark/src/test/spark-4.1/org/apache/spark/sql/CometCollationSuite.scala index 05b488d8cd..917501efc8 100644 --- a/spark/src/test/spark-4.1/org/apache/spark/sql/CometCollationSuite.scala +++ b/spark/src/test/spark-4.1/org/apache/spark/sql/CometCollationSuite.scala @@ -214,4 +214,36 @@ class CometCollationSuite extends CometTestBase { "SELECT unix_timestamp(CAST(_2 AS TIMESTAMP), _7) FROM datetime_collation_tbl") } } + + // Port of upstream DefaultCollationTestSuite."create/alter view created from a table". The + // upstream test asserts case-insensitive equality semantics through a view whose column + // carries a non-default collation (UNICODE_CI on c2, then on c1 after ALTER VIEW). Comet's + // native equality/hash compare bytes, not collation-aware keys, so any query that pushes + // a non-default collated string through Comet must fall back to Spark or produce a wrong + // answer. This test runs each subquery through `checkSparkAnswer`, which fails on any + // Comet-vs-Spark divergence. + test("create/alter view created from a table (port of DefaultCollationTestSuite)") { + val testTable = "collation_view_src" + val testView = "collation_view" + withTable(testTable) { + sql(s"CREATE TABLE $testTable (c1 STRING, c2 STRING COLLATE UNICODE_CI) USING parquet") + sql(s"INSERT INTO $testTable VALUES ('a', 'a'), ('A', 'A')") + + withView(testView) { + sql(s"CREATE VIEW $testView AS SELECT * FROM $testTable") + + // c2 filter uses UNICODE_CI (case-insensitive) so both rows match. + checkSparkAnswer(sql(s"SELECT COUNT(*) FROM $testView WHERE c2 = 'A'")) + // c1 filter uses UTF8_BINARY (case-sensitive) so only one row matches. + checkSparkAnswer(sql(s"SELECT COUNT(*) FROM $testView WHERE c1 = 'A'")) + checkSparkAnswer(sql(s"SELECT COUNT(*) FROM $testView WHERE c1 = substring('A', 0, 1)")) + // Literal with explicit collation wins over c1's default collation. + checkSparkAnswer(sql(s"SELECT COUNT(*) FROM $testView WHERE c1 = 'A' collate UNICODE_CI")) + + sql(s"ALTER VIEW $testView AS SELECT c1 COLLATE UNICODE_CI AS c1, c2 FROM $testTable") + // After ALTER: c1 is UNICODE_CI, so both rows match. + checkSparkAnswer(sql(s"SELECT COUNT(*) FROM $testView WHERE c1 = 'A'")) + } + } + } } From dc9a371eba84b1f237867c5d2145bf4df9e720ba Mon Sep 17 00:00:00 2001 From: comphead Date: Mon, 13 Jul 2026 11:08:43 -0700 Subject: [PATCH 20/34] chore: fallback to Spark if legacy sql configurations are set --- .../spark/sql/CometCollationSuite.scala | 32 ------------------- 1 file changed, 32 deletions(-) diff --git a/spark/src/test/spark-4.1/org/apache/spark/sql/CometCollationSuite.scala b/spark/src/test/spark-4.1/org/apache/spark/sql/CometCollationSuite.scala index 917501efc8..05b488d8cd 100644 --- a/spark/src/test/spark-4.1/org/apache/spark/sql/CometCollationSuite.scala +++ b/spark/src/test/spark-4.1/org/apache/spark/sql/CometCollationSuite.scala @@ -214,36 +214,4 @@ class CometCollationSuite extends CometTestBase { "SELECT unix_timestamp(CAST(_2 AS TIMESTAMP), _7) FROM datetime_collation_tbl") } } - - // Port of upstream DefaultCollationTestSuite."create/alter view created from a table". The - // upstream test asserts case-insensitive equality semantics through a view whose column - // carries a non-default collation (UNICODE_CI on c2, then on c1 after ALTER VIEW). Comet's - // native equality/hash compare bytes, not collation-aware keys, so any query that pushes - // a non-default collated string through Comet must fall back to Spark or produce a wrong - // answer. This test runs each subquery through `checkSparkAnswer`, which fails on any - // Comet-vs-Spark divergence. - test("create/alter view created from a table (port of DefaultCollationTestSuite)") { - val testTable = "collation_view_src" - val testView = "collation_view" - withTable(testTable) { - sql(s"CREATE TABLE $testTable (c1 STRING, c2 STRING COLLATE UNICODE_CI) USING parquet") - sql(s"INSERT INTO $testTable VALUES ('a', 'a'), ('A', 'A')") - - withView(testView) { - sql(s"CREATE VIEW $testView AS SELECT * FROM $testTable") - - // c2 filter uses UNICODE_CI (case-insensitive) so both rows match. - checkSparkAnswer(sql(s"SELECT COUNT(*) FROM $testView WHERE c2 = 'A'")) - // c1 filter uses UTF8_BINARY (case-sensitive) so only one row matches. - checkSparkAnswer(sql(s"SELECT COUNT(*) FROM $testView WHERE c1 = 'A'")) - checkSparkAnswer(sql(s"SELECT COUNT(*) FROM $testView WHERE c1 = substring('A', 0, 1)")) - // Literal with explicit collation wins over c1's default collation. - checkSparkAnswer(sql(s"SELECT COUNT(*) FROM $testView WHERE c1 = 'A' collate UNICODE_CI")) - - sql(s"ALTER VIEW $testView AS SELECT c1 COLLATE UNICODE_CI AS c1, c2 FROM $testTable") - // After ALTER: c1 is UNICODE_CI, so both rows match. - checkSparkAnswer(sql(s"SELECT COUNT(*) FROM $testView WHERE c1 = 'A'")) - } - } - } } From b9f18901b61f4e0c48f90036bb90c78c52bd05b9 Mon Sep 17 00:00:00 2001 From: comphead Date: Wed, 15 Jul 2026 17:08:02 -0700 Subject: [PATCH 21/34] chore: fallback to Spark if legacy sql configurations are set --- dev/diffs/4.0.2.diff | 22 ---------------------- dev/diffs/4.1.2.diff | 22 ---------------------- 2 files changed, 44 deletions(-) diff --git a/dev/diffs/4.0.2.diff b/dev/diffs/4.0.2.diff index 4edaae8cfe..d8e506b631 100644 --- a/dev/diffs/4.0.2.diff +++ b/dev/diffs/4.0.2.diff @@ -1374,28 +1374,6 @@ index 2e33f6505ab..be967565303 100644 } withTable("t1", "t2") { -diff --git a/sql/core/src/test/scala/org/apache/spark/sql/VariantEndToEndSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/VariantEndToEndSuite.scala -index a40e34d94d0..d3b3c0801bb 100644 ---- a/sql/core/src/test/scala/org/apache/spark/sql/VariantEndToEndSuite.scala -+++ b/sql/core/src/test/scala/org/apache/spark/sql/VariantEndToEndSuite.scala -@@ -17,6 +17,7 @@ - package org.apache.spark.sql - - import org.apache.spark.{SparkException, SparkRuntimeException} -+import org.apache.spark.sql.IgnoreComet - import org.apache.spark.sql.QueryTest.sameRows - import org.apache.spark.sql.catalyst.InternalRow - import org.apache.spark.sql.catalyst.expressions.{Cast, Literal} -@@ -340,7 +341,8 @@ class VariantEndToEndSuite extends QueryTest with SharedSparkSession { - dataVector.close() - } - -- test("cast to variant/to_variant_object with scan input") { -+ test("cast to variant/to_variant_object with scan input", -+ IgnoreComet("\"VariantType\" not supported")) { - Seq("NO_CODEGEN", "CODEGEN_ONLY").foreach { codegenMode => - withSQLConf(SQLConf.CODEGEN_FACTORY_MODE.key -> codegenMode) { - withTempPath { dir => diff --git a/sql/core/src/test/scala/org/apache/spark/sql/collation/CollationSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/collation/CollationSuite.scala index 11e9547dfc5..637411056ae 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/collation/CollationSuite.scala diff --git a/dev/diffs/4.1.2.diff b/dev/diffs/4.1.2.diff index 86448fed06..9a1b30b010 100644 --- a/dev/diffs/4.1.2.diff +++ b/dev/diffs/4.1.2.diff @@ -1473,28 +1473,6 @@ index 3ba48da0e32..0313aaa9ec6 100644 } withTable("t1", "t2") { -diff --git a/sql/core/src/test/scala/org/apache/spark/sql/VariantEndToEndSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/VariantEndToEndSuite.scala -index 8a0e2c29653..b4c436a88df 100644 ---- a/sql/core/src/test/scala/org/apache/spark/sql/VariantEndToEndSuite.scala -+++ b/sql/core/src/test/scala/org/apache/spark/sql/VariantEndToEndSuite.scala -@@ -17,6 +17,7 @@ - package org.apache.spark.sql - - import org.apache.spark.{SparkException, SparkRuntimeException} -+import org.apache.spark.sql.IgnoreComet - import org.apache.spark.sql.QueryTest.sameRows - import org.apache.spark.sql.catalyst.InternalRow - import org.apache.spark.sql.catalyst.expressions.{Cast, Literal} -@@ -340,7 +341,8 @@ class VariantEndToEndSuite extends QueryTest with SharedSparkSession { - dataVector.close() - } - -- test("cast to variant/to_variant_object with scan input") { -+ test("cast to variant/to_variant_object with scan input", -+ IgnoreComet("\"VariantType\" not supported")) { - Seq("NO_CODEGEN", "CODEGEN_ONLY").foreach { codegenMode => - withSQLConf(SQLConf.CODEGEN_FACTORY_MODE.key -> codegenMode) { - withTempPath { dir => diff --git a/sql/core/src/test/scala/org/apache/spark/sql/VariantShreddingSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/VariantShreddingSuite.scala index fee375db10a..8c2c24e2c5f 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/VariantShreddingSuite.scala From e81fb820b98b4a36d45449bee710a7dc1cb1880c Mon Sep 17 00:00:00 2001 From: comphead Date: Wed, 15 Jul 2026 17:13:35 -0700 Subject: [PATCH 22/34] chore: fallback to Spark if legacy sql configurations are set --- spark/src/main/scala/org/apache/comet/serde/aggregates.scala | 1 - 1 file changed, 1 deletion(-) diff --git a/spark/src/main/scala/org/apache/comet/serde/aggregates.scala b/spark/src/main/scala/org/apache/comet/serde/aggregates.scala index 7381c65acb..1fb676366c 100644 --- a/spark/src/main/scala/org/apache/comet/serde/aggregates.scala +++ b/spark/src/main/scala/org/apache/comet/serde/aggregates.scala @@ -108,7 +108,6 @@ object CometMax extends CometAggregateExpressionSerde[Max] { } object CometCount extends CometAggregateExpressionSerde[Count] { - override def convert( aggExpr: AggregateExpression, expr: Count, From bc1eabc5bf39c9f85eb10de9dfe9966cf9588071 Mon Sep 17 00:00:00 2001 From: comphead Date: Thu, 16 Jul 2026 09:37:07 -0700 Subject: [PATCH 23/34] chore: move str_to_map collation dispatcher to collation branch The CodegenDispatchFallback mixin on CometStrToMap and the matching str_to_map_collation.sql fixture edit belong with the collation PR, not this legacy-conf branch. Revert both back to upstream/main here; they land on the `collation` branch instead. --- .../src/main/scala/org/apache/comet/serde/maps.scala | 11 +++-------- .../expressions/map/str_to_map_collation.sql | 9 +++++---- 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/spark/src/main/scala/org/apache/comet/serde/maps.scala b/spark/src/main/scala/org/apache/comet/serde/maps.scala index c8304440c3..a5ae24d215 100644 --- a/spark/src/main/scala/org/apache/comet/serde/maps.scala +++ b/spark/src/main/scala/org/apache/comet/serde/maps.scala @@ -166,17 +166,12 @@ object CometMapFromEntries } } -object CometStrToMap - extends CometScalarFunction[StringToMap]("str_to_map") - with CometTypeShim - with CodegenDispatchFallback { +object CometStrToMap extends CometScalarFunction[StringToMap]("str_to_map") with CometTypeShim { // Spark 4.1.1+ honours spark.sql.legacy.truncateForEmptyRegexSplit by truncating trailing // empty entries from the split result. Comet's native str_to_map always behaves as if the flag - // were false. When the flag is true, mark this Incompatible so the CodegenDispatchFallback - // trait routes the expression through the JVM codegen dispatcher (Spark's own doGenCode inside - // the Comet kernel) rather than falling the entire projection back to Spark. Read by string - // key so it resolves on older Spark versions where the config is not registered. + // were false, so it is incompatible when legacy truncation is enabled. Read by string key so it + // resolves on older Spark versions where the config is not registered. private val legacyTruncateConfig = "spark.sql.legacy.truncateForEmptyRegexSplit" private val legacyTruncateReason = diff --git a/spark/src/test/resources/sql-tests/expressions/map/str_to_map_collation.sql b/spark/src/test/resources/sql-tests/expressions/map/str_to_map_collation.sql index 41594f612c..7816a1442c 100644 --- a/spark/src/test/resources/sql-tests/expressions/map/str_to_map_collation.sql +++ b/spark/src/test/resources/sql-tests/expressions/map/str_to_map_collation.sql @@ -17,16 +17,17 @@ -- MinSparkVersion: 4.0 --- Spark 4.0+ applies collation-aware delimiter matching in str_to_map. +-- Spark 4.0+ applies collation-aware delimiter matching in str_to_map. Comet's native +-- implementation matches raw delimiter bytes, so non-UTF8_BINARY collations must fall back. -- collated input string -query +query expect_fallback(`str_to_map` does not support non-UTF8_BINARY collations) SELECT str_to_map('a:1,b:2' COLLATE UTF8_LCASE, ',', ':') -- collated pair delimiter -query +query expect_fallback(`str_to_map` does not support non-UTF8_BINARY collations) SELECT str_to_map('aX1,bX2', 'x' COLLATE UTF8_LCASE, ':') -- collated key-value delimiter -query +query expect_fallback(`str_to_map` does not support non-UTF8_BINARY collations) SELECT str_to_map('aX1,bX2', ',', 'x' COLLATE UTF8_LCASE) From 4c56ea80034ade542231fd916482d1c27bdbb8b9 Mon Sep 17 00:00:00 2001 From: comphead Date: Thu, 16 Jul 2026 12:36:50 -0700 Subject: [PATCH 24/34] chore: move str_to_map_legacy_truncate.sql edit to collation branch The updated assertions in this fixture rely on CometStrToMap having CodegenDispatchFallback mixed in, which now lives on the collation branch. Revert to upstream/main here so the file matches the on-disk serde behaviour of the chore branch. --- .../map/str_to_map_legacy_truncate.sql | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/spark/src/test/resources/sql-tests/expressions/map/str_to_map_legacy_truncate.sql b/spark/src/test/resources/sql-tests/expressions/map/str_to_map_legacy_truncate.sql index 8ab8058920..a5b5eb9f16 100644 --- a/spark/src/test/resources/sql-tests/expressions/map/str_to_map_legacy_truncate.sql +++ b/spark/src/test/resources/sql-tests/expressions/map/str_to_map_legacy_truncate.sql @@ -15,22 +15,19 @@ -- specific language governing permissions and limitations -- under the License. --- Tests that str_to_map routes through the JVM codegen dispatcher when --- spark.sql.legacy.truncateForEmptyRegexSplit is enabled. In legacy mode Spark truncates --- trailing empty entries from the split result, which Comet's native str_to_map does not --- honour. `CometStrToMap` marks the expression Incompatible when the flag is on and mixes in --- [[CodegenDispatchFallback]], so the projection stays native (Spark's own `doGenCode` runs --- inside the Comet kernel) while producing Spark-exact results. +-- Tests that str_to_map falls back to Spark when +-- spark.sql.legacy.truncateForEmptyRegexSplit is enabled. In legacy mode Spark truncates trailing +-- empty entries from the split result, which Comet's native str_to_map does not honour. -- See https://github.com/apache/datafusion-comet/issues/4477 -- Config: spark.sql.legacy.truncateForEmptyRegexSplit=true --- trailing pair delimiter: legacy mode truncates the trailing empty entry; Comet delegates to --- the codegen dispatcher. -query +-- trailing pair delimiter: legacy mode truncates the trailing empty entry, so Comet must fall +-- back to Spark +query expect_fallback(truncateForEmptyRegexSplit) SELECT str_to_map('a:1,b:2,', ',', ':') --- column input is also handled via the codegen dispatcher +-- column input also falls back statement CREATE TABLE test_str_to_map_legacy(s STRING, pair_delim STRING, key_value_delim STRING) USING parquet @@ -40,5 +37,5 @@ INSERT INTO test_str_to_map_legacy VALUES ('x:1;y:2;', ';', ':'), (NULL, ',', ':') -query +query expect_fallback(truncateForEmptyRegexSplit) SELECT str_to_map(s, pair_delim, key_value_delim) FROM test_str_to_map_legacy From a75411d4090e6b3c07441097e970f0ae60c4c96f Mon Sep 17 00:00:00 2001 From: comphead Date: Sat, 18 Jul 2026 10:18:24 -0700 Subject: [PATCH 25/34] chore: fallback to Spark if legacy sql configurations are set --- spark/src/main/scala/org/apache/comet/serde/predicates.scala | 1 + 1 file changed, 1 insertion(+) diff --git a/spark/src/main/scala/org/apache/comet/serde/predicates.scala b/spark/src/main/scala/org/apache/comet/serde/predicates.scala index 79e4a0c64b..b679e2c7fa 100644 --- a/spark/src/main/scala/org/apache/comet/serde/predicates.scala +++ b/spark/src/main/scala/org/apache/comet/serde/predicates.scala @@ -22,6 +22,7 @@ package org.apache.comet.serde import scala.jdk.CollectionConverters._ import org.apache.spark.sql.catalyst.expressions.{And, Attribute, BinaryExpression, EqualNullSafe, EqualTo, Expression, GreaterThan, GreaterThanOrEqual, In, InSet, IsNaN, IsNotNull, IsNull, LessThan, LessThanOrEqual, Literal, Not, Or} +import org.apache.spark.sql.internal.SQLConf import org.apache.spark.sql.types.BooleanType import org.apache.comet.CometSparkSessionExtensions.withFallbackReason From c035e4dd9a005e4d563a1e61280da5c69e078546 Mon Sep 17 00:00:00 2001 From: comphead Date: Sat, 18 Jul 2026 10:58:54 -0700 Subject: [PATCH 26/34] chore: fallback to Spark if legacy sql configurations are set --- .../scala/org/apache/comet/serde/maps.scala | 11 ++++++++--- .../expressions/map/str_to_map_collation.sql | 9 ++++----- .../map/str_to_map_legacy_truncate.sql | 19 +++++++++++-------- 3 files changed, 23 insertions(+), 16 deletions(-) diff --git a/spark/src/main/scala/org/apache/comet/serde/maps.scala b/spark/src/main/scala/org/apache/comet/serde/maps.scala index a5ae24d215..c8304440c3 100644 --- a/spark/src/main/scala/org/apache/comet/serde/maps.scala +++ b/spark/src/main/scala/org/apache/comet/serde/maps.scala @@ -166,12 +166,17 @@ object CometMapFromEntries } } -object CometStrToMap extends CometScalarFunction[StringToMap]("str_to_map") with CometTypeShim { +object CometStrToMap + extends CometScalarFunction[StringToMap]("str_to_map") + with CometTypeShim + with CodegenDispatchFallback { // Spark 4.1.1+ honours spark.sql.legacy.truncateForEmptyRegexSplit by truncating trailing // empty entries from the split result. Comet's native str_to_map always behaves as if the flag - // were false, so it is incompatible when legacy truncation is enabled. Read by string key so it - // resolves on older Spark versions where the config is not registered. + // were false. When the flag is true, mark this Incompatible so the CodegenDispatchFallback + // trait routes the expression through the JVM codegen dispatcher (Spark's own doGenCode inside + // the Comet kernel) rather than falling the entire projection back to Spark. Read by string + // key so it resolves on older Spark versions where the config is not registered. private val legacyTruncateConfig = "spark.sql.legacy.truncateForEmptyRegexSplit" private val legacyTruncateReason = diff --git a/spark/src/test/resources/sql-tests/expressions/map/str_to_map_collation.sql b/spark/src/test/resources/sql-tests/expressions/map/str_to_map_collation.sql index 7816a1442c..41594f612c 100644 --- a/spark/src/test/resources/sql-tests/expressions/map/str_to_map_collation.sql +++ b/spark/src/test/resources/sql-tests/expressions/map/str_to_map_collation.sql @@ -17,17 +17,16 @@ -- MinSparkVersion: 4.0 --- Spark 4.0+ applies collation-aware delimiter matching in str_to_map. Comet's native --- implementation matches raw delimiter bytes, so non-UTF8_BINARY collations must fall back. +-- Spark 4.0+ applies collation-aware delimiter matching in str_to_map. -- collated input string -query expect_fallback(`str_to_map` does not support non-UTF8_BINARY collations) +query SELECT str_to_map('a:1,b:2' COLLATE UTF8_LCASE, ',', ':') -- collated pair delimiter -query expect_fallback(`str_to_map` does not support non-UTF8_BINARY collations) +query SELECT str_to_map('aX1,bX2', 'x' COLLATE UTF8_LCASE, ':') -- collated key-value delimiter -query expect_fallback(`str_to_map` does not support non-UTF8_BINARY collations) +query SELECT str_to_map('aX1,bX2', ',', 'x' COLLATE UTF8_LCASE) diff --git a/spark/src/test/resources/sql-tests/expressions/map/str_to_map_legacy_truncate.sql b/spark/src/test/resources/sql-tests/expressions/map/str_to_map_legacy_truncate.sql index a5b5eb9f16..8ab8058920 100644 --- a/spark/src/test/resources/sql-tests/expressions/map/str_to_map_legacy_truncate.sql +++ b/spark/src/test/resources/sql-tests/expressions/map/str_to_map_legacy_truncate.sql @@ -15,19 +15,22 @@ -- specific language governing permissions and limitations -- under the License. --- Tests that str_to_map falls back to Spark when --- spark.sql.legacy.truncateForEmptyRegexSplit is enabled. In legacy mode Spark truncates trailing --- empty entries from the split result, which Comet's native str_to_map does not honour. +-- Tests that str_to_map routes through the JVM codegen dispatcher when +-- spark.sql.legacy.truncateForEmptyRegexSplit is enabled. In legacy mode Spark truncates +-- trailing empty entries from the split result, which Comet's native str_to_map does not +-- honour. `CometStrToMap` marks the expression Incompatible when the flag is on and mixes in +-- [[CodegenDispatchFallback]], so the projection stays native (Spark's own `doGenCode` runs +-- inside the Comet kernel) while producing Spark-exact results. -- See https://github.com/apache/datafusion-comet/issues/4477 -- Config: spark.sql.legacy.truncateForEmptyRegexSplit=true --- trailing pair delimiter: legacy mode truncates the trailing empty entry, so Comet must fall --- back to Spark -query expect_fallback(truncateForEmptyRegexSplit) +-- trailing pair delimiter: legacy mode truncates the trailing empty entry; Comet delegates to +-- the codegen dispatcher. +query SELECT str_to_map('a:1,b:2,', ',', ':') --- column input also falls back +-- column input is also handled via the codegen dispatcher statement CREATE TABLE test_str_to_map_legacy(s STRING, pair_delim STRING, key_value_delim STRING) USING parquet @@ -37,5 +40,5 @@ INSERT INTO test_str_to_map_legacy VALUES ('x:1;y:2;', ';', ':'), (NULL, ',', ':') -query expect_fallback(truncateForEmptyRegexSplit) +query SELECT str_to_map(s, pair_delim, key_value_delim) FROM test_str_to_map_legacy From 2e76ba82261f98543b610d265d2262500ff8c6c2 Mon Sep 17 00:00:00 2001 From: comphead Date: Tue, 21 Jul 2026 08:56:54 -0700 Subject: [PATCH 27/34] chore: fallback to Spark if legacy sql configurations are set --- dev/diffs/4.0.2.diff | 70 ++++++++++++++------------------------------ dev/diffs/4.1.2.diff | 70 ++++++++++++++------------------------------ 2 files changed, 44 insertions(+), 96 deletions(-) diff --git a/dev/diffs/4.0.2.diff b/dev/diffs/4.0.2.diff index d8e506b631..871db0b818 100644 --- a/dev/diffs/4.0.2.diff +++ b/dev/diffs/4.0.2.diff @@ -785,54 +785,6 @@ index 9c529d14221..ab2850b5d68 100644 }.flatten assert(filters.contains(GreaterThan(scan.logicalPlan.output.head, Literal(5L)))) } -diff --git a/sql/core/src/test/scala/org/apache/spark/sql/IgnoreComet.scala b/sql/core/src/test/scala/org/apache/spark/sql/IgnoreComet.scala -new file mode 100644 -index 00000000000..4b31bea33de ---- /dev/null -+++ b/sql/core/src/test/scala/org/apache/spark/sql/IgnoreComet.scala -@@ -0,0 +1,42 @@ -+/* -+ * 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.spark.sql -+ -+import org.scalactic.source.Position -+import org.scalatest.Tag -+ -+import org.apache.spark.sql.test.SQLTestUtils -+ -+/** -+ * Tests with this tag will be ignored when Comet is enabled (e.g., via `ENABLE_COMET`). -+ */ -+case class IgnoreComet(reason: String) extends Tag("DisableComet") -+ -+/** -+ * Helper trait that disables Comet for all tests regardless of default config values. -+ */ -+trait IgnoreCometSuite extends SQLTestUtils { -+ override protected def test(testName: String, testTags: Tag*)(testFun: => Any) -+ (implicit pos: Position): Unit = { -+ if (isCometEnabled) { -+ ignore(testName + " (disabled when Comet is on)", testTags: _*)(testFun) -+ } else { -+ super.test(testName, testTags: _*)(testFun) -+ } -+ } -+} diff --git a/sql/core/src/test/scala/org/apache/spark/sql/JoinHintSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/JoinHintSuite.scala index 53e47f428c3..a55d8f0c161 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/JoinHintSuite.scala @@ -1374,6 +1326,28 @@ index 2e33f6505ab..be967565303 100644 } withTable("t1", "t2") { +diff --git a/sql/core/src/test/scala/org/apache/spark/sql/VariantEndToEndSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/VariantEndToEndSuite.scala +index a40e34d94d0..abc1f035d15 100644 +--- a/sql/core/src/test/scala/org/apache/spark/sql/VariantEndToEndSuite.scala ++++ b/sql/core/src/test/scala/org/apache/spark/sql/VariantEndToEndSuite.scala +@@ -21,6 +21,7 @@ import org.apache.spark.sql.QueryTest.sameRows + import org.apache.spark.sql.catalyst.InternalRow + import org.apache.spark.sql.catalyst.expressions.{Cast, Literal} + import org.apache.spark.sql.catalyst.expressions.variant.{ToVariantObject, VariantExpressionEvalUtils} ++import org.apache.spark.sql.comet.CometNativeColumnarToRowExec + import org.apache.spark.sql.execution.WholeStageCodegenExec + import org.apache.spark.sql.execution.vectorized.OnHeapColumnVector + import org.apache.spark.sql.functions._ +@@ -358,7 +359,8 @@ class VariantEndToEndSuite extends QueryTest with SharedSparkSession { + s"cast(to_variant_object(s) as ${schema(2).dataType.sql})") + checkAnswer(df, input) + val plan = df.queryExecution.executedPlan +- assert(plan.isInstanceOf[WholeStageCodegenExec] == (codegenMode == "CODEGEN_ONLY")) ++ assert(plan.isInstanceOf[WholeStageCodegenExec] == (codegenMode == "CODEGEN_ONLY") ++ || plan.isInstanceOf[CometNativeColumnarToRowExec]) + } + } + } diff --git a/sql/core/src/test/scala/org/apache/spark/sql/collation/CollationSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/collation/CollationSuite.scala index 11e9547dfc5..637411056ae 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/collation/CollationSuite.scala diff --git a/dev/diffs/4.1.2.diff b/dev/diffs/4.1.2.diff index 9a1b30b010..be919c96ae 100644 --- a/dev/diffs/4.1.2.diff +++ b/dev/diffs/4.1.2.diff @@ -856,54 +856,6 @@ index 95e86fe4311..fb2b6363af6 100644 }.flatten assert(filters.contains(GreaterThan(scan.logicalPlan.output.head, Literal(5L)))) } -diff --git a/sql/core/src/test/scala/org/apache/spark/sql/IgnoreComet.scala b/sql/core/src/test/scala/org/apache/spark/sql/IgnoreComet.scala -new file mode 100644 -index 00000000000..4b31bea33de ---- /dev/null -+++ b/sql/core/src/test/scala/org/apache/spark/sql/IgnoreComet.scala -@@ -0,0 +1,42 @@ -+/* -+ * 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.spark.sql -+ -+import org.scalactic.source.Position -+import org.scalatest.Tag -+ -+import org.apache.spark.sql.test.SQLTestUtils -+ -+/** -+ * Tests with this tag will be ignored when Comet is enabled (e.g., via `ENABLE_COMET`). -+ */ -+case class IgnoreComet(reason: String) extends Tag("DisableComet") -+ -+/** -+ * Helper trait that disables Comet for all tests regardless of default config values. -+ */ -+trait IgnoreCometSuite extends SQLTestUtils { -+ override protected def test(testName: String, testTags: Tag*)(testFun: => Any) -+ (implicit pos: Position): Unit = { -+ if (isCometEnabled) { -+ ignore(testName + " (disabled when Comet is on)", testTags: _*)(testFun) -+ } else { -+ super.test(testName, testTags: _*)(testFun) -+ } -+ } -+} diff --git a/sql/core/src/test/scala/org/apache/spark/sql/JoinHintSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/JoinHintSuite.scala index 53e47f428c3..a55d8f0c161 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/JoinHintSuite.scala @@ -1473,6 +1425,28 @@ index 3ba48da0e32..0313aaa9ec6 100644 } withTable("t1", "t2") { +diff --git a/sql/core/src/test/scala/org/apache/spark/sql/VariantEndToEndSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/VariantEndToEndSuite.scala +index 8a0e2c29653..d276a51cbc6 100644 +--- a/sql/core/src/test/scala/org/apache/spark/sql/VariantEndToEndSuite.scala ++++ b/sql/core/src/test/scala/org/apache/spark/sql/VariantEndToEndSuite.scala +@@ -21,6 +21,7 @@ import org.apache.spark.sql.QueryTest.sameRows + import org.apache.spark.sql.catalyst.InternalRow + import org.apache.spark.sql.catalyst.expressions.{Cast, Literal} + import org.apache.spark.sql.catalyst.expressions.variant.{ToVariantObject, VariantExpressionEvalUtils} ++import org.apache.spark.sql.comet.CometNativeColumnarToRowExec + import org.apache.spark.sql.execution.WholeStageCodegenExec + import org.apache.spark.sql.execution.vectorized.OnHeapColumnVector + import org.apache.spark.sql.functions._ +@@ -358,7 +359,8 @@ class VariantEndToEndSuite extends QueryTest with SharedSparkSession { + s"cast(to_variant_object(s) as ${schema(2).dataType.sql})") + checkAnswer(df, input) + val plan = df.queryExecution.executedPlan +- assert(plan.isInstanceOf[WholeStageCodegenExec] == (codegenMode == "CODEGEN_ONLY")) ++ assert(plan.isInstanceOf[WholeStageCodegenExec] == (codegenMode == "CODEGEN_ONLY") ++ || plan.isInstanceOf[CometNativeColumnarToRowExec]) + } + } + } diff --git a/sql/core/src/test/scala/org/apache/spark/sql/VariantShreddingSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/VariantShreddingSuite.scala index fee375db10a..8c2c24e2c5f 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/VariantShreddingSuite.scala From 3d3258c6b771212d475741ffe1c11abe6d3d1980 Mon Sep 17 00:00:00 2001 From: comphead Date: Tue, 21 Jul 2026 14:10:24 -0700 Subject: [PATCH 28/34] chore: fallback to Spark if legacy sql configurations are set --- dev/diffs/4.0.2.diff | 48 ++++++++++++++++++++++++++++++++++++++++++++ dev/diffs/4.1.2.diff | 48 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+) diff --git a/dev/diffs/4.0.2.diff b/dev/diffs/4.0.2.diff index 871db0b818..882f8128f4 100644 --- a/dev/diffs/4.0.2.diff +++ b/dev/diffs/4.0.2.diff @@ -785,6 +785,54 @@ index 9c529d14221..ab2850b5d68 100644 }.flatten assert(filters.contains(GreaterThan(scan.logicalPlan.output.head, Literal(5L)))) } +diff --git a/sql/core/src/test/scala/org/apache/spark/sql/IgnoreComet.scala b/sql/core/src/test/scala/org/apache/spark/sql/IgnoreComet.scala +new file mode 100644 +index 00000000000..4b31bea33de +--- /dev/null ++++ b/sql/core/src/test/scala/org/apache/spark/sql/IgnoreComet.scala +@@ -0,0 +1,42 @@ ++/* ++ * 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.spark.sql ++ ++import org.scalactic.source.Position ++import org.scalatest.Tag ++ ++import org.apache.spark.sql.test.SQLTestUtils ++ ++/** ++ * Tests with this tag will be ignored when Comet is enabled (e.g., via `ENABLE_COMET`). ++ */ ++case class IgnoreComet(reason: String) extends Tag("DisableComet") ++ ++/** ++ * Helper trait that disables Comet for all tests regardless of default config values. ++ */ ++trait IgnoreCometSuite extends SQLTestUtils { ++ override protected def test(testName: String, testTags: Tag*)(testFun: => Any) ++ (implicit pos: Position): Unit = { ++ if (isCometEnabled) { ++ ignore(testName + " (disabled when Comet is on)", testTags: _*)(testFun) ++ } else { ++ super.test(testName, testTags: _*)(testFun) ++ } ++ } ++} diff --git a/sql/core/src/test/scala/org/apache/spark/sql/JoinHintSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/JoinHintSuite.scala index 53e47f428c3..a55d8f0c161 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/JoinHintSuite.scala diff --git a/dev/diffs/4.1.2.diff b/dev/diffs/4.1.2.diff index be919c96ae..e56a3ed22d 100644 --- a/dev/diffs/4.1.2.diff +++ b/dev/diffs/4.1.2.diff @@ -856,6 +856,54 @@ index 95e86fe4311..fb2b6363af6 100644 }.flatten assert(filters.contains(GreaterThan(scan.logicalPlan.output.head, Literal(5L)))) } +diff --git a/sql/core/src/test/scala/org/apache/spark/sql/IgnoreComet.scala b/sql/core/src/test/scala/org/apache/spark/sql/IgnoreComet.scala +new file mode 100644 +index 00000000000..4b31bea33de +--- /dev/null ++++ b/sql/core/src/test/scala/org/apache/spark/sql/IgnoreComet.scala +@@ -0,0 +1,42 @@ ++/* ++ * 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.spark.sql ++ ++import org.scalactic.source.Position ++import org.scalatest.Tag ++ ++import org.apache.spark.sql.test.SQLTestUtils ++ ++/** ++ * Tests with this tag will be ignored when Comet is enabled (e.g., via `ENABLE_COMET`). ++ */ ++case class IgnoreComet(reason: String) extends Tag("DisableComet") ++ ++/** ++ * Helper trait that disables Comet for all tests regardless of default config values. ++ */ ++trait IgnoreCometSuite extends SQLTestUtils { ++ override protected def test(testName: String, testTags: Tag*)(testFun: => Any) ++ (implicit pos: Position): Unit = { ++ if (isCometEnabled) { ++ ignore(testName + " (disabled when Comet is on)", testTags: _*)(testFun) ++ } else { ++ super.test(testName, testTags: _*)(testFun) ++ } ++ } ++} diff --git a/sql/core/src/test/scala/org/apache/spark/sql/JoinHintSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/JoinHintSuite.scala index 53e47f428c3..a55d8f0c161 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/JoinHintSuite.scala From bfe1a7656f142c0311f908cbddfdd967961ef27b Mon Sep 17 00:00:00 2001 From: comphead Date: Tue, 21 Jul 2026 16:54:48 -0700 Subject: [PATCH 29/34] chore: fallback to Spark if legacy sql configurations are set --- .../user-guide/latest/compatibility/index.md | 9 ++------- .../comet/shims/ShimLegacyConfFallback.scala | 1 - .../comet/shims/ShimLegacyConfFallback.scala | 2 -- .../comet/CometSparkSessionExtensionsSuite.scala | 14 +++++++++----- 4 files changed, 11 insertions(+), 15 deletions(-) diff --git a/docs/source/user-guide/latest/compatibility/index.md b/docs/source/user-guide/latest/compatibility/index.md index ca028167f0..d87258d119 100644 --- a/docs/source/user-guide/latest/compatibility/index.md +++ b/docs/source/user-guide/latest/compatibility/index.md @@ -111,7 +111,8 @@ semantics. Comet handles these in two ways: - **Per-expression**: when a legacy config affects a specific Spark expression that Comet supports (for example `spark.sql.legacy.castComplexTypesToString.enabled` for `Cast`, `spark.sql.legacy.negativeIndexInArrayInsert` for `array_insert`, - `spark.sql.legacy.nullInEmptyListBehavior` for `IN`), Comet's serde routes the expression + `spark.sql.legacy.nullInEmptyListBehavior` for `IN`, `spark.sql.legacy.timeParserPolicy` + for datetime parsing expressions), Comet's serde routes the expression through the JVM codegen dispatcher (Spark's own `doGenCode` inside the Comet kernel) or through a native code path that honors the flag. No session-wide fallback is triggered. - **Session-wide execution fallback**: when a legacy config affects execution semantics but @@ -176,12 +177,6 @@ the legacy flag does not disable Comet for the whole session. | ----------------------------------------- | ---------------------- | | `spark.sql.legacy.viewSchemaCompensation` | `true` | -**Datetime parser policy** - -| Config key | Comet-expected default | -| ----------------------------------- | ---------------------- | -| `spark.sql.legacy.timeParserPolicy` | `CORRECTED` | - **Parquet reader/writer semantics** | Config key | Comet-expected default | diff --git a/spark/src/main/spark-3.x/org/apache/comet/shims/ShimLegacyConfFallback.scala b/spark/src/main/spark-3.x/org/apache/comet/shims/ShimLegacyConfFallback.scala index c675f8405f..8b0dcd3cf5 100644 --- a/spark/src/main/spark-3.x/org/apache/comet/shims/ShimLegacyConfFallback.scala +++ b/spark/src/main/spark-3.x/org/apache/comet/shims/ShimLegacyConfFallback.scala @@ -43,7 +43,6 @@ trait ShimLegacyConfFallback { "spark.sql.legacy.disableMapKeyNormalization" -> "false", "spark.sql.legacy.setopsPrecedence.enabled" -> "false", "spark.sql.legacy.viewSchemaCompensation" -> "true", - "spark.sql.legacy.timeParserPolicy" -> "CORRECTED", "spark.sql.legacy.parquet.datetimeRebaseModeInRead" -> "CORRECTED", "spark.sql.legacy.parquet.datetimeRebaseModeInWrite" -> "CORRECTED", "spark.sql.legacy.parquet.int96RebaseModeInRead" -> "CORRECTED", diff --git a/spark/src/main/spark-4.x/org/apache/comet/shims/ShimLegacyConfFallback.scala b/spark/src/main/spark-4.x/org/apache/comet/shims/ShimLegacyConfFallback.scala index 25a49cd11f..579e670aac 100644 --- a/spark/src/main/spark-4.x/org/apache/comet/shims/ShimLegacyConfFallback.scala +++ b/spark/src/main/spark-4.x/org/apache/comet/shims/ShimLegacyConfFallback.scala @@ -62,8 +62,6 @@ trait ShimLegacyConfFallback { SQLConf.LEGACY_SETOPS_PRECEDENCE_ENABLED.defaultValueString, "spark.sql.legacy.viewSchemaCompensation" -> SQLConf.VIEW_SCHEMA_COMPENSATION.defaultValueString, - "spark.sql.legacy.timeParserPolicy" -> - SQLConf.LEGACY_TIME_PARSER_POLICY.defaultValueString, "spark.sql.legacy.parquet.datetimeRebaseModeInRead" -> SQLConf.PARQUET_REBASE_MODE_IN_READ.defaultValueString, "spark.sql.legacy.parquet.datetimeRebaseModeInWrite" -> diff --git a/spark/src/test/scala/org/apache/comet/CometSparkSessionExtensionsSuite.scala b/spark/src/test/scala/org/apache/comet/CometSparkSessionExtensionsSuite.scala index e65b86569f..6405075dd5 100644 --- a/spark/src/test/scala/org/apache/comet/CometSparkSessionExtensionsSuite.scala +++ b/spark/src/test/scala/org/apache/comet/CometSparkSessionExtensionsSuite.scala @@ -69,15 +69,19 @@ class CometSparkSessionExtensionsSuite extends CometTestBase { conf.setConfString("spark.sql.legacy.charVarcharAsString", "FALSE") assert(isCometLoaded(conf)) - // Enum-default configs also trigger when set to a non-default value. - conf.setConfString("spark.sql.legacy.timeParserPolicy", "LEGACY") + // Enum-default configs also trigger when set to a non-default value. Use viewSchemaCompensation + // as an enum-valued example (default `true`) since timeParserPolicy is handled per-expression. + conf.setConfString("spark.sql.legacy.viewSchemaCompensation", "false") assert(!isCometLoaded(conf)) - conf.setConfString("spark.sql.legacy.timeParserPolicy", "CORRECTED") + conf.setConfString("spark.sql.legacy.viewSchemaCompensation", "TRUE") assert(isCometLoaded(conf)) // Legacy configs handled per-expression (e.g. castComplexTypesToString, - // allowNegativeScaleOfDecimal) are NOT part of the fallback set and must not disable Comet - // on their own. + // allowNegativeScaleOfDecimal, timeParserPolicy) are NOT part of the fallback set and must not + // disable Comet on their own. + conf.setConfString("spark.sql.legacy.timeParserPolicy", "LEGACY") + assert(isCometLoaded(conf)) + conf.unsetConf("spark.sql.legacy.timeParserPolicy") conf.setConfString("spark.sql.legacy.castComplexTypesToString.enabled", "true") assert(isCometLoaded(conf)) conf.unsetConf("spark.sql.legacy.castComplexTypesToString.enabled") From 5665fa16c828fbe78ff5eaf8fd98ed25157d3781 Mon Sep 17 00:00:00 2001 From: comphead Date: Wed, 22 Jul 2026 16:27:41 -0700 Subject: [PATCH 30/34] chore: fallback to Spark if legacy sql configurations are set --- .../user-guide/latest/compatibility/index.md | 27 ++++++++----- .../apache/comet/expressions/CometCast.scala | 14 +++++++ .../apache/comet/rules/CometScanRule.scala | 40 +++++++++++++++++++ .../comet/shims/ShimLegacyConfFallback.scala | 10 ++--- .../comet/shims/ShimLegacyConfFallback.scala | 21 ++++------ .../CometSparkSessionExtensionsSuite.scala | 9 +++++ .../comet/rules/CometScanRuleSuite.scala | 35 ++++++++++++++++ 7 files changed, 129 insertions(+), 27 deletions(-) diff --git a/docs/source/user-guide/latest/compatibility/index.md b/docs/source/user-guide/latest/compatibility/index.md index d87258d119..7f724c1443 100644 --- a/docs/source/user-guide/latest/compatibility/index.md +++ b/docs/source/user-guide/latest/compatibility/index.md @@ -177,15 +177,24 @@ the legacy flag does not disable Comet for the whole session. | ----------------------------------------- | ---------------------- | | `spark.sql.legacy.viewSchemaCompensation` | `true` | -**Parquet reader/writer semantics** - -| Config key | Comet-expected default | -| ---------------------------------------------------- | ---------------------- | -| `spark.sql.legacy.parquet.datetimeRebaseModeInRead` | `CORRECTED` | -| `spark.sql.legacy.parquet.datetimeRebaseModeInWrite` | `CORRECTED` | -| `spark.sql.legacy.parquet.int96RebaseModeInRead` | `CORRECTED` | -| `spark.sql.legacy.parquet.int96RebaseModeInWrite` | `CORRECTED` | -| `spark.sql.legacy.parquet.nanosAsLong` | `false` | +**Parquet reader semantics (per-scan, not session-wide)** + +Parquet legacy read configs affect how bytes on disk map to Spark rows, so a single non-default +value would only ever change results for queries that read Parquet files. Instead of disabling +Comet for the whole session, `CometScanRule` checks these keys per scan and falls back the +individual scan to Spark. Non-Parquet queries in the same session continue to run on Comet. + +Both the primary name and the `spark.sql.legacy.*` alias are monitored (Spark's `SQLConf.contains` +does NOT follow `withAlternative` links). Write-side rebase configs are intentionally not part +of this check: they only affect writes and would not change scan output. + +| Config key | Comet-expected default | +| --------------------------------------------------- | ---------------------- | +| `spark.sql.parquet.datetimeRebaseModeInRead` | `CORRECTED` | +| `spark.sql.legacy.parquet.datetimeRebaseModeInRead` | `CORRECTED` | +| `spark.sql.parquet.int96RebaseModeInRead` | `CORRECTED` | +| `spark.sql.legacy.parquet.int96RebaseModeInRead` | `CORRECTED` | +| `spark.sql.legacy.parquet.nanosAsLong` | `false` | **Cached-plan behavior on file-source scans** 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 57db38a2e2..3484651ece 100644 --- a/spark/src/main/scala/org/apache/comet/expressions/CometCast.scala +++ b/spark/src/main/scala/org/apache/comet/expressions/CometCast.scala @@ -44,11 +44,22 @@ object CometCast private[comet] val legacyCastComplexTypesToStringReason: String = "spark.sql.legacy.castComplexTypesToString.enabled=true is not supported natively" + private[comet] val nonDefaultTimeParserPolicyReason: String = + "spark.sql.legacy.timeParserPolicy is set to a non-CORRECTED value; the native " + + "string-to-datetime parser only implements CORRECTED semantics" + private def legacyCastComplexTypesToString: Boolean = SQLConf.get .getConfString("spark.sql.legacy.castComplexTypesToString.enabled", "false") .toBoolean + // Non-CORRECTED policies (LEGACY, EXCEPTION) change string-to-date/timestamp parsing behavior in + // ways the native cast kernel does not replicate. + private def isNonDefaultTimeParserPolicy: Boolean = + !SQLConf.get + .getConfString("spark.sql.legacy.timeParserPolicy", "CORRECTED") + .equalsIgnoreCase("CORRECTED") + def supportedTypes: Seq[DataType] = Seq( DataTypes.BooleanType, @@ -264,6 +275,9 @@ object CometCast Compatible() case _: DecimalType => Compatible() + case DataTypes.DateType | DataTypes.TimestampType | _: TimestampNTZType + if isNonDefaultTimeParserPolicy => + Incompatible(Some(nonDefaultTimeParserPolicyReason)) case DataTypes.DateType => // https://github.com/apache/datafusion-comet/issues/327 Compatible(Some("Only supports years between 262143 BC and 262142 AD")) diff --git a/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala b/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala index 59331129d5..9967f5b6bc 100644 --- a/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala +++ b/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala @@ -204,6 +204,12 @@ case class CometScanRule(session: SparkSession) s"Native Parquet scan requires ${COMET_EXEC_ENABLED.key} to be enabled") return None } + CometScanRule.parquetFallbackReason(conf) match { + case Some(reason) => + withFallbackReason(scanExec, reason) + return None + case None => + } // Comet's native readers go through object_store, which only understands a fixed set of URL // schemes. A custom Hadoop FileSystem (e.g. registered via spark.hadoop.fs..impl) would // surface at execution time as `Generic URL error: Unable to recognise URL "..."`. Decline here @@ -844,6 +850,40 @@ object CometScanRule extends Logging { val SKIP_COMET_SCAN_TAG: org.apache.spark.sql.catalyst.trees.TreeNodeTag[Unit] = org.apache.spark.sql.catalyst.trees.TreeNodeTag[Unit]("comet.skipCometScan") + /** + * Parquet legacy configs that change how bytes on disk map to Spark rows/values. Set to a + * non-CORRECTED / non-false value, the native Parquet reader would silently return different + * results than Spark for the affected files. Comet monitors both the primary key + * (`spark.sql.parquet.*RebaseMode*`, Spark 3.2+) and its `spark.sql.legacy.*` alias since + * `SQLConf.contains` does NOT follow `withAlternative` links. Write-side rebase configs are + * intentionally omitted -- they only affect writes and shouldn't disqualify a scan. + */ + private val parquetReadFallbackDefaults: Map[String, String] = Map( + "spark.sql.parquet.datetimeRebaseModeInRead" -> "CORRECTED", + "spark.sql.legacy.parquet.datetimeRebaseModeInRead" -> "CORRECTED", + "spark.sql.parquet.int96RebaseModeInRead" -> "CORRECTED", + "spark.sql.legacy.parquet.int96RebaseModeInRead" -> "CORRECTED", + "spark.sql.legacy.parquet.nanosAsLong" -> "false") + + /** + * Returns a fallback reason when any monitored Parquet legacy config is explicitly set to a + * non-default value, or `None` otherwise. Called by [[nativeScan]] so the fallback only fires + * for the parquet scan it affects, not the whole session. + */ + private[rules] def parquetFallbackReason( + conf: org.apache.spark.sql.internal.SQLConf): Option[String] = { + val triggered = parquetReadFallbackDefaults.collect { + case (key, safeDefault) + if conf.contains(key) && !conf.getConfString(key).equalsIgnoreCase(safeDefault) => + key + }.toSeq.sorted + if (triggered.isEmpty) None + else + Some( + "Native Parquet scan does not implement the legacy semantics requested by " + + s"${triggered.mkString(", ")}; falling back to Spark for this scan.") + } + /** * Single-pass validation of Iceberg FileScanTasks. * diff --git a/spark/src/main/spark-3.x/org/apache/comet/shims/ShimLegacyConfFallback.scala b/spark/src/main/spark-3.x/org/apache/comet/shims/ShimLegacyConfFallback.scala index 8b0dcd3cf5..568029df5d 100644 --- a/spark/src/main/spark-3.x/org/apache/comet/shims/ShimLegacyConfFallback.scala +++ b/spark/src/main/spark-3.x/org/apache/comet/shims/ShimLegacyConfFallback.scala @@ -27,6 +27,11 @@ package org.apache.comet.shims * fallback rule is "when a legacy key is set to something other than the Spark 4 default, disable * Comet". This keeps 3.x and 4.x behaviourally aligned even though 3.x can't derive the defaults * live. See the 4.x shim for the reference-derived variant. + * + * Parquet-specific legacy configs (`spark.sql.(legacy.)?parquet.*RebaseMode*`, + * `spark.sql.legacy.parquet.nanosAsLong`) are intentionally NOT in this session-wide set. They + * are checked per-scan in [[org.apache.comet.rules.CometScanRule.parquetFallbackReason]] and only + * fall back the scan they affect, so non-Parquet queries in the same session stay on Comet. */ trait ShimLegacyConfFallback { @@ -43,10 +48,5 @@ trait ShimLegacyConfFallback { "spark.sql.legacy.disableMapKeyNormalization" -> "false", "spark.sql.legacy.setopsPrecedence.enabled" -> "false", "spark.sql.legacy.viewSchemaCompensation" -> "true", - "spark.sql.legacy.parquet.datetimeRebaseModeInRead" -> "CORRECTED", - "spark.sql.legacy.parquet.datetimeRebaseModeInWrite" -> "CORRECTED", - "spark.sql.legacy.parquet.int96RebaseModeInRead" -> "CORRECTED", - "spark.sql.legacy.parquet.int96RebaseModeInWrite" -> "CORRECTED", - "spark.sql.legacy.parquet.nanosAsLong" -> "false", "spark.sql.legacy.readFileSourceTableCacheIgnoreOptions" -> "false") } diff --git a/spark/src/main/spark-4.x/org/apache/comet/shims/ShimLegacyConfFallback.scala b/spark/src/main/spark-4.x/org/apache/comet/shims/ShimLegacyConfFallback.scala index 579e670aac..280b80721a 100644 --- a/spark/src/main/spark-4.x/org/apache/comet/shims/ShimLegacyConfFallback.scala +++ b/spark/src/main/spark-4.x/org/apache/comet/shims/ShimLegacyConfFallback.scala @@ -27,9 +27,14 @@ import org.apache.spark.sql.internal.SQLConf * the hardcoded counterpart. * * Every key returned here uses the `spark.sql.legacy.*` name that the check compares against. - * When a legacy key was removed in Spark 4 (the four parquet rebase aliases), we still ship the - * legacy key -> Spark 4 non-legacy default; on Spark 4 the check is effectively inert because - * `SQLConf.set` rejects removed keys, but the entry is kept so 3.x behaviour is consistent. + * When a legacy key was removed in Spark 4, we still ship the legacy key -> Spark 4 non-legacy + * default; on Spark 4 the check is effectively inert because `SQLConf.set` rejects removed keys, + * but the entry is kept so 3.x behaviour is consistent. + * + * Parquet-specific legacy configs (`spark.sql.(legacy.)?parquet.*RebaseMode*`, + * `spark.sql.legacy.parquet.nanosAsLong`) are intentionally NOT in this session-wide set. They + * are checked per-scan in [[org.apache.comet.rules.CometScanRule.parquetFallbackReason]] and only + * fall back the scan they affect, so non-Parquet queries in the same session stay on Comet. * * Note: `ConfigEntry` itself is `private[spark]`, so we never spell the type — every value below * is a direct method call on a `SQLConf` val, and the compiler resolves `defaultValueString` @@ -62,16 +67,6 @@ trait ShimLegacyConfFallback { SQLConf.LEGACY_SETOPS_PRECEDENCE_ENABLED.defaultValueString, "spark.sql.legacy.viewSchemaCompensation" -> SQLConf.VIEW_SCHEMA_COMPENSATION.defaultValueString, - "spark.sql.legacy.parquet.datetimeRebaseModeInRead" -> - SQLConf.PARQUET_REBASE_MODE_IN_READ.defaultValueString, - "spark.sql.legacy.parquet.datetimeRebaseModeInWrite" -> - SQLConf.PARQUET_REBASE_MODE_IN_WRITE.defaultValueString, - "spark.sql.legacy.parquet.int96RebaseModeInRead" -> - SQLConf.PARQUET_INT96_REBASE_MODE_IN_READ.defaultValueString, - "spark.sql.legacy.parquet.int96RebaseModeInWrite" -> - SQLConf.PARQUET_INT96_REBASE_MODE_IN_WRITE.defaultValueString, - "spark.sql.legacy.parquet.nanosAsLong" -> - SQLConf.LEGACY_PARQUET_NANOS_AS_LONG.defaultValueString, "spark.sql.legacy.readFileSourceTableCacheIgnoreOptions" -> SQLConf.READ_FILE_SOURCE_TABLE_CACHE_IGNORE_OPTIONS.defaultValueString) } diff --git a/spark/src/test/scala/org/apache/comet/CometSparkSessionExtensionsSuite.scala b/spark/src/test/scala/org/apache/comet/CometSparkSessionExtensionsSuite.scala index 6405075dd5..cf6c467dd0 100644 --- a/spark/src/test/scala/org/apache/comet/CometSparkSessionExtensionsSuite.scala +++ b/spark/src/test/scala/org/apache/comet/CometSparkSessionExtensionsSuite.scala @@ -89,6 +89,15 @@ class CometSparkSessionExtensionsSuite extends CometTestBase { assert(isCometLoaded(conf)) conf.unsetConf("spark.sql.legacy.allowNegativeScaleOfDecimal") + // Parquet legacy configs are checked per-scan by CometScanRule, not session-wide, so setting + // them here must NOT disable Comet for the whole session. + conf.setConfString("spark.sql.parquet.datetimeRebaseModeInRead", "LEGACY") + assert(isCometLoaded(conf)) + conf.unsetConf("spark.sql.parquet.datetimeRebaseModeInRead") + conf.setConfString("spark.sql.legacy.parquet.datetimeRebaseModeInRead", "LEGACY") + assert(isCometLoaded(conf)) + conf.unsetConf("spark.sql.legacy.parquet.datetimeRebaseModeInRead") + // Opt-out: users can keep Comet enabled by disabling the fallback (compatibility not // guaranteed). conf.setConfString("spark.sql.legacy.charVarcharAsString", "true") diff --git a/spark/src/test/scala/org/apache/comet/rules/CometScanRuleSuite.scala b/spark/src/test/scala/org/apache/comet/rules/CometScanRuleSuite.scala index f444fe62c9..90ed75ed10 100644 --- a/spark/src/test/scala/org/apache/comet/rules/CometScanRuleSuite.scala +++ b/spark/src/test/scala/org/apache/comet/rules/CometScanRuleSuite.scala @@ -98,6 +98,41 @@ class CometScanRuleSuite extends CometTestBase { } } + test("parquetFallbackReason: default configs -> no fallback") { + val conf = new org.apache.spark.sql.internal.SQLConf + assert(CometScanRule.parquetFallbackReason(conf).isEmpty) + } + + test("parquetFallbackReason: primary key set to LEGACY triggers per-scan fallback") { + val conf = new org.apache.spark.sql.internal.SQLConf + conf.setConfString("spark.sql.parquet.datetimeRebaseModeInRead", "LEGACY") + val reason = CometScanRule.parquetFallbackReason(conf) + assert(reason.isDefined) + assert(reason.get.contains("spark.sql.parquet.datetimeRebaseModeInRead")) + } + + test("parquetFallbackReason: legacy alias set to LEGACY triggers per-scan fallback") { + val conf = new org.apache.spark.sql.internal.SQLConf + conf.setConfString("spark.sql.legacy.parquet.datetimeRebaseModeInRead", "LEGACY") + val reason = CometScanRule.parquetFallbackReason(conf) + assert(reason.isDefined) + assert(reason.get.contains("spark.sql.legacy.parquet.datetimeRebaseModeInRead")) + } + + test("parquetFallbackReason: nanosAsLong=true triggers per-scan fallback") { + val conf = new org.apache.spark.sql.internal.SQLConf + conf.setConfString("spark.sql.legacy.parquet.nanosAsLong", "true") + val reason = CometScanRule.parquetFallbackReason(conf) + assert(reason.isDefined) + assert(reason.get.contains("spark.sql.legacy.parquet.nanosAsLong")) + } + + test("parquetFallbackReason: write-side rebase config does not trigger scan fallback") { + val conf = new org.apache.spark.sql.internal.SQLConf + conf.setConfString("spark.sql.parquet.datetimeRebaseModeInWrite", "LEGACY") + assert(CometScanRule.parquetFallbackReason(conf).isEmpty) + } + test("CometScanRule should fallback to Spark for ShortType when safety check enabled") { withTempPath { path => // Create test data with ShortType which may be from unsigned UINT_8 From ab8a7e86dac45f205d8e305b316ba0f21cd437f0 Mon Sep 17 00:00:00 2001 From: comphead Date: Wed, 22 Jul 2026 17:41:26 -0700 Subject: [PATCH 31/34] chore: fallback to Spark if legacy sql configurations are set --- .../org/apache/comet/rules/CometScanRule.scala | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala b/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala index 9967f5b6bc..d7bb588879 100644 --- a/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala +++ b/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala @@ -872,11 +872,14 @@ object CometScanRule extends Logging { */ private[rules] def parquetFallbackReason( conf: org.apache.spark.sql.internal.SQLConf): Option[String] = { - val triggered = parquetReadFallbackDefaults.collect { - case (key, safeDefault) - if conf.contains(key) && !conf.getConfString(key).equalsIgnoreCase(safeDefault) => - key - }.toSeq.sorted + val triggered = parquetReadFallbackDefaults + .collect { + case (key, safeDefault) + if conf.contains(key) && !conf.getConfString(key).equalsIgnoreCase(safeDefault) => + key + } + .toSeq + .sorted if (triggered.isEmpty) None else Some( From 2c7de490d20ba377fb6613eebcc4377a48fe2edb Mon Sep 17 00:00:00 2001 From: comphead Date: Thu, 23 Jul 2026 08:47:10 -0700 Subject: [PATCH 32/34] chore: fallback to Spark if legacy sql configurations are set --- .../main/scala/org/apache/comet/rules/CometScanRule.scala | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala b/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala index d7bb588879..26fee893f1 100644 --- a/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala +++ b/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala @@ -880,11 +880,13 @@ object CometScanRule extends Logging { } .toSeq .sorted - if (triggered.isEmpty) None - else + if (triggered.isEmpty) { + None + } else { Some( "Native Parquet scan does not implement the legacy semantics requested by " + s"${triggered.mkString(", ")}; falling back to Spark for this scan.") + } } /** From c47d3fb61acdbdfe6119c2a2288ead20bfac31bb Mon Sep 17 00:00:00 2001 From: comphead Date: Thu, 23 Jul 2026 13:26:20 -0700 Subject: [PATCH 33/34] chore: fallback to Spark if legacy sql configurations are set --- .../comet/CometSparkSessionExtensions.scala | 2 +- .../org/apache/comet/LegacyConfFallback.scala | 24 +++++++++---------- .../apache/comet/rules/CometScanRule.scala | 2 +- 3 files changed, 13 insertions(+), 15 deletions(-) diff --git a/spark/src/main/scala/org/apache/comet/CometSparkSessionExtensions.scala b/spark/src/main/scala/org/apache/comet/CometSparkSessionExtensions.scala index 07be830ec0..1cb9df3f19 100644 --- a/spark/src/main/scala/org/apache/comet/CometSparkSessionExtensions.scala +++ b/spark/src/main/scala/org/apache/comet/CometSparkSessionExtensions.scala @@ -160,7 +160,7 @@ object CometSparkSessionExtensions extends Logging { if (COMET_LEGACY_CONF_FALLBACK_ENABLED.get(conf)) { val triggered = LegacyConfFallback.triggeredConfigs(conf) if (triggered.nonEmpty) { - val keys = triggered.toSeq.sorted.mkString(", ") + val keys = triggered.mkString(", ") logWarning( "Comet extension is disabled because the following execution-affecting " + s"spark.sql.legacy.* configs are set to non-default values: $keys. Comet does not " + diff --git a/spark/src/main/scala/org/apache/comet/LegacyConfFallback.scala b/spark/src/main/scala/org/apache/comet/LegacyConfFallback.scala index 9c2788c271..46e8d95619 100644 --- a/spark/src/main/scala/org/apache/comet/LegacyConfFallback.scala +++ b/spark/src/main/scala/org/apache/comet/LegacyConfFallback.scala @@ -44,19 +44,17 @@ import org.apache.comet.shims.ShimLegacyConfFallback private[comet] object LegacyConfFallback extends ShimLegacyConfFallback { /** - * Map of legacy config key -> case-insensitive Spark default value. A config triggers the - * fallback when it is present in the session conf AND its value is not equal (case-insensitive) - * to the default recorded here. + * Keys in [[legacyConfDefaults]] whose value on `conf` differs (case-insensitive) from the + * recorded Spark default. Returned sorted so the warning message is deterministic. */ - val executionAffectingDefaults: Map[String, String] = legacyConfDefaults - - /** Keys in [[executionAffectingDefaults]] that are set to a non-default value on `conf`. */ - def triggeredConfigs(conf: SQLConf): Iterable[String] = { - executionAffectingDefaults.iterator.collect { - case (key, safeDefault) - if conf.contains(key) && - !conf.getConfString(key).equalsIgnoreCase(safeDefault) => - key - }.toSeq + def triggeredConfigs(conf: SQLConf): Seq[String] = { + legacyConfDefaults + .collect { + case (key, safeDefault) + if !conf.getConfString(key, safeDefault).equalsIgnoreCase(safeDefault) => + key + } + .toSeq + .sorted } } diff --git a/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala b/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala index 26fee893f1..3557253a06 100644 --- a/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala +++ b/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala @@ -875,7 +875,7 @@ object CometScanRule extends Logging { val triggered = parquetReadFallbackDefaults .collect { case (key, safeDefault) - if conf.contains(key) && !conf.getConfString(key).equalsIgnoreCase(safeDefault) => + if !conf.getConfString(key, safeDefault).equalsIgnoreCase(safeDefault) => key } .toSeq From 90cb673fa6bdb0e54cbbd372baec37bf3212b194 Mon Sep 17 00:00:00 2001 From: comphead Date: Thu, 23 Jul 2026 17:14:55 -0700 Subject: [PATCH 34/34] chore: fallback to Spark if legacy sql configurations are set --- .../user-guide/latest/compatibility/index.md | 8 ++- .../scala/org/apache/comet/serde/arrays.scala | 8 +++ .../array/exists_legacy_three_valued.sql | 72 +++++++++++++++++++ 3 files changed, 85 insertions(+), 3 deletions(-) create mode 100644 spark/src/test/resources/sql-tests/expressions/array/exists_legacy_three_valued.sql diff --git a/docs/source/user-guide/latest/compatibility/index.md b/docs/source/user-guide/latest/compatibility/index.md index 7f724c1443..c61f9cf7a0 100644 --- a/docs/source/user-guide/latest/compatibility/index.md +++ b/docs/source/user-guide/latest/compatibility/index.md @@ -112,9 +112,11 @@ semantics. Comet handles these in two ways: supports (for example `spark.sql.legacy.castComplexTypesToString.enabled` for `Cast`, `spark.sql.legacy.negativeIndexInArrayInsert` for `array_insert`, `spark.sql.legacy.nullInEmptyListBehavior` for `IN`, `spark.sql.legacy.timeParserPolicy` - for datetime parsing expressions), Comet's serde routes the expression - through the JVM codegen dispatcher (Spark's own `doGenCode` inside the Comet kernel) or - through a native code path that honors the flag. No session-wide fallback is triggered. + for datetime parsing expressions, `spark.sql.legacy.sizeOfNull` for `size`/`cardinality`, + `spark.sql.legacy.followThreeValuedLogicInArrayExists` for `exists`), Comet's serde routes + the expression through the JVM codegen dispatcher (Spark's own `doGenCode` inside the Comet + kernel) or through a native code path that honors the flag. No session-wide fallback is + triggered. - **Session-wide execution fallback**: when a legacy config affects execution semantics but is consumed by an analyzer/optimizer rule, a data-source reader/writer, or a type-system utility (rather than a specific Comet-supported expression), Comet cannot fix the divergence diff --git a/spark/src/main/scala/org/apache/comet/serde/arrays.scala b/spark/src/main/scala/org/apache/comet/serde/arrays.scala index 1b91406c49..47e58332ee 100644 --- a/spark/src/main/scala/org/apache/comet/serde/arrays.scala +++ b/spark/src/main/scala/org/apache/comet/serde/arrays.scala @@ -761,6 +761,10 @@ object CometArrayFilter extends CometExpressionSerde[ArrayFilter] { } } +// `spark.sql.legacy.sizeOfNull` (default `true`, effective value `LEGACY_SIZE_OF_NULL && +// !ANSI_ENABLED`) is captured by Spark's `Size` constructor into the `legacySizeOfNull` field. +// `convert` below reads that field and bakes the correct literal (`-1` or `null`) into the +// CaseWhen's else branch, so both semantics run natively without a fallback. object CometSize extends CometExpressionSerde[Size] { override def getSupportLevel(expr: Size): SupportLevel = { @@ -943,6 +947,10 @@ trait ArraysBase { object CometArrayTransform extends CometCodegenDispatch[ArrayTransform] +// `spark.sql.legacy.followThreeValuedLogicInArrayExists` (default `true`) is captured by +// Spark's `ArrayExists` constructor into the `followThreeValuedLogic` field. Routing through +// codegen dispatch runs Spark's own `doGenCode`, which closes over that field, so both flag +// values produce Spark-exact results without a per-serde gate. object CometArrayExists extends CometCodegenDispatch[ArrayExists] object CometArrayForAll extends CometCodegenDispatch[ArrayForAll] diff --git a/spark/src/test/resources/sql-tests/expressions/array/exists_legacy_three_valued.sql b/spark/src/test/resources/sql-tests/expressions/array/exists_legacy_three_valued.sql new file mode 100644 index 0000000000..f9b1b6b110 --- /dev/null +++ b/spark/src/test/resources/sql-tests/expressions/array/exists_legacy_three_valued.sql @@ -0,0 +1,72 @@ +-- 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. + +-- Regression coverage for `spark.sql.legacy.followThreeValuedLogicInArrayExists`. Spark's +-- `ArrayExists` captures the flag into a boolean field at construction time +-- (`ArrayExists.followThreeValuedLogic`). `CometArrayExists` is a `CometCodegenDispatch`, so +-- Spark's own `doGenCode` runs inside the Comet kernel and closes over that field. Both flag +-- values therefore produce Spark-exact results without any serde-level gate. +-- +-- The config's default is `true` in all Comet-supported Spark versions (3.4.3, 3.5.8, 4.0.1, +-- 4.1.1). Under `true`, a predicate that yields NULL for some element and no TRUE elsewhere +-- makes `exists` return NULL. Under `false`, NULL predicate results are treated as FALSE. + +-- ConfigMatrix: spark.sql.legacy.followThreeValuedLogicInArrayExists=true,false +-- ConfigMatrix: parquet.enable.dictionary=false,true + +statement +CREATE TABLE test_exists_3vl(a array) USING parquet + +statement +INSERT INTO test_exists_3vl VALUES + (array(1, 2, 3)), + (array(1, NULL, 3)), + (array(NULL, NULL)), + (array()), + (NULL) + +-- Non-null array with a matching element: always true regardless of flag. +query +SELECT exists(a, x -> x > 2) FROM test_exists_3vl + +-- Predicate never true for non-null elements. Under legacy=true a null element makes the +-- result NULL; under legacy=false the null is treated as false so the result is false. +query +SELECT exists(a, x -> x > 10) FROM test_exists_3vl + +-- Predicate explicitly handles NULL (returns TRUE for null elements) — result is +-- independent of the flag. +query +SELECT exists(a, x -> x IS NULL) FROM test_exists_3vl + +-- Predicate that is null-safe (COALESCE) — predicate never returns NULL, so both flag values +-- produce the same TRUE/FALSE result. +query +SELECT exists(a, x -> coalesce(x, -1) > 0) FROM test_exists_3vl + +-- Literal-array cases pinning each corner: +-- * a matching value is found -> true under both configs +-- * no match, but a null element present -> null (legacy=true) / false (legacy=false) +-- * empty array -> false under both configs +-- * NULL array -> null under both configs +query +SELECT + exists(array(1, NULL, 3), x -> x > 2), + exists(array(1, NULL, 3), x -> x > 10), + exists(array(NULL, NULL), x -> x > 0), + exists(array(), x -> x > 0), + exists(cast(NULL as array), x -> x > 0)