findepi commented on code in PR #13452:
URL: https://github.com/apache/datafusion/pull/13452#discussion_r1845466179
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -1138,27 +1138,44 @@ fn numeric_string_coercion(lhs_type: &DataType,
rhs_type: &DataType) -> Option<D
}
}
+fn coerce_list_children(lhs_field: &FieldRef, rhs_field: &FieldRef) ->
Option<FieldRef> {
+ Some(Arc::new(
+
Arc::unwrap_or_clone(Arc::clone(lhs_field)).with_data_type(comparison_coercion(
Review Comment:
@alamb can there be a problem if we coerce two lists and they have different
field _names_?
@blaginin the resulting field nullability should be OR of nullability of
sources
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -1138,27 +1138,44 @@ fn numeric_string_coercion(lhs_type: &DataType,
rhs_type: &DataType) -> Option<D
}
}
+fn coerce_list_children(lhs_field: &FieldRef, rhs_field: &FieldRef) ->
Option<FieldRef> {
+ Some(Arc::new(
+
Arc::unwrap_or_clone(Arc::clone(lhs_field)).with_data_type(comparison_coercion(
+ lhs_field.data_type(),
+ rhs_field.data_type(),
+ )?),
+ ))
+}
+
/// Coercion rules for list types.
fn list_coercion(lhs_type: &DataType, rhs_type: &DataType) -> Option<DataType>
{
use arrow::datatypes::DataType::*;
match (lhs_type, rhs_type) {
- (List(_), List(_)) => Some(lhs_type.clone()),
- (LargeList(_), List(_)) => Some(lhs_type.clone()),
- (List(_), LargeList(_)) => Some(rhs_type.clone()),
- (LargeList(_), LargeList(_)) => Some(lhs_type.clone()),
- (List(_), FixedSizeList(_, _)) => Some(lhs_type.clone()),
- (FixedSizeList(_, _), List(_)) => Some(rhs_type.clone()),
+ (
+ LargeList(lhs_field),
+ List(rhs_field) | LargeList(rhs_field) | FixedSizeList(rhs_field,
_),
+ )
+ | (FixedSizeList(lhs_field, _) | List(lhs_field),
LargeList(rhs_field)) => {
Review Comment:
maintain consistent ordering to make the code easier to follow
```suggestion
| (List(lhs_field) | FixedSizeList(lhs_field, _) ,
LargeList(rhs_field)) => {
```
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -1138,27 +1138,44 @@ fn numeric_string_coercion(lhs_type: &DataType,
rhs_type: &DataType) -> Option<D
}
}
+fn coerce_list_children(lhs_field: &FieldRef, rhs_field: &FieldRef) ->
Option<FieldRef> {
+ Some(Arc::new(
+
Arc::unwrap_or_clone(Arc::clone(lhs_field)).with_data_type(comparison_coercion(
+ lhs_field.data_type(),
+ rhs_field.data_type(),
+ )?),
+ ))
+}
+
/// Coercion rules for list types.
fn list_coercion(lhs_type: &DataType, rhs_type: &DataType) -> Option<DataType>
{
use arrow::datatypes::DataType::*;
match (lhs_type, rhs_type) {
- (List(_), List(_)) => Some(lhs_type.clone()),
- (LargeList(_), List(_)) => Some(lhs_type.clone()),
Review Comment:
You can add some test cases of what used to fail and now it doesn't.
For example query unioning list(uint8) with list(int32) where right values
don't fit int8 (too big or negative).
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -1138,27 +1138,44 @@ fn numeric_string_coercion(lhs_type: &DataType,
rhs_type: &DataType) -> Option<D
}
}
+fn coerce_list_children(lhs_field: &FieldRef, rhs_field: &FieldRef) ->
Option<FieldRef> {
+ Some(Arc::new(
+
Arc::unwrap_or_clone(Arc::clone(lhs_field)).with_data_type(comparison_coercion(
Review Comment:
`Arc::unwrap_or_clone` feels unnecessarly indirect, given the fact we know
for sure that there are 2+ arcs to the field.
Use ordinary clone
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -1138,27 +1138,44 @@ fn numeric_string_coercion(lhs_type: &DataType,
rhs_type: &DataType) -> Option<D
}
}
+fn coerce_list_children(lhs_field: &FieldRef, rhs_field: &FieldRef) ->
Option<FieldRef> {
+ Some(Arc::new(
+
Arc::unwrap_or_clone(Arc::clone(lhs_field)).with_data_type(comparison_coercion(
Review Comment:
> comparison_coercion
should we use `type_union_resolution` here?
--
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]
