apache / apache/datafusion-comet

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

Open
#4,180 2 comments 0 reactions 0 assignees View on GitHub
area:expressions area:scan enhancement priority:medium
Dominant language
Scala
Stars
1.3k
Forks
373
Avg merge
2d 4h
Merged PRs (30d)
198

Description

## What is the problem the feature request solves?

**yes, this is all AI generated ... I asked Claude to find all the Spark configs that we should ensure Comet honors**

Spark exposes hundreds of configuration options, many of which alter query semantics (result correctness, not just performance). Comet's test matrix almost exclusively runs with default configs, which means non-default settings may silently produce results that diverge from Spark.

A user running Spark with, e.g., `spark.sql.legacy.timeParserPolicy=LEGACY` or `spark.sql.parquet.datetimeRebaseModeInRead=LEGACY` has no guarantee that Comet will produce the same results as vanilla Spark. In the worst case, results are silently wrong with no warning.

Today, Comet reads and respects only a small subset of semantic configs:

| Config | Handling |
|---|---|
| `spark.sql.ansi.enabled` | Native (cast eval mode) |
| `spark.sql.session.timeZone` | Native (Parquet + timestamp expressions) |
| `spark.sql.parquet.int96AsTimestamp` | Native |
| `spark.sql.parquet.int96TimestampConversion` | Disables Comet entirely when `true` |
| `spark.sql.caseSensitiveAnalysis` | Native (Parquet) |
| `spark.sql.parquet.filterPushdown` (+ subflags) | Native |
| `spark.sql.nestedSchemaPruningEnabled` | Native |
| `spark.sql.decimalOperationsAllowPrecisionLoss` | Native |
| Non-default string collations (Spark 4.0+) | Selective operator fallback |

Everything else is effectively "hope the default works for you."

## Describe the potential solution

Audit all semantic-affecting Spark SQL configs and, for each one, pick one of three states:

1. **Respected natively** — Comet reads the config and produces Spark-compatible results. Covered by tests that run with non-default values.
2. **Fallback trigger** — Non-default value disables Comet for the affected query (or entirely, like `int96TimestampConversion`).
3. **Known-safe to ignore** — Config doesn't change results in code paths Comet runs.

Each config needs an explicit decision, a test, and documentation. Below is a starting inventory organized by risk.

### Tier 1 — Correctness bombs (silent wrong results)

These most likely produce divergent results today:

- [ ] `spark.sql.parquet.datetimeRebaseModeInRead` (EXCEPTION / LEGACY / CORRECTED) — Julian↔Gregorian rebase for pre-1582 dates. Comet has `spark.comet.exceptionOnDatetimeRebase` but it's unclear whether all three modes are honored end-to-end.
- [ ] `spark.sql.parquet.int96RebaseModeInRead` — same as above for INT96.
- [ ] `spark.sql.parquet.datetimeRebaseModeInWrite` / `int96RebaseModeInWrite` — write path.
- [ ] `spark.sql.legacy.timeParserPolicy` (CORRECTED / LEGACY / EXCEPTION) — changes `to_date`, `to_timestamp`, `unix_timestamp`, CSV/JSON date parsing entirely (SimpleDateFormat vs DateTimeFormatter).
- [ ] `spark.sql.parquet.binaryAsString` — changes returned schema (binary columns read as string).

### Tier 2 — Function / expression-level semantics

- [ ] `spark.sql.legacy.sizeOfNull` — `size(null)` returns `-1` vs `null`.
- [ ] `spark.sql.legacy.allowNegativeScaleOfDecimal` — decimal parsing and arithmetic.
- [ ] `spark.sql.legacy.typeCoercion.datetimeToString.enabled` — implicit date↔string coercion in concat/coalesce.
- [ ] `spark.sql.legacy.allowCastNumericToTimestamp` — `CAST(bigint AS timestamp)` permitted or not.
- [ ] `spark.sql.legacy.followThreeValuedLogicInArrayExists` — `exists(array, pred)` null handling.
- [ ] `spark.sql.function.concatBinaryAsString` — `concat(binary, binary)` return type.
- [ ] `spark.sql.function.eltOutputAsString` — same for `elt()`.
- [ ] `spark.sql.legacy.integerGroupingIdEnabled` — `grouping_id()` return type.
- [ ] `spark.sql.legacy.json.allowEmptyString.enabled` — JSON parsing behavior.

### Tier 3 — Schema / type behavior

- [ ] `spark.sql.legacy.charVarcharAsString` — CHAR/VARCHAR padding/truncation.
- [ ] `spark.sql.readSideCharPadding` — CHAR(n) read-side padding.
- [ ] `spark.sql.legacy.createEmptyCollectionUsingStringType` — empty `array()` / `map()` element type.

### Tier 4 — Already read, but verify coverage

Confirm that these are honored in all Comet code paths, not just the ones where they were originally wired up:

- [ ] `spark.sql.ansi.enabled` — beyond cast, does ANSI propagate to division-by-zero, integer overflow in `+`/`-`/`*`, array index out-of-bounds, etc.?
- [ ] `spark.sql.caseSensitiveAnalysis` — confirmed for Parquet scan; what about nested struct field access in expressions?
- [ ] `spark.sql.session.timeZone` — confirmed for Parquet and casts; verify for all temporal expressions (`date_format`, `from_utc_timestamp`, `to_utc_timestamp`, `date_trunc`, etc.).
- [ ] `spark.sql.decimalOperationsAllowPrecisionLoss` — confirmed for the serde path; verify arithmetic kernels match.

### Suggested process per config

For each item:

1. Identify the Spark code paths that branch on the config.
2. Check whether Comet's corresponding code path reads the config.
3. Add a test that runs with the non-default value and asserts Spark-vs-Comet parity (or explicit fallback).
4. Decide: native-respect, fallback-trigger, or known-safe-to-ignore. Document the decision.

### Optional: config whitelist gate

Once the audit is done, we could add an opt-in mode (e.g., `spark.comet.strictConfigCompatibility=true`) that falls back to Spark whenever a semantic config is set to a non-default value not on the whitelist. This would give users a way to trade coverage for correctness guarantees without forcing the tradeoff on everyone.

## Additional context

Motivation: the current "default configs only" testing posture is a significant compatibility risk as Comet sees broader adoption. Users routinely tune `legacy.timeParserPolicy`, rebase modes, and ANSI behavior in production — these are not edge cases.

### Describe the potential solution

_No response_

### Additional context

_No response_

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.