fix: support null calendar interval literals - #5133
Conversation
andygrove
left a comment
There was a problem hiding this comment.
Thanks for the quick fix. The arm itself is right: Interval(MonthDayNano) is what CalendarIntervalType maps to, and ScalarValue::IntervalMonthDayNano(None) is the matching null scalar. Two things I would like to sort out before this merges.
1. I do not think the cited fixture covers this
The PR says calendar_interval.sql covers CAST(NULL AS INTERVAL) and requires native execution. The text is in there, but it sits inside SELECT * FROM VALUES (...), so the null interval arrives as a row value through the local table scan rather than as a null literal in a projection. That is exactly the distinction #5058 draws: interval values flowing through a LocalTableScan were already fine, and it is the literal path that hit the catch-all. The fixture is unmodified here and CI on main is green, so it must be passing without this change, which means nothing in the repo would catch a regression.
Could you add the repro from the issue? It needs a projection over a real scan so the literal is serialized and the query runs natively, something like:
statement
CREATE TABLE test_null_interval(id int) USING parquet
statement
INSERT INTO test_null_interval VALUES (1)
query
SELECT CAST(NULL AS INTERVAL) FROM test_null_intervalThe NullPropagation route from the issue is worth a line too, since that is how this gets hit in practice rather than by someone writing a null cast by hand:
query
SELECT make_interval(NULL, 2, 3, 4, 5, 6, 7.008009) FROM test_null_interval2. This match is the bug, not the missing arm
native/core/src/execution/planner.rs
The null branch enumerates types by hand, and every time a type is added to the JVM support checks someone has to remember to come back here. That is how we got here, and the failure mode is bad: the type passes isSupportedDataType, the plan is accepted, and it blows up in native planning instead of falling back.
DataFusion already has exactly this function. ScalarValue::try_new_null(&data_type) covers every arm currently in the match, including Struct, Map, List, Time64(Nanosecond), Duration(Microsecond), Decimal128, Timestamp with timezone, and Null, and it covers all three interval units on top of that. Its Struct arm builds the same null struct array that ScalarStructBuilder::new_null gives us. So the whole block could collapse to one call and this class of bug goes away:
let scalar_value = if literal.is_null {
ScalarValue::try_new_null(&data_type)?
} else {
...One thing to decide if you take this: unsupported types would then report DataFusion's Can't create a null scalar from data_type "..." instead of our ... is not supported in Comet. A map_err would keep the Comet wording if you want it.
This also matters for your #5161, which adds an Interval(YearMonth) arm to this same match plus the IntervalUnit import. As written the two PRs will conflict, and switching to try_new_null lets both drop their arm. For what it is worth I checked the third unit and DayTimeIntervalType maps to DurationVector, so Duration(Microsecond) already covers it and YearMonth was the only other gap.
If you would rather keep the explicit match, that is a defensible call, but then please add the Interval(YearMonth) arm here as well so this file stops being a source of hard failures one type at a time.
I think we should keep the explicit matching here, whenever we have checked that all types are supported, then we can switch to use |
|
@andygrove thanks for the review. added sql tests |
Which issue does this PR close?
Closes #5058.
Rationale for this change
CalendarIntervalTypeis supported as ArrowInterval(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 toScalarValue::IntervalMonthDayNano(None)in the native planner.How are these changes tested?
The existing
calendar_interval.sqlfixture coversCAST(NULL AS INTERVAL)and requires native execution.cargo fmt --manifest-path native/Cargo.toml --all --checkmake core./mvnw test -Dtest=none -Dsuites="org.apache.comet.CometSqlFileTestSuite calendar_interval" -Dscalastyle.skip=truewith JDK 17 (1 test passed)