ARROW-10791: [Rust] StreamReader, read_dictionary duplicating schema info #8820
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I had submitted this before but I think it got lost in a rebase somewhere. I think this is more correct and informative.
This simplifies the code in `read_dictionary` and `StreamReader`; the `ipc_schema` gets parsed and converted into a `Schema` that `StreamReader` also holds onto, so use the `Schema` to find which fields use dictionaries rather than keeping both around.
nevi-me
approved these changes
Dec 3, 2020
| @@ -489,7 +489,12 @@ fn array_from_json( | |||
| Ok(Arc::new(array)) | |||
| } | |||
| DataType::Dictionary(key_type, value_type) => { | |||
| let dict_id = field.dict_id(); | |||
| let dict_id = field.dict_id().ok_or_else(|| { | |||
There was a problem hiding this comment.
I apologise, I do remember this change, but I'm not sure of what happened; I presume while I was rebasing.
There was a problem hiding this comment.
No worries! I remember I sent you a rebased PR to your branch that was... complicated... so I'm not surprised :) Thanks for the merge!
GeorgeAp
pushed a commit
to sirensolutions/arrow
that referenced
this pull request
Jun 7, 2021
…info The purpose of this PR is refactoring `read_dictionary` to only need one kind of `Schema`, which lets us then remove the `find_dictionary_field` function and the `ipc_schema` field on `StreamReader` by adding a way to look up schema fields that use a particular dictionary by ID. I'm also resubmitting a change to the `dict_id`/`dict_is_ordered` methods on `Field`; I had submitted this to @nevi-me to become part of apache#8200 but it looks like it got lost in a rebase or something? I think it's more correct to only return values if the fields have a dictionary as their datatype. Closes apache#8820 from carols10cents/simplify-dictionary-field-lookup Authored-by: Carol (Nichols || Goulding) <[email protected]> Signed-off-by: Neville Dipale <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The purpose of this PR is refactoring
read_dictionaryto only need one kind ofSchema, which lets us then remove thefind_dictionary_fieldfunction and theipc_schemafield onStreamReaderby adding a way to look up schema fields that use a particular dictionary by ID.I'm also resubmitting a change to the
dict_id/dict_is_orderedmethods onField; I had submitted this to @nevi-me to become part of #8200 but it looks like it got lost in a rebase or something? I think it's more correct to only return values if the fields have a dictionary as their datatype.