-
Notifications
You must be signed in to change notification settings - Fork 786
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 typed dictionary (#2136) #2297
Conversation
type IntoIter = ArrayIter<Self>; | ||
|
||
fn into_iter(self) -> Self::IntoIter { | ||
ArrayIter::new(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.
I think this is a nice example of how the composability of traits can be better than macros
arrow/src/array/equal_json.rs
Outdated
self.keys().equals_json(json) | ||
} | ||
} | ||
|
||
impl<'a, K: ArrowPrimitiveType, V> JsonEqual for TypedDictionaryArray<'a, K, V> { |
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 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 think this is a great convenience. I suggest adding a pointer / mention in the DictionaryArray
docstring
what do you think @jhorstmann and @viirya ?
I recommend we hold off on merging this until other people have had a chance to look at it cc @HaoYang670 |
#2322 now builds on top of this, showing a concrete use-case |
I intend to merge this today, so that it can make the arrow 20 release. We can always revisit this in the future |
arrow/src/array/array_dictionary.rs
Outdated
#[derive(Copy, Clone)] | ||
pub struct TypedDictionaryArray<'a, K: ArrowPrimitiveType, V> { | ||
/// The dictionary array | ||
pub dictionary: &'a DictionaryArray<K>, |
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.
Sorry for the late.
I prefer to name it base
, but never mind, this is really a small tip.
LGTM!
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 opted to make these private for now, so we can always rename them later
Benchmark runs are scheduled for baseline = b6eaf22 and contender = 4a3919b. 4a3919b is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
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.
lgtm
Which issue does this PR close?
Closes #2136
Rationale for this change
What changes are included in this PR?
Adds a
TypedDictionaryArray
and provides a method to downcast aDictionaryArray
to itAre there any user-facing changes?
No