-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Fix casts of ScalarValue::Utf8 to DictionaryArray
#2875
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
Conversation
ScalarValue::Utf8 toDictionaryArray
| &array, | ||
| &self.cast_type, | ||
| )?)), | ||
| ColumnarValue::Scalar(scalar) |
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.
This is the actual code change -- the rest of the PR is tests
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.
Won't this prevent this from using the scalar comparison kernels, effectively reversing #2808
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.
🤔 that is a good point -- perhaps I'll have a go at a proper fix for #2874
| // $VALUE_FN is an expression (like `result.value`) that extracts the value at index `i` | ||
| macro_rules! generic_test_cast { | ||
| ($A_ARRAY:ident, $A_TYPE:expr, $A_VEC:expr, $TYPEARRAY:ident, $TYPE:expr, $VEC:expr) => {{ | ||
| ($A_ARRAY:ident, $A_TYPE:expr, $A_VEC:expr, $TYPEARRAY:ident, $TYPE:expr, $VEC:expr, $VALUE_FN:expr) => {{ |
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.
needed to add a way to extract the value of the result at element i because it is different for a DictionaryArray
ScalarValue::Utf8 toDictionaryArrayScalarValue::Utf8 to DictionaryArray
3b4c110 to
24df39e
Compare
tustvold
left a comment
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.
I'm not a massive fan of this, as I think it will basically prevent evaluating scalar predicates directly against dictionaries, but I'm approving as it is blocking upgrades.
I personally would prefer we carry the data type on ColumnarValue::Scalar or something...
|
Will convert to draft as I attempt to "do it properly" as in #2874 |
|
Update: Doing it properly in #2891 is showing good promise |
|
Closing in favor of #2891 |
Which issue does this PR close?
Closes #2873
Rationale for this change
Fixes a bug introduced in #2819
See #2873 (a query that used to work does not anymore). The underlying issue is described in #2874
What changes are included in this PR?
Hack around #2874
Are there any user-facing changes?
Sone queries on dictionaries work again
cc @viirya