chore: fallback to Spark if legacy sql configurations are set by comphead · Pull Request #4799 · apache/datafusion-comet

comphead · 2026-07-02T20:26:46Z

Which issue does this PR close?

Closes #4786.
Closes #5013

Rationale for this change

spark.sql.legacy.* configs opt into pre-modern Spark semantics that Comet's native operators
don't implement. Leaving Comet enabled when they're set can silently diverge from Spark. This
PR makes Comet safe-by-default across the whole family.

What changes are included in this PR?

Session-wide fallback — new spark.comet.legacyConfFallback.enabled (default true).
When any curated spark.sql.legacy.* key is set to a non-default value, isCometLoaded
returns false and warns. Curated set (via new LegacyConfFallback + version-shimmed
ShimLegacyConfFallback) covers decimal/analyzer rules, char/varchar, upcast/type-coercion,
optimizer plan-shaping, view-schema compensation, and file-source cache options.

Per-scan Parquet fallback — CometScanRule.parquetFallbackReason checks both the primary
key and its spark.sql.legacy.* alias for datetimeRebaseModeInRead, int96RebaseModeInRead,
and nanosAsLong, falling back only the affected scan. Write-side configs excluded.

Per-expression handling for configs tied to specific expressions:

Cast: honor timeParserPolicy; route castComplexTypesToString.enabled via codegen
dispatch; add VariantType guard.
ArrayInsert: gate negativeIndexInArrayInsert via CodegenDispatchFallback.
In/InSet: gate nullInEmptyListBehavior (resolved as explicit || !ansiEnabled).
StructsToCsv: gate nullValueWrittenAsQuotedEmptyStringCsv.
StringLPad/StringRPad: reject BinaryType str (lpadRpadAlwaysReturnString).
Docs-only notes on Size / ArrayExists (already honored via Spark-constructed fields).

Shim — new isVariantType on CometTypeShim (false on 3.x, real check on 4.x).

Tests + docs — unit tests for session-wide and per-scan fallbacks, SQL file tests for
legacy dispatch (array_insert, exists, complex-type cast), refreshed 4.0.2/4.1.2 diffs,
new "Spark legacy configs" section in compatibility/index.md.

How are these changes tested?

New tests in CometSparkSessionExtensionsSuite, CometScanRuleSuite, updated
CometCastSuite/CometExpressionSuite, new SQL file tests, refreshed Spark diffs.

andygrove · 2026-07-02T21:07:16Z

