-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fallback to Utf8View for Dict(_, Utf8View)
in type_union_resolution_coercion
#14602
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this!
new_value_type.map(|t| DataType::Dictionary(index_type.clone(), Box::new(t))) | ||
match type_union_resolution_coercion(value_type, other_type) { | ||
// Dict(k, Utf8View) is redundant, Utf8View is good enough | ||
Some(DataType::Utf8View) => Some(DataType::Utf8View), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly also true for binary views (even if I didn't report in my original issue)?
Dict(_, Utf8View)
in type_union_resolution_coercion
Dict(_, ViewType)
in type_union_resolution_coercion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we get some tests for this as well?
Dict(_, ViewType)
in type_union_resolution_coercion
Dict(_, Utf8View)
in type_union_resolution_coercion
It seems binary view is problematic, so I remove it for now. Add it if there is test covered |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @jayzhan211 and @davidhewitt
Which issue does this PR close?
coalesce
leads to type unsupported arrow cast #14581.Rationale for this change
utf8view is good enough, there is no reason we need
dict(_, utf8view)
.What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?