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.

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

Remove PrimitiveBuilder::finish_dict (#1978) #1980

Merged

Conversation

tustvold
Copy link
Contributor

Which issue does this PR close?

Closes #1978
Part of #1979

Rationale for this change

The finish_dict method has always struck me as a little bit strange, and it is definitely not safe. This PR just removes it

What changes are included in this PR?

Starts the process of #1979 and uses this to remove the need for PrimitiveBuilder::finish_dict

Are there any user-facing changes?

Yes, this removes an API

@tustvold tustvold added the api-change Changes to the arrow API label Jun 30, 2022
@github-actions github-actions bot added the arrow Changes to the arrow crate label Jun 30, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #1980 (954916d) into master (c47d14c) will increase coverage by 0.00%.
The diff coverage is 94.28%.

❗ Current head 954916d differs from pull request most recent head 85d9229. Consider uploading reports for the commit 85d9229 to get more accurate results

@@           Coverage Diff           @@
##           master    #1980   +/-   ##
=======================================
  Coverage   83.54%   83.54%           
=======================================
  Files         221      221           
  Lines       57395    57408   +13     
=======================================
+ Hits        47950    47961   +11     
- Misses       9445     9447    +2     
Impacted Files Coverage Δ
arrow/src/array/builder/primitive_builder.rs 93.11% <ø> (+0.32%) ⬆️
.../src/array/builder/primitive_dictionary_builder.rs 78.68% <87.50%> (+0.50%) ⬆️
...row/src/array/builder/string_dictionary_builder.rs 90.64% <87.50%> (-1.09%) ⬇️
arrow/src/array/array_primitive.rs 95.01% <100.00%> (+0.03%) ⬆️
arrow/src/array/array_string.rs 97.76% <100.00%> (+0.03%) ⬆️
arrow/src/array/data.rs 84.54% <100.00%> (+0.13%) ⬆️
arrow/src/datatypes/datatype.rs 65.42% <0.00%> (-0.38%) ⬇️
parquet_derive/src/parquet_field.rs 65.98% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c47d14c...85d9229. Read the comment docs.

Comment on lines -200 to -201
/// Builds the `DictionaryArray` and reset this builder.
pub fn finish_dict(&mut self, values: ArrayRef) -> DictionaryArray<T> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like an API should not public.

@tustvold tustvold merged commit 4350b8c into apache:master Jul 1, 2022
@alamb alamb changed the title Remove PrimitiveBuilder::finish_dict (#1978) Remove PrimitiveBuilder::finish_dict (#1978) Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change Changes to the arrow API arrow Changes to the arrow crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PrimitiveBuilder::finish_dict does not validate dictionary offsets
3 participants