-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Support substrait serialization for ScalarValue::Utf8View
and ScalarValue::BinaryView
#12118
Comments
ScalarValue::Utf8View
and ScalarValue::BinaryView
@XiangpengHao has a draft pr here #11898 |
take |
There is a long discussion over here about the type system in substrait. The summary outcome (also reflected in the spec here) is that we have logical type "string" and different variations for the physical type (e.g. uft8 vs largeutf8 vs utf8view). I've passing tests using this approach; want to check code coverage on a few things before putting up the PR (a.k.a. it looks like the largeutf8 variation was not fully implementation everywhere -- so I want to make sure we have full test coverage). |
Yup, this aligns with my understanding - the PR looks good from cursory review! |
Is your feature request related to a problem or challenge?
Part of #11752
We are trying to enable DataFusion to use StringViewArray by default. If we do that it means
ScalarValue::Utf8View
andScalarValue::BinaryView
will be more likely to be used in plansDescribe the solution you'd like
Thus we need to ensure
ScalarValue::Utf8View
andScalarValue::BinaryView
can be serialized using datafusion substraitDescribe alternatives you've considered
I recommend adding coverage for
ScalarValue::Utf8View
and `ScalarValue::BinaryView to the tests heredatafusion/datafusion/substrait/src/logical_plan/producer.rs
Lines 2283 to 2284 in b4069a6
And then update the code to get the tests to pass
Additional context
No response
The text was updated successfully, but these errors were encountered: