andygrove commented on PR #5133:
URL:
https://github.com/apache/datafusion-comet/pull/5133#issuecomment-5441441165
> **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.
The fix itself is obviously right and the SQL coverage is in the correct
place. Two suggestions.
**The same gap exists for other types**
That `match data_type` block in `planner.rs` is a hand-maintained list, and
`Interval(MonthDayNano)` is not the only thing missing from it.
`Interval(YearMonth)` is the immediate one, since `YearMonthIntervalType` is
handled all over `CometBatchKernelCodegen*` and a null year-month literal would
hit the same `"{dt:?} is not supported in Comet"` error. `LargeUtf8`,
`Utf8View`, and `FixedSizeBinary` are others that would fail the same way.
Note that two arms of that match already do the right thing generically:
`DataType::Map(f, s) => DataType::Map(f, s).try_into()?` and the `List` arm
both go through DataFusion's `ScalarValue: TryFrom<&DataType>`, which returns a
typed null for essentially every type. Would it be better to collapse the whole
block to `ScalarValue::try_from(&data_type)` and keep explicit arms only where
Comet genuinely needs to differ from DataFusion? That closes this class of bug
permanently rather than one type at a time.
If you would rather keep this PR minimal, adding `Interval(YearMonth)`
alongside `MonthDayNano` here and filing an issue for the general cleanup would
also work.
**Test placement**
The two new queries go into `calendar_interval.sql`, which already covers
non-null calendar intervals. That is the right file. One thing to check: does
`SELECT CAST(NULL AS INTERVAL) FROM test_null_interval` actually reach the
native planner, or does Spark's `ConstantFolding` collapse it before
serialization? The comment on the `make_interval` case says `NullPropagation`
folds it to a null interval literal, which is exactly what we want to exercise,
so presumably yes. It would be worth confirming that the test fails on `main`
without the planner change, since a fixture that passes either way is not
protecting anything.
--
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]
