coercion vec[Dictionary, Utf8] to Dictionary for coalesce function#9958
coercion vec[Dictionary, Utf8] to Dictionary for coalesce function#9958alamb merged 4 commits intoapache:mainfrom
Conversation
github-actions
bot
added
logical-expr
Lordworms
changed the title
github-actions
bot
added
sqllogictest
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you for working on this @Lordworms 🙏
I think we need a few more tests and to handle a few more cases, but it looks good so far
| Some(type_into.clone()) | ||
| } | ||
|
|
||
| Dictionary(_, _) if matches!(type_from, Utf8) => Some(type_into.clone()), |
There was a problem hiding this comment.
I think this currently says it is possible to coerce from Utf8 to any dictionary type (e.g. even Dictionary(Int64, Int32))
I think the dictionary_coercion function does something similar here: https://github.com/apache/arrow-datafusion/blob/37b73751b19b82516443579c714b3b5986dac927/datafusion/expr/src/type_coercion/binary.rs#L635-L634
So maybe we need to verify we can coerce the value types, like this?
| Dictionary(_, _) if matches!(type_from, Utf8) => Some(type_into.clone()), | |
| Dictionary(_, value_type) if coerce_from(value_type, from_type) => Some(type_into.clone()), |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @Lordworms -- this looks very nice to me 🙏
…pache#9958) * for debug finish remove print add space * fix clippy * finish * fix clippy
…pache#9958) * for debug finish remove print add space * fix clippy * finish * fix clippy
…9958) (#10104) * for debug finish remove print add space * fix clippy * finish * fix clippy Co-authored-by: Lordworms <[email protected]>
2 participants
Which issue does this PR close?
Closes #9925
Rationale for this change
The signature of the coalesce function has changed to variadic_equal and we need to add a new rule to coercion [Dictionary, Utf8] to Dictionary
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?