apache / apache/datafusion-comet

Datetime rebase: track the documented scan limitation, and spark.comet.exceptionOnDatetimeRebase is dead code

Open
#5,010 4 comments 0 reactions 1 assignee Claimed by @peterxcli View on GitHub
area:scan bug correctness documentation priority:medium
Dominant language
Scala
Stars
1.3k
Forks
373
Avg merge
2d 4h
Merged PRs (30d)
198

Description

### Describe the bug

**Updated after filing:** the core limitation here is already documented in the user guide, in [`compatibility/scans.md`](https://github.com/apache/datafusion-comet/blob/main/docs/source/user-guide/latest/compatibility/scans.md) under "The following limitation may produce incorrect results without falling back to Spark":

> No support for datetime rebasing. When reading Parquet files containing dates or timestamps written before Spark 3.0 (which used a hybrid Julian/Gregorian calendar), dates/timestamps will be read as if they were written using the Proleptic Gregorian calendar. This may produce incorrect results for dates before October 15, 1582.

This issue is retained as the tracking issue for that limitation, which the doc does not currently link to. #1638 previously tracked the same gap for `native_iceberg_compat` and was closed as obsolete when that reader was removed; the gap still exists in the current native scan. The INT96 variant was filed separately as #5011 and closed as a duplicate of this issue.

Two aspects are not covered by the documented text and are the actionable part of this issue.

#### 1. `spark.comet.exceptionOnDatetimeRebase` is dead code

The config is declared at `CometConf.scala:695`, is not `.internal()`, and is in `CATEGORY_EXEC`, so it publishes to the user-facing configs page. Its doc string promises:

> When this is true, Comet will throw exceptions when seeing these dates/timestamps that were written by Spark version before 3.0.

It is not read anywhere else in the tree. `SparkParquetOptions::use_legacy_date_timestamp_or_ntz` (`native/core/src/parquet/parquet_support.rs:78`), which appears to be its intended destination, is only ever initialized to `false` and never consulted. A user who sets the config to `true` specifically to be protected from this limitation gets silence and wrong results instead.

Either wire the config up so it raises on legacy-calendar data, or remove it so it stops advertising a guarantee that does not exist.

#### 2. The documented scope is narrower than the actual behavior

* The doc says "written before Spark 3.0". Any current Spark writes legacy-calendar data when `spark.sql.parquet.datetimeRebaseModeInWrite=LEGACY` is set, and Comet mis-reads those files too. The repro below uses Spark 4.1 as the writer.
* The doc implies visibly-wrong dates. In practice filters and aggregates over the affected column silently return the wrong answer, which is harder to notice than an obviously-shifted date.
* `spark.sql.parquet.datetimeRebaseModeInRead` / `spark.sql.legacy.parquet.datetimeRebaseModeInRead` are not mentioned. Comet reads neither, and also ignores the `org.apache.spark.legacyDateTime` file metadata that takes precedence over them in Spark.

### Steps to reproduce

```scala
val path = "/tmp/legacy_dates"

// Written by Spark 4.1, not by a pre-3.0 Spark.
spark.conf.set("spark.sql.parquet.datetimeRebaseModeInWrite", "LEGACY")
spark.sql("SELECT cast(s as date) as d FROM VALUES ('1000-01-01'),('1990-01-01') AS v(s)")
.write.mode("overwrite").parquet(path)
spark.conf.unset("spark.sql.parquet.datetimeRebaseModeInWrite")

spark.conf.set("spark.comet.enabled", "false")
spark.read.parquet(path).show()
// 1000-01-01
// 1990-01-01

spark.conf.set("spark.comet.enabled", "true")
spark.read.parquet(path).show()
// 1000-01-06 <-- shifted by the Julian/Gregorian offset
// 1990-01-01
```

Setting `spark.comet.exceptionOnDatetimeRebase=true` changes nothing.

Silent divergence in filters and aggregates, not just projection:

```sql
SELECT count(*) FROM parquet.`/tmp/legacy_dates` WHERE d = date'1000-01-01'
-- spark: 1
-- comet: 0

SELECT min(d), max(d) FROM parquet.`/tmp/legacy_dates`
-- spark: 1000-01-01, 1990-01-01
-- comet: 1000-01-06, 1990-01-01
```

`TIMESTAMP_MICROS` written the same way behaves the same (`1000-01-01 12:34:56` reads back as `1000-01-06 12:41:58`). `spark.comet.scan.enabled=false` makes every case above match, isolating this to the native scan.

### Expected behavior

For the underlying limitation, either of:

1. the native scan rebases legacy-calendar dates/timestamps as Spark does, honouring the file metadata first and `spark.sql.parquet.datetimeRebaseModeInRead` when it is absent; or
2. Comet falls back to Spark for scans of files carrying `org.apache.spark.legacyDateTime` and for non-default values of the read config.

Independently of which is chosen, `spark.comet.exceptionOnDatetimeRebase` should either work or be removed, and `scans.md` should be widened as described above.

### Additional context

* Reproduced on `apache/main` @ `0761e549a`, default Maven profile (Spark 4.1.2), macOS aarch64, debug build.
* Write-side rebase (`datetimeRebaseModeInWrite`, `int96RebaseModeInWrite`) is unaffected in the default configuration, since `spark.comet.parquet.write.enabled` defaults to `false`.
* Found while auditing the config inventory in #4180.

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.