-
Notifications
You must be signed in to change notification settings - Fork 115
feat[encodings/dict]: allow different nullability of codes and values in dict #3736
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
… in dict Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
|
|
||
| impl DictArray { | ||
| pub fn try_new(mut codes: ArrayRef, values: ArrayRef) -> VortexResult<Self> { | ||
| pub fn try_new(codes: ArrayRef, values: ArrayRef) -> VortexResult<Self> { |
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.
Can we add a unit test checking for diff nullability of codes and values?
robert3005
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.
There's a small optimisation possible at the reader level but at this level it's just a matter of casts. I think this would make more sense if we had dict array as a selection vector.
…code-values # Conflicts: # encodings/dict/src/array.rs # encodings/dict/src/serde.rs
Deploying vortex-bench with
|
| Latest commit: |
11a605b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://751405d7.vortex-93b.pages.dev |
| Branch Preview URL: | https://ji-dict-diff-null-code-value.vortex-93b.pages.dev |
Codecov Report❌ Patch coverage is
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
If we are going to do this then don't you also want the nulls to live in values by default? |
|
By default, do you mean the in writer? |
|
I mean the dictbuilder in btrblocks compressor and the dictwriter |
robert3005
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.
Let's do it, this was the original dict implementation before we simplified it. Before we see any benefit from it we have to make sure to change the writer and compressor
This is not a breaking change. This can allow expr pushdown on the values without having to check the codes validity. Mirrors: #3736 --------- Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
No description provided.