-
Notifications
You must be signed in to change notification settings - Fork 855
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
Add DictionaryArray::key
function
#1912
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1912 +/- ##
==========================================
- Coverage 83.41% 83.41% -0.01%
==========================================
Files 214 214
Lines 56991 57004 +13
==========================================
+ Hits 47541 47550 +9
- Misses 9450 9454 +4
Continue to review full report at Codecov.
|
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 a nice interface 👍 I left a comment about that expect
.
self.keys | ||
.value(i) | ||
.to_usize() | ||
.expect("Dictionary index not usize") |
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.
For most cases this unwrap won't panic, but do we need to maintain the same behavior with https://github.com/apache/arrow-datafusion/blob/080c32400ddfa2d45b5bebb820184eac8fd5a03a/datafusion/common/src/scalar.rs#L342-L358 ? If so the return type can either be Result<Option<_>>
or Option<_>
, I'm ok with both (hard to choose...).
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.
Panicking should be fine, it's a validation failure if the array contains negative indexes. TBH I keep meaning to change all these checked conversions to numeric casts (i.e. as), I wouldn't be surprised if this leads to non-trivial performance benefits.
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.
Filed #1918 to track
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.
Looks good to me. Currently to I need to call value
on keys
array. This should be convenient.
Which issue does this PR close?
Closes #1911
Rationale for this change
See #1911
What changes are included in this PR?
DictionaryArray::key
functionAre there any user-facing changes?
Yes new function