andygrove opened a new pull request, #5202: URL: https://github.com/apache/datafusion-comet/pull/5202
## Which issue does this PR close? Closes #5195. ## Rationale for this change `spark.comet.exceptionOnDatetimeRebase` was declared but never read. It published to the user-facing configs page and promised a safety guarantee Comet does not provide: a user who read the scan limitation in `compatibility/scans.md`, learned that Comet mis-reads legacy-calendar data, and set this config specifically to be protected from that, got silence and wrong results instead of the promised exception. This takes option 1 from the issue — wire it up — rather than removing it. The underlying scan limitation (implementing rebasing, or falling back per scan) is still tracked by #5010; this only makes the existing config mean what its doc string says. ## What changes are included in this PR? **Detection.** New `native/core/src/parquet/legacy_datetime.rs` decides whether a file's dates/timestamps were written in the legacy hybrid calendar, from the footer's key-value metadata. It mirrors the LEGACY arm of Spark's `DataSourceUtils.getRebaseSpec`: - `org.apache.spark.version` before `3.0.0` (string comparison, as Spark does) - an `org.apache.spark.legacyDateTime` key (`datetimeRebaseModeInWrite=LEGACY`) - an `org.apache.spark.legacyINT96` key (`int96RebaseModeInWrite=LEGACY`) - a writer in `[3.0.0, 3.1.0)` *and* the file has INT96 columns — Spark 3.0 predates `int96RebaseModeInWrite`, so it stamps no key and its INT96 values are hybrid-calendar Files with **no** `org.apache.spark.version` key were not written by Spark and carry no rebase signal, so they are not flagged. Spark falls back to `datetimeRebaseModeInRead` for those and raises only on a decoded ancient value; Comet has only the footer here, so failing every non-Spark Parquet file would make the config unusable. **Enforcement point.** The check runs in the Parquet reader factory's `get_metadata`, which is invoked for every file. It deliberately does *not* live in the schema/expression adapter: DataFusion only creates that when the logical and physical schemas differ or a predicate is pushed down, so a plain `SELECT d FROM ...` could skip it entirely. Since that factory now has two Comet-specific jobs, it is renamed `EagerPageIndexReaderFactory` -> `CometParquetFileReaderFactory` (`comet_parquet_reader_factory.rs`), with the module docs split into two sections and the upstream apache/datafusion#23978 revert note preserved. **Gating.** The check is armed only when the requested schema actually reads a date or timestamp, walking recursively through structs, lists, maps, dictionaries, and unions. This mirrors Spark, which raises only when it decodes a value that needs rebasing, and means a scan projecting only calendar-insensitive columns out of a legacy file is not failed. Pushed-down filter columns are covered because Spark's `requiredSchema` includes filter attributes. The config stays off by default, so there is no behavior change and no per-file cost unless a user opts in. **Plumbing.** `exception_on_legacy_datetime = 19` on `NativeScanCommon`, set in `CometNativeScan.convert`, threaded through `init_datasource_exec`. **Cleanup.** - Removed the dead `SparkParquetOptions::use_legacy_date_timestamp_or_ntz` field, which the issue identified as this config's apparent intended destination (only ever initialized to `false`, never consulted). - Moved the config from `CATEGORY_EXEC` to `CATEGORY_SCAN`, alongside `spark.comet.scan.unsignedSmallIntSafetyCheck` and `spark.comet.scan.allowDisabledParquetVectorizedReader`, and rewrote its doc string to describe failing rather than rebasing. - `scans.md` no longer describes the config as dead code and now documents what triggers the guard. ### Note on the error type No new `SparkError` variant is added. The rejection travels out as a `ParquetError`, which already classifies as `SparkError::CannotReadFile`, whose `message` carries the full Comet text to the JVM — and `FAILED_READ_FILE` is the same envelope Spark's `FileScanRDD` puts a read-time failure in. That avoided touching `error.rs` plus three `ShimSparkErrorConverter` shims for no user-visible gain. The tradeoff worth flagging for review: the outer error class reads as "corrupt file" rather than "calendar mismatch". Happy to promote it to its own error type in a follow-up if reviewers prefer that. What a user sees: ``` org.apache.spark.SparkException: [FAILED_READ_FILE.NO_HINT] Encountered error while reading file file:///.../part-00000-....snappy.parquet. SQLSTATE: KD001 Caused by: org.apache.spark.SparkException: Parquet error: Parquet error: this file was written using the legacy hybrid (Julian + Gregorian) calendar. Comet's native scan does not rebase dates/timestamps to the Proleptic Gregorian calendar, and so would return incorrect values for dates before 1582-10-15 and timestamps before 1900-01-01T00:00:00Z. This scan failed instead because spark.comet.exceptionOnDatetimeRebase is enabled. Set it to false to read these values as-is without rebasing, or disable Comet for this query so that Spark rebases them. ``` ## How are these changes tested? New `ParquetDatetimeRebaseSuite` (5 tests, passing on both Spark 4.1 and Spark 3.4): - legacy-calendar dates raise when enabled — via full read, a date-only projection, and a query that only *filters* on the date column - with the config disabled (the default), the file reads as `1000-01-06` — exactly the silently-wrong value from the issue report, so the wiring is provably a no-op unless opted into - a projection of only non-datetime columns does not raise - CORRECTED-written files (the common case) do not raise, so enabling the config costs nothing on normal data - legacy-calendar INT96 timestamps raise 10 new Rust unit tests cover the footer predicate (no metadata, non-Spark writer, pre-3.0 writer, modern writer with/without each legacy key, the Spark 3.0 INT96 case, a version key with no value) and the recursive date/timestamp schema walk. The INT96 probe is passed as a closure that panics in tests where it must not be consulted, pinning its laziness. Regression: `parquet` Rust module 95/95; `ParquetReadV1Suite` 51/51; `ParquetTimestampLtzAsNtzSuite`, `SparkErrorConverterSuite`, `CometConfSuite`, `CometPublicApiSuite`. `test-compile` clean on spark-3.4, spark-3.5, and spark-4.0; `cargo fmt`, `clippy -D warnings`, `spotless:check`, and `scalastyle:check` all clean. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
