kumarUjjawal commented on code in PR #24565:
URL: https://github.com/apache/datafusion/pull/24565#discussion_r3835052879
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -1617,6 +1630,38 @@ mod tests {
);
}
+ #[test]
+ fn test_coerced_from_run_end_encoded() {
Review 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.
##########
datafusion/optimizer/src/analyzer/type_coercion.rs:
##########
@@ -1083,6 +1083,8 @@ fn extract_window_frame_target_type(col_type: &DataType)
-> Result<DataType> {
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 {
Review 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.
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -1617,6 +1630,38 @@ mod tests {
);
}
+ #[test]
+ fn test_coerced_from_run_end_encoded() {
+ let run_end_encoded_of = |value_type: DataType| {
+ DataType::RunEndEncoded(
+ Field::new("run_ends", DataType::Int32, false).into(),
+ Field::new("values", value_type, true).into(),
+ )
+ };
+
+ let type_into = run_end_encoded_of(DataType::UInt32);
+ let type_from = DataType::Int64;
+ assert_eq!(coerced_from(&type_into, &type_from), None);
+
+ let type_from = run_end_encoded_of(DataType::UInt32);
+ let type_into = DataType::Int64;
+ assert_eq!(
+ coerced_from(&type_into, &type_from),
+ Some(type_into.clone())
+ );
+
+ // Signature candidates for functions like `date_bin` are plain
+ // 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));
Review Comment:
I think this will fail at formatting, make sure to run `cargo fmt --all`
--
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]
