fix: remove stale Interval -> Int64 arm from can_cast_types - #10939
Open
dylanpulver wants to merge 1 commit into
Open
fix: remove stale Interval -> Int64 arm from can_cast_types#10939dylanpulver wants to merge 1 commit into
dylanpulver wants to merge 1 commit into
Conversation
…st` cannot do
`can_cast_types` reported `true` for `Interval(YearMonth) -> Int64` and
`Interval(DayTime) -> Int64`, but `cast_with_options` has no such arm and returns
`CastError("Casting from Interval(YearMonth) to Int64 not supported")`.
Both sides were added together in apache#1196. apache#5769 made `IntervalDayTime` and
`IntervalMonthDayNano` structured types and deliberately removed the
`Interval <-> Int64` cast support, but left the `can_cast_types` arm behind.
This is not only an API-consistency issue. `cast/union.rs::resolve_child_array`
picks the first union child that `can_cast_types` accepts, so casting a union
containing an interval child to `Int64` selected the interval child and failed,
even when another child (e.g. `Utf8`) could actually be cast.
`arrow/tests/array_cast.rs::test_can_cast_types` exists to keep the two in sync
and did not catch this: `Int64` was missing from `get_all_types()`, where
`UInt64` appears in its place and then again in its own position. Restoring
`Int64` makes that test fail on main, and it is the only mismatch it exposes.
Rich-T-kid
suggested changes
Sep 1, 2026
Rich-T-kid
left a comment
Rich-T-kid
left a comment
Contributor
There was a problem hiding this comment.
Thank you for the fix @dylanpulver.
I think we can trim this PR a bit, I left some suggestions
Comment on lines
+324
to
+327
| // Note: there is deliberately no `(Interval(_), Int64)` arm. No interval unit has an | ||
| // unambiguous `i64` value (`YearMonth` is a count of months, `DayTime` packs days and | ||
| // milliseconds, `MonthDayNano` is 128 bits wide), and `cast_with_options` implements no | ||
| // such cast. Cast via `Duration` instead. |
Contributor
There was a problem hiding this comment.
I don't think this is strictly needed. we don't need to explain why certain cast don't exist.
if you think its still warranted I think we should shorted this.
Comment on lines
+12451
to
+12452
| #[test] | ||
| fn test_can_cast_interval_to_int64_matches_cast() { |
Contributor
There was a problem hiding this comment.
similar Idea for these test, I think we can add one test that asserts
can_cast_types()reports false for interval(yearMonth/dayTime) -> int64cast(yearMonth/dayTime,int64)fails with an error as expected
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
can_cast_typespromisedInterval(YearMonth) -> Int64andInterval(DayTime) -> Int64;cast_with_optionshas no such arm and errors. #5769 removed the cast side deliberately and left thecan_cast_typesarm behind, so this restores the intended state rather than adding a cast. The user-visible effect is incast/union.rs::resolve_child_array, which picks the first union childcan_cast_typesaccepts: a union with an interval child and aUtf8child failed to cast toInt64even though theUtf8child casts fine.If you would rather have
Interval -> Int64actually implemented, this is the wrong direction and I am happy to close it — that would be a feature rather than a fix.What changes are included in this PR?
The
(Interval(_), Int64)arm is removed fromcan_cast_types.arrow/tests/array_cast.rs::get_all_types()getsInt64back: it currently readsInt8, Int16, Int32, UInt64, UInt8, UInt16, UInt32, UInt64, soInt64was never used as a cast target. Line 568 of that file has the intended sequence.Are these changes tested?
Matched pair,
cargo test -p arrow --features="chrono-tz prettyprint" --test array_cast test_can_cast_types: passes on unmodified main; with onlyInt64restored and no source change it fails withfrom Interval(YearMonth) to Int64 but can_cast_types reported true; with both changes it passes. RestoringInt64exposed that one mismatch and no others.Two unit tests added in
arrow-cast:test_can_cast_interval_to_int64_matches_castassertscan_cast_types == cast(..).is_ok()for all three interval units rather than hard coding the answer, andtest_cast_union_to_int64_skips_uncastable_interval_childcovers the union path.Reverting the source with the tests in place fails both of them and the integration test. A half fix (
YearMonth => false,DayTime => true) still passes the union test but fails the enumerating one onInterval(DayTime), which is why the enumeration is there —test_can_cast_typespanics on the first mismatch and would only ever have shownYearMonth.cargo fmt --all -- --checkclean,cargo clippy -p arrow-cast --all-targets -- -D warningsclean,cargo test -p arrow-cast379 passed + 11 doc tests,--test array_cast10 passed. rustc 1.97.1, matchingrust-toolchain.toml.Not tested: full workspace, miri,
force_validate,--all-features, benchmarks. My sweep for other stale arms of this class was dispatch-level over null arrays, so value-level cast bugs were not covered.Are there any user-facing changes?
can_cast_types(Interval(YearMonth) | Interval(DayTime), Int64)returnsfalseinstead oftrue. Nothing could have relied on thetrue, since the cast always failed. Casting a union toInt64now succeeds in cases that previously errored.AI disclosure, per CONTRIBUTING: the investigation, the patch and the tests were produced with AI assistance (Claude Opus 5). The measurements quoted above were run against this branch.