Support struct coercion in type_union_resolution
#12839
Conversation
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
|
wasmtest Ci failure has been fixed in #12844 |
There was a problem hiding this comment.
Thank you @jayzhan211 -- this looks great
This PR seems like an improvement to me, but I am a bit worried we still have a testing gap.
Since it is not clear what the actual semantics are supposed to be I can't fully tell if this fixes the problem @kazuyukitanimura was seeing
| return None; | ||
| } | ||
|
|
||
| let types = std::iter::zip(lhs.iter(), rhs.iter()) |
There was a problem hiding this comment.
I am not familiar with the semantics, but as I read this code it would not coerce structs with fields named the same but in a different order.
For example
{
a: 1,
b: 2
}
Would not be coerceable / comparable to
{
b: 20 // note the fields are in different order
a: 10,
}
Is that intended? 🤔
There was a problem hiding this comment.
It seems like a bug 👍
There was a problem hiding this comment.
We need to fix Values first for this case, currently we doesn't have schema (column type) for Values, so we can't tell which order is correct if we got 2 different order struct. I think we should provider schema for building values or assume the first struct is the correct one.
|
|
||
| Ok(valid_types) | ||
| // Every signature failed, return the joined error | ||
| if res.is_empty() { |
| @@ -106,6 +108,32 @@ impl ScalarUDFImpl for MakeArray { | |||
|
|
|||
| fn coerce_types(&self, arg_types: &[DataType]) -> Result<Vec<DataType>> { | |||
| if let Some(new_type) = type_union_resolution(arg_types) { | |||
| // TODO: Move the logic to type_union_resolution if this applies to other functions as well | |||
There was a problem hiding this comment.
Ah, this is the same question I had above -- this code seems to properly account for differences in field order
| @@ -373,3 +373,30 @@ You reached the bottom! | |||
|
|
|||
| statement ok | |||
| drop view complex_view; | |||
|
|
|||
| # struct with different keys r1 and r2 is not valid | |||
There was a problem hiding this comment.
Could you also please add a test that shows what happens with coercion when the fields are in a different order?
Like this
> create table t(a struct<r1 varchar, c int>) as values ({r1: 'foo', c:1}), ({c:2, r: 'bar'});On main it seems to get an error:
Error during planning: Inconsistent data type across values list at row 1 column 0. Was Struct([Field { name: "r1", data_type: Utf8, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }, Field { name: "c", data_type: Int64, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }]) but found Struct([Field { name: "c", data_type: Int64, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }, Field { name: "r", data_type: Utf8, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }])
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
|
I will take a look on #5046 and struct in coalesce |
|
Thanks @alamb @kazuyukitanimura |
| (row('purple', 1), row('green', 2.3)); | ||
|
|
||
| # out of order struct literal | ||
| # TODO: This query should not fail |
Which issue does this PR close?
Closes #12843.
Previous I switch coercion rule of array and value from
comparison_coerciontotype_union_resolution. But some types are not covered intype_union_resolution.Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?