You are viewing a plain text version of this content. The canonical link for it is here.

Posted to github@datafusion.apache.org by "peterxcli (via GitHub)" <gi...@apache.org> on 2026/07/29 15:37:54 UTC

[PR] fix: support null calendar interval literals [datafusion-comet]

peterxcli opened a new pull request, #5133:
URL: https://github.com/apache/datafusion-comet/pull/5133

   ## Which issue does this PR close?
   
   Closes #5058.
   
   ## Rationale for this change
   
   `CalendarIntervalType` is supported as Arrow `Interval(MonthDayNano)`, but the native planner did not construct a null scalar for that type. As a result, null calendar interval literals passed the JVM support check and then failed during native planning.
   
   ## What changes are included in this PR?
   
   Map null `Interval(MonthDayNano)` literals to `ScalarValue::IntervalMonthDayNano(None)` in the native planner.
   
   ## How are these changes tested?
   
   The existing `calendar_interval.sql` fixture covers `CAST(NULL AS INTERVAL)` and requires native execution.
   
   - `cargo fmt --manifest-path native/Cargo.toml --all --check`
   - `make core`
   - `./mvnw test -Dtest=none -Dsuites="org.apache.comet.CometSqlFileTestSuite calendar_interval" -Dscalastyle.skip=true` with JDK 17 (1 test passed)
   


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] fix: support null calendar interval literals [datafusion-comet]

Posted by "peterxcli (via GitHub)" <gi...@apache.org>.

peterxcli commented on PR #5133:
URL: https://github.com/apache/datafusion-comet/pull/5133#issuecomment-5441852668

Verified the test protects the fix: with the `Interval(MonthDayNano)` arm removed and everything else unchanged, `CometSqlFileTestSuite calendar_interval` fails with exactly the issue's error (`Interval(MonthDayNano) is not supported in Comet`), and passes with it. So `ConstantFolding` doesn't fold the literal away — it reaches the native planner.

On the other types: a null `Interval(YearMonth)` literal can't reach this match today. `CometLiteral.getSupportLevel` gates on `supportedDataType`, which doesn't include `YearMonthIntervalType`, so those literals fall back to Spark rather than failing natively. I'd rather add that arm in #5161, where the type actually becomes reachable. `LargeUtf8`/`Utf8View`/`FixedSizeBinary` can't appear here at all — `to_arrow_datatype` never produces them from the proto. So after this PR the explicit match covers every type the serde can emit, which is the state I wanted before considering the `try_from` collapse (per the earlier review round).

--
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] fix: support null calendar interval literals [datafusion-comet]

Posted by "sunchao (via GitHub)" <gi...@apache.org>.

sunchao merged PR #5133:
URL: https://github.com/apache/datafusion-comet/pull/5133


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] fix: support null calendar interval literals [datafusion-comet]

Posted by "peterxcli (via GitHub)" <gi...@apache.org>.

peterxcli commented on PR #5133:
URL: https://github.com/apache/datafusion-comet/pull/5133#issuecomment-5149456472

   @andygrove thanks for the review. added sql tests


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] fix: support null calendar interval literals [datafusion-comet]

Posted by "andygrove (via GitHub)" <gi...@apache.org>.

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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix: support null calendar interval literals [datafusion-comet]

Posted by "peterxcli (via GitHub)" <gi...@apache.org>.

peterxcli commented on PR #5133:
URL: https://github.com/apache/datafusion-comet/pull/5133#issuecomment-5149422199

   > 2. This match is the bug, not the missing arm
   
   I think we should keep the explicit matching here, whenever we have checked we've supported all types, then we can switch to use `ScalarValue::try_new_null`. switch back...


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org