Skip to content
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.

Sign up for GitHub

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

Jump to bottom

DictionaryBuilders can Create Invalid DictionaryArrays #2684

Closed
tustvold opened this issue Sep 8, 2022 · 3 comments · Fixed by #2685
Closed

DictionaryBuilders can Create Invalid DictionaryArrays #2684

tustvold opened this issue Sep 8, 2022 · 3 comments · Fixed by #2685
Assignees
@tustvold
Labels
arrow Changes to the arrow crate bug parquet Changes to the parquet crate

Comments

@tustvold
Copy link
Contributor

tustvold commented Sep 8, 2022

Describe the bug

Both StringDictionaryBuilder and PrimitiveDictionaryBuilder are created with existing builders. This is problematic if the key builders are not empty, as these already inserted keys will not be validated

To Reproduce

Expected behavior

Requiring the builders to create a dictionary builder is a pretty strange API, and I would like to propose just removing it

Additional context
@tustvold tustvold added the bug label Sep 8, 2022
@tustvold tustvold self-assigned this Sep 8, 2022
tustvold added a commit to tustvold/arrow-rs that referenced this issue Sep 8, 2022
@tustvold
Simplify DictionaryBuilder constructors (apache#2684) (apache#2054)
7fcb8a5
tustvold added a commit to tustvold/arrow-rs that referenced this issue Sep 8, 2022
@tustvold
Simplify DictionaryBuilder constructors (apache#2684) (apache#2054)
47c0d19
@tustvold tustvold mentioned this issue Sep 8, 2022
Merged
tustvold added a commit that referenced this issue Sep 8, 2022
@tustvold @viirya
Simplify DictionaryBuilder constructors (#2684) (#2054) (#2685)
5e2b4c7 
* Simplify DictionaryBuilder constructors (#2684) (#2054)

* Apply suggestions from code review

Co-authored-by: Liang-Chi Hsieh <viirya@gmail.com>

Co-authored-by: Liang-Chi Hsieh <viirya@gmail.com>
@alamb
Copy link
Contributor

alamb commented Sep 16, 2022

Automatically added labels {'parquet'} from #2685

@alamb alamb added the parquet Changes to the parquet crate label Sep 16, 2022
@alamb
Copy link
Contributor

alamb commented Sep 16, 2022

Automatically added labels {'arrow'} from #2685

@alamb alamb added arrow Changes to the arrow crate api-change Changes to the arrow API labels Sep 16, 2022
@alamb
Copy link
Contributor

alamb commented Sep 16, 2022

Automatically added labels {'api-change'} from #2685

@alamb alamb removed the api-change Changes to the arrow API label Sep 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tustvold tustvold

Labels
arrow Changes to the arrow crate bug parquet Changes to the parquet crate
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Simplify DictionaryBuilder constructors (#2684) (#2054) tustvold/arrow-rs
2 participants
@alamb @tustvold