andygrove commented on PR #5048:
URL:
https://github.com/apache/datafusion-comet/pull/5048#issuecomment-5441484402
> **Note on this review:** this was generated by an LLM (Claude Code) at my
request while I worked through a review backlog. I have not verified the
individual findings myself. Please treat everything below as suggestions to
evaluate rather than as authoritative review feedback, and push back on
anything that is wrong or already handled.
Trading a silent wrong answer for a fallback is the right call, and I like
that the fallback decision mirrors Spark's own
`DataSourceUtils.datetimeRebaseSpec` closely, including the string comparison
against `"3.0.0"` and the treatment of a missing version key. Stamping
`org.apache.spark.version` from the native writer is a necessary companion
change and it is good that it is here rather than a follow-up.
Three concerns.
**Every Parquet footer is now read on the driver at planning time**
`requiresDatetimeRebase` opens every selected file's footer before the scan
is converted, on any query whose required schema contains a `DATE` or
`TIMESTAMP`. The short circuit helps when a legacy file is found early, but the
common case is that no file needs rebasing, and then all of them are read. On a
table with tens of thousands of files in object storage, eight threads doing a
footer read each is going to add a lot of planning latency to queries that
previously did no driver-side I/O at all.
Some things that would help:
- How long does this add for a realistically large table? A number in the
description would let us judge whether this is fine or not.
- Is the result cached anywhere? `CometScanRule` runs per query execution,
and AQE re-planning can invoke it again. Paying this repeatedly for the same
file set would be worse than paying it once.
- Should there be a config to skip the check for users who know their data
is corrected, accepting the previous behavior? Given the fallback is
conservative, an opt-out seems reasonable.
**What happens when the write rebase mode is `LEGACY`?**
The native writer now unconditionally stamps `org.apache.spark.version` and
writes corrected values. If a user has
`spark.sql.parquet.datetimeRebaseModeInWrite=LEGACY`, Spark would write rebased
values and mark the file with `org.apache.spark.legacyDateTime`. As far as I
can see from this diff, Comet's native write path ignores that config, writes
corrected values, and stamps a version that says "corrected". A later reader
would then trust the marker and read the wrong values with no error.
Does `CometDataWritingCommand` already fall back when the write rebase mode
is not `CORRECTED`? If it does, pointing at that check would settle it. If it
does not, that is a data-corruption path and it should either be fixed here or
filed with a clear issue, because this PR is what makes the version marker
load-bearing.
**The `TimestampNTZType` condition**
```scala
val hasTimestamp =
SupportLevel.containsType(scanExec.requiredSchema, classOf[TimestampType])
||
(COMET_SCHEMA_EVOLUTION_ENABLED &&
SupportLevel.containsType(scanExec.requiredSchema,
classOf[TimestampNTZType]))
```
Tying the NTZ case to schema evolution is not obvious. Is the reasoning that
NTZ can only come from an INT96 physical column when schema evolution lets a
`TIMESTAMP` be read as `TIMESTAMP_NTZ`? If so, a comment saying that would help
a lot, because on its face the rebase question and the schema-evolution config
are unrelated and a future reader is likely to "simplify" this.
**One smaller thing**
The version comparison `v < minVersion` is a lexicographic string compare,
which would misclassify a hypothetical Spark 10.x. That is exactly what Spark
does, so matching it is correct. Worth a one-line comment saying it
deliberately mirrors `DataSourceUtils`, otherwise it looks like a bug someone
will "fix".
--
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]
