andygrove commented on PR #5161:
URL:
https://github.com/apache/datafusion-comet/pull/5161#issuecomment-5441509899
> **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.
Nice, targeted change. The reasoning for putting the exception in
`CometScanTypeChecker` rather than the shared `DataTypeSupport` predicate is
well argued, and the extra `IntVal` arm for `Interval(YearMonth)` plus the
improved error message are the right supporting pieces. Extending the Parquet
test to all 13 ANSI interval ranges is thorough.
A few questions.
**The new case short-circuits every other check**
```scala
dt match {
case _: YearMonthIntervalType | _: DayTimeIntervalType => true
case ShortType if ... =>
case dt if isStringCollationType(dt) =>
...
}
```
Putting the interval arm first means these two types skip every subsequent
guard in `isTypeSupported`. That is almost certainly fine today, since an
interval is not a `ShortType`, not a collated string, and not complex. But it
reads as "intervals are exempt from all checks" rather than "intervals are
supported", and if a future check needs to apply to all leaf types it will
silently not apply to these. Would it be better to place the arm at the end,
just before the default, so it reads as the fallthrough it actually is?
**Only V1 is tested**
`ansi_interval_scan.sql` sets `spark.sql.sources.useV1SourceList=parquet`,
so the fixture covers `FileSourceScanExec` and not `CometBatchScanExec`.
`CometScanTypeChecker` is used on the V2 path too. Was V2 tested? If the answer
is "it works but the fixture pins V1 for determinism", saying so in a comment
would help. If V2 is untested, it is worth adding, since the partition-value
serialization path in particular differs between the two.
**Is the year-month literal path exercised beyond partition values?**
The description says year-month literal serialization is needed for
partition columns and year-month predicates. The SQL fixture covers partition
columns. Does anything cover a predicate, for example `WHERE ym > INTERVAL
'1-0' YEAR TO MONTH`, running natively? That is the case where a wrong literal
encoding gives a wrong answer rather than an error, so it is worth pinning
explicitly.
**Docs formatting**
The `datatypes.md` table rows changed length but the separator row did not.
Prettier will want to realign that table, and prettier drift fails CI. Worth
running `make format` plus prettier before pushing so this does not come back
as a CI failure unrelated to the change.
--
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]
