apache / apache/datafusion-comet
Audit Spark SQL configs that affect query semantics and ensure Comet honors them (or falls back)
- 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
Assessment
This issue has not been assessed yet.