peterxcli commented on PR #5048: URL: https://github.com/apache/datafusion-comet/pull/5048#issuecomment-5447340747
Thanks @andygrove — all four points were worth acting on. Addressed in 2323bae3a (and the branch is now merged up to main, which also resolves the conflict with #5490's `output_schema` proto field). **1. Plan-time footer reads** The earlier round had already moved the check to `SKIP_ROW_GROUPS` footer reads with a bounded 8-way pool and early exit on the first legacy footer. This round adds the three things you asked about: - **Caching**: footer facts (the Spark version key and the two legacy markers) are now cached per `(path, length, modificationTime)` in a bounded LRU (32K entries). The facts are independent of read modes and requested types, so one entry answers the rebase question for any query — AQE re-planning and repeated queries over the same file set no longer pay any I/O. Only never-before-seen files read a footer. - **Opt-out**: new config `spark.comet.scan.parquet.checkDatetimeRebase` (default `true`). Users who know their data is corrected can disable the check and get the previous behavior. - **Numbers**: 2,000 single-row files on a local SSD: cold check ~10.9s (~5.4ms/file at 8-way parallelism), fully-cached re-check 16–57ms. Object storage will be latency-bound (roughly one to two footer GETs per file across 8 threads), which is exactly why the cache and the opt-out exist. I've added the numbers to the PR description. On the pre-DPP file set (from your earlier comment): `selectedPartitions` is indeed the superset before dynamic pruning, but plan time is the only place a fallback decision can be made, and checking a superset errs in the conservative direction; with the cache the extra files are read once, ever. **2. LEGACY write rebase mode** Correct — there was no check, and the write path silently ignored `spark.sql.parquet.datetimeRebaseModeInWrite`. One correction to the failure scenario as stated: since the native writer writes corrected values *and* stamps a corrected version with no legacy markers, the file it produces is self-consistent — a Spark 3.x+ reader gets correct values back. The real problems are silently ignoring an explicit user config, and downstream legacy readers (Spark 2.x, Hive) that LEGACY exists to serve reading shifted values. Either way the fix is the same: `CometDataWritingCommand.getSupportLevel` now returns `Unsupported` when the effective write rebase mode is LEGACY (`datetimeRebaseModeInWrite`, plus `int96RebaseModeInWrite` when the schema has timestamps) and the output schema contains dates or timestamps. TIMESTAMP_NTZ is exempt because Spark never rebases NTZ on write. Deliberately not gated: EXCEPTION, the default. Spark under EXCEPTION writes the same corrected bytes as CORRECTED and only raises when it encounters a pre-Gregorian value; falling back on EXCEPTION would disable native datetime writes under default configs entirely, and the raise is data-dependent so it can't be detected at plan time. The remaining divergence is narrow — Comet writes a correct, correctly-stamped value where Spark would raise on an ancient one. I can file a follow-up for a native-side ancient-value check if you want EXCEPTION parity. **3. The TimestampNTZ condition** Your guess is exactly right, and the constant I used made it look worse than it is. TIMESTAMP_NTZ never needs rebasing by itself: Spark neither rebases NTZ on write (`ParquetWriteSupport` emits the raw micros) nor on read (`ParquetVectorUpdaterFactory`: "TIMESTAMP_NTZ is a new data type and has no legacy files that need to do rebase"). The rebase question only arises when a Parquet TIMESTAMP (LTZ or INT96) column is *read as* NTZ, which Comet permits only on Spark 4.x. The precise gate for that permission is `COMET_ALLOW_TIMESTAMP_LTZ_AS_NTZ` (SPARK-47447), not `COMET_SCHEMA_EVOLUTION_ENABLED` — they happen to carry the same per-version values, which is why the tests passed, but the schema-evolution constant is about widening primitive promotions and was the wrong name for this. Switched to `COMET_ALLOW_TIMESTAMP_LTZ_AS_NTZ` with a comment explaining the above. **4. The version comparison** Added a comment noting that `v < minVersion` deliberately mirrors the lexicographic string comparison in Spark's `DataSourceUtils.datetimeRebaseSpec`. Tests: the three `ParquetReadV1Suite` rebase tests (now also covering the cache, the early exit, and the opt-out config), a new `CometParquetWriterSuite` test asserting LEGACY write mode falls back while CORRECTED stays native, the full `CometParquetWriterSuite` (34/34) as a regression check on the write gate, and the native `parquet_writer` cargo tests all pass locally. -- 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]
