fix: support RunEndEncoded arrays in function and window frame type coercion - #24565
fix: support RunEndEncoded arrays in function and window frame type coercion#24565thorfour wants to merge 2 commits into
Conversation
coerced_from() already special-cases Dictionary(_, value_type) so that functions declared with the old-style Exact/OneOf signature (which don't go through the newer Coercible/TypeSignatureClass system) match against the dictionary's value type. RunEndEncoded had no equivalent case, so any such function (e.g. date_bin) failed to plan when given a RunEndEncoded-wrapped column, even though the same column as a plain Dictionary would have matched fine. Add the symmetric RunEndEncoded case, unwrapping to the value field's type before recursing, mirroring the existing Dictionary handling.
extract_window_frame_target_type() already recurses through Dictionary(_, value_type) to find the natural type for a RANGE window frame bound, but had no equivalent case for RunEndEncoded, so ordering a RANGE window by a REE-dict-encoded column (the same column as a plain Dictionary would work) failed with "Cannot run range queries on datatype: RunEndEncoded(...)". Add the symmetric RunEndEncoded case, mirroring the existing Dictionary handling.
github-actions
Bot
added
logical-expr
kumarUjjawal
left a comment
kumarUjjawal
left a comment
There was a problem hiding this comment.
Thank you @thorfour for working on this. I have left few comments please take a look.
| Ok(DataType::Interval(IntervalUnit::MonthDayNano)) | ||
| } else if let DataType::Dictionary(_, value_type) = col_type { | ||
| extract_window_frame_target_type(value_type) | ||
| } else if let DataType::RunEndEncoded(_, value_type) = col_type { |
There was a problem hiding this comment.
Unwrapping REE here also lets RANGE <offset> PRECEDING/FOLLOWING past planning. An offset frame adds the offset to each ordering value, and a value pulled from an REE column stays run-end wrapped, which arrow's arithmetic kernels don't handle. Keeping the plan time error until scalar arithmetic unwraps REE would be better.
| } | ||
|
|
||
| #[test] | ||
| fn test_coerced_from_run_end_encoded() { |
There was a problem hiding this comment.
This exercises coerced_from directly, but nothing runs date_bin over an actual REE column, which is what the issue reports. The window frame change has no coverage at all. run_end_encoded.slt already has the arrow_cast scaffolding to build REE columns.
Also both positive cases go from REE to a plain type. Coercing a plain type into an REE target is only tested negatively, so that arm's success path stays uncovered.
| // Timestamp, but a REE-encoded column (e.g. a segment written with | ||
| // REE-dict encoding for that field) should still coerce against | ||
| // them via the wrapped value type. | ||
| let type_from = run_end_encoded_of(DataType::Timestamp(TimeUnit::Nanosecond, None)); |
There was a problem hiding this comment.
I think this will fail at formatting, make sure to run cargo fmt --all
kumarUjjawal
changed the title
Jefffrey
left a comment
Jefffrey
left a comment
There was a problem hiding this comment.
as mentioned would be good to have an end to end test via SLT
| // coerced run-end encoded the same way as dictionary: callers of a | ||
| // signature built from `Exact`/`OneOf` only know the logical value | ||
| // type, not that the argument happens to be physically run-encoded. |
There was a problem hiding this comment.
| // coerced run-end encoded the same way as dictionary: callers of a | |
| // signature built from `Exact`/`OneOf` only know the logical value | |
| // type, not that the argument happens to be physically run-encoded. |
probably not necessary to explain this
3 participants
Which issue does this PR close?
coerced_fromandextract_window_frame_target_typedoesn't support RunEndEncoded arrays #24564.Rationale for this change
Adds missing support for coercing REE arrays and extracting range windows from REE arrays.
What changes are included in this PR?
Added case statements for supporting REE arrays
Are these changes tested?
Yes a unit test is included.
Are there any user-facing changes?
No