+`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


can't we just update the serde for specific expressions to return Unsupported rather than fall back for all queries, even queries that would not be affected by a legacy config?

we should just fall back to the codegen dispatch approach in most cases

Let me investigate this, sometimes the conf param is vaguely explained like

val LEGACY_JAVA_CHARSETS = buildConf("spark.sql.legacy.javaCharsets") .internal() .doc("When set to true, the functions like `encode()` can use charsets from JDK while " + "encoding or decoding string values. If it is false, such functions support only one of " + "the charsets: 'US-ASCII', 'ISO-8859-1', 'UTF-8', 'UTF-16BE', 'UTF-16LE', 'UTF-16', " + "'UTF-32'.") .version("4.0.0") .booleanConf .createWithDefault(false)

it refers to encode() explicitly but also to something else. Maybe I can fish out exact functions from Spark code depending on legacy params and in this case we can fallback to codegen dispatch

Also, Spark has legacy configs that default to true, so this will make all jobs fall back by default.

val VIEW_SCHEMA_BINDING_ENABLED = buildConf("spark.sql.legacy.viewSchemaBindingMode") .internal() .doc("Set to false to disable the WITH SCHEMA clause for view DDL and suppress the line in " + "DESCRIBE EXTENDED and SHOW CREATE TABLE.") .version("4.0.0") .booleanConf .createWithDefault(true)

andygrove · 2026-07-15T16:58:46Z

+-  test("cast to variant/to_variant_object with scan input") {
+  test("cast to variant/to_variant_object with scan input",
+      IgnoreComet("\"VariantType\" not supported")) {


This seems like a regression. This test was previously passing because we fell back to Spark and now we fail the query?

andygrove · 2026-07-15T16:59:59Z

+  // 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()
+    }
+  }


This change looks good and could really be a standalone PR. This also looks like a bug fix for an issue where we were lacking tests?

andygrove · 2026-07-15T17:00:20Z

 }

 object CometCount extends CometAggregateExpressionSerde[Count] {
+


could you revert this whitespace change

andygrove · 2026-07-15T17:11:53Z

+    "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",


Can we not handle the parquet ones in scan planning?

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.

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.

andygrove · 2026-07-22T17:01:18Z

Nice to see the redesign land. A couple of correctness questions before this ships. (Review drafted with LLM assistance.)

1. timeParserPolicy=LEGACY currently produces no fallback and no per-expression check.

The new test in CometSparkSessionExtensionsSuite asserts that spark.sql.legacy.timeParserPolicy=LEGACY must NOT trigger the session-wide fallback, on the stated reasoning that it's handled per-expression. But grepping the JVM source shows no serde actually consults timeParserPolicy. So under timeParserPolicy=LEGACY, Comet stays enabled and runs the native datetime parsers with the default (CORRECTED) semantics, and results silently diverge from Spark for any query that parses a date/timestamp string. The comment on that test case says the same thing as the description, but neither seems to match the code.

Either the flag needs to go into ShimLegacyConfFallback.legacyConfDefaults, or the affected datetime expressions need to gate on it (mirroring how CometArrayInsert and CometCast do). Same question for spark.sql.legacy.castComplexTypesToString.enabled when spark.comet.expr.Cast.allowIncompatible=true — the native path runs but doesn't honor the legacy formatting. That's within the documented Incompatible contract, so it's arguably fine, just worth being intentional about.

2. Setting the primary parquet key instead of the legacy alias silently bypasses the fallback.

The fallback keys off conf.contains("spark.sql.legacy.parquet.datetimeRebaseModeInRead") etc., but Spark treats these as aliases for spark.sql.parquet.datetimeRebaseModeInRead. A user who sets the primary (non-legacy) name to LEGACY gets zero fallback signal, and the native Parquet reader doesn't honor it either — so the write in the file gets applied as-is without rebasing. That's a real divergence for any Parquet file written by Spark 2.x. Could the check either look at the primary key (via the resolved SQLConf entry) or explicitly document that only the legacy alias is monitored?

3. Description bullet mentions timeParserPolicy in the curated session-wide list.

The description's session-wide bullet reads parquet datetime/int96 rebase modes, charVarcharAsString, timeParserPolicy, ..., but timeParserPolicy is deliberately NOT in legacyConfDefaults. Whichever way (1) is resolved, the description should match.

andygrove · 2026-07-23T21:13:31Z

Comparing the curated lists here against the inventory in #4180, these spark.sql.legacy.* configs from that issue are not currently handled by this PR (no session-wide fallback, no per-expression dispatch, no support-level check):

spark.sql.legacy.sizeOfNull (size(null) returns -1 vs null)
spark.sql.legacy.allowCastNumericToTimestamp (CAST(bigint AS timestamp) permitted or not)
spark.sql.legacy.followThreeValuedLogicInArrayExists (exists(array, pred) null handling)
spark.sql.legacy.integerGroupingIdEnabled (grouping_id() return type)
spark.sql.legacy.json.allowEmptyString.enabled (JSON parsing behavior)
spark.sql.legacy.createEmptyCollectionUsingStringType (empty array() / map() element type)

Should any of these be added to legacyConfDefaults or gated per-expression in this PR, or is the intent to leave them for follow-up work under #4180?

Some may already be safe to ignore. A few are analyzer-side and get resolved before Comet ever sees the plan (integerGroupingIdEnabled, createEmptyCollectionUsingStringType, allowCastNumericToTimestamp), so if that reasoning holds it would be worth capturing in the compatibility doc alongside the existing note for allowNegativeScaleOfDecimal, so a reader can tell "considered and safe" from "not yet looked at". The ones that seem most likely to need real handling are sizeOfNull and followThreeValuedLogicInArrayExists, since both change runtime results for expressions Comet executes natively.

andygrove · 2026-07-27T16:19:31Z

+      if (triggered.nonEmpty) {
+        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 " +
+            "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


Do we still need this? I thought we were moving the checks to the affected expressions/scans now? Disabling the plugin seems like overkill, IMO

andygrove · 2026-07-27T16:28:53Z

+    extends CometExpressionSerde[Cast]
+    with CometExprShim
+    with CometTypeShim
+    with CodegenDispatchFallback {


Adding codegen-dispatch for cast is a large change. Seems like this should be a separate PR?

andygrove · 2026-07-27T16:37:23Z

Zooming out from the individual file comments, I think this PR is doing several independent things and would be much easier to review and to revert if it were split. Right now it is 24 files touching 25 distinct legacy configs plus a VariantType change and a Cast dispatch change, and the pieces have very different risk profiles.

A split I would find easy to review, roughly in dependency order:

1. Per-scan Parquet fallback. CometScanRule.parquetFallbackReason plus the CometScanRuleSuite tests and the doc table. Fully self-contained, three configs, low risk. This one could go in almost as-is.

2. Session-wide fallback. LegacyConfFallback, both ShimLegacyConfFallback files, COMET_LEGACY_CONF_FALLBACK_ENABLED, the isCometLoaded hook, the CometSparkSessionExtensionsSuite test, and the compat doc section. Thirteen analyzer and optimizer configs, all with the same shape. Also self-contained.

3. CometCast mixing in CodegenDispatchFallback. This is the one I would most like to see on its own. It flips every Unsupported and non-opted-in Incompatible cast from "fall back to Spark" to "run Spark's doGenCode inside the Comet kernel". Cast is everywhere, so this is a broad behavior and performance change, and the test churn shows it: map to string casts lose expect_fallback, the CalendarIntervalType to StringType fallback assertion is deleted, and CometCastSuite swaps checkSparkAnswerAndFallbackReason for checkSparkAnswerAndOperator. As its own PR it could carry benchmark numbers and a clear before and after, and it would be revertable without losing the legacy-config work.

4. VariantType guard. The isVariantType shim, the CometCast guard, and the two dev/diffs hunks. Unrelated to legacy configs, and it stands on its own merits.

5. Per-expression legacy gates. These could go together or separately. I already noted on the individual files that In/InSet and ArrayInsert read as standalone fixes. The In/InSet one in particular is more than a legacy opt-in, since on 3.4 and 3.5 with ANSI off legacyNullInEmptyBehavior resolves to true by default, so it is fixing the default path. That deserves its own tests and its own line in the changelog rather than being folded into a legacy-config PR.

Note that 3 has to land before the castComplexTypesToString change in 5, since that change depends on the dispatcher being available on Cast. The timeParserPolicy gate needs rework anyway per my other comment, so it would naturally drop out of the first pass.

Happy to review each piece quickly as it comes. Splitting would also mean the low-risk parts (1 and 2) are not blocked on working out the Cast dispatch question.

(Comment drafted with LLM assistance.)

comphead · 2026-07-27T21:38:06Z

I'll detach cast as codegen as a separate PR

comphead · 2026-07-27T21:38:50Z

Also will remove the global Comet fallback for params

"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.readFileSourceTableCacheIgnoreOptions" -> "false"

and we need to address them separately

andygrove reviewed Jul 2, 2026

View reviewed changes

comphead marked this pull request as draft July 2, 2026 21:28

comphead force-pushed the chore branch 4 times, most recently from f5d2abc to 7125db0 Compare July 10, 2026 00:27

comphead marked this pull request as ready for review July 13, 2026 23:21

comphead requested review from andygrove and mbutrovich July 13, 2026 23:21

comphead added this to the 1.0.0 milestone Jul 14, 2026

andygrove reviewed Jul 15, 2026

View reviewed changes

Comment thread spark/src/main/scala/org/apache/comet/serde/predicates.scala Outdated

andygrove reviewed Jul 15, 2026

View reviewed changes

comphead force-pushed the chore branch from e51ee4d to 08472c8 Compare July 16, 2026 00:16

comphead mentioned this pull request Jul 16, 2026

feat: fall back to Spark for collated predicate operands #4948

Merged

comphead force-pushed the chore branch from 08472c8 to c2245c1 Compare July 18, 2026 17:18

comphead added 12 commits July 21, 2026 16:29

chore: fallback to Spark if legacy sql configurations are set

fb9ca26

chore: fallback to Spark if legacy sql configurations are set

a79ba6a

chore: fallback to Spark if legacy sql configurations are set

cb6a426

chore: fallback to Spark if legacy sql configurations are set

e84b42e

chore: fallback to Spark if legacy sql configurations are set

8f103de

chore: fallback to Spark if legacy sql configurations are set

2c88b04

chore: fallback to Spark if legacy sql configurations are set

fa48963

chore: fallback to Spark if legacy sql configurations are set

97a89b3

chore: fallback to Spark if legacy sql configurations are set

964a75d

chore: fallback to Spark if legacy sql configurations are set

db91685

chore: fallback to Spark if legacy sql configurations are set

4b2cde5

chore: fallback to Spark if legacy sql configurations are set

d348f3f

comphead added 10 commits July 21, 2026 16:29

chore: fallback to Spark if legacy sql configurations are set

dc9a371

chore: fallback to Spark if legacy sql configurations are set

b9f1890

chore: fallback to Spark if legacy sql configurations are set

e81fb82

chore: fallback to Spark if legacy sql configurations are set

a75411d

chore: fallback to Spark if legacy sql configurations are set

c035e4d

chore: fallback to Spark if legacy sql configurations are set

2e76ba8

chore: fallback to Spark if legacy sql configurations are set

3d3258c

chore: fallback to Spark if legacy sql configurations are set

bfe1a76

comphead force-pushed the chore branch from f6af863 to bfe1a76 Compare July 21, 2026 23:55

comphead requested a review from andygrove July 22, 2026 16:13

comphead added 2 commits July 22, 2026 16:27

chore: fallback to Spark if legacy sql configurations are set

5665fa1

chore: fallback to Spark if legacy sql configurations are set

ab8a7e8

comphead mentioned this pull request Jul 23, 2026

test: cover ArrayExists three-valued logic config #5000

Open

comphead added 2 commits July 23, 2026 08:47

chore: fallback to Spark if legacy sql configurations are set

2c7de49

chore: fallback to Spark if legacy sql configurations are set

c47d3fb

andygrove mentioned this pull request Jul 23, 2026

Audit Spark SQL configs that affect query semantics and ensure Comet honors them (or falls back) #4180

Open

21 tasks

chore: fallback to Spark if legacy sql configurations are set

90cb673

andygrove mentioned this pull request Jul 27, 2026

fix: fall back to Spark for negative-scale decimal casts and arithmetic #5050

Closed

comphead mentioned this pull request Jul 27, 2026

Native panic casting to negative-scale decimal when spark.sql.legacy.allowNegativeScaleOfDecimal=true #5013

Open

andygrove reviewed Jul 27, 2026

View reviewed changes

comphead mentioned this pull request Jul 28, 2026

feat: add codegen dispatch fallback for CometCast incompatible/unsupported cases (including legacy config paths) #5079

Merged

andygrove mentioned this pull request Jul 28, 2026

Triage the spark.sql.legacy.* configs dropped from the session-wide fallback in #4799 #5087

Open

		}

		object CometCount extends CometAggregateExpressionSerde[Count] {

Uh oh!

Conversation

comphead commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

How are these changes tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andygrove Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andygrove commented Jul 22, 2026

Uh oh!

andygrove commented Jul 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andygrove commented Jul 27, 2026

Uh oh!

comphead commented Jul 27, 2026

Uh oh!

comphead commented Jul 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

comphead commented Jul 2, 2026 •

edited

Loading

andygrove Jul 15, 2026 •

edited

Loading