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

FixedSizeBinaryArray::try_from_sparse_iter failed when given all Nones #1508

Closed
wants to merge 2 commits into from

Conversation

jamescorbett
Copy link

Which issue does this PR close?

Closes ##1390.

Rationale for this change

Minimally invasive change to work around Options being unsized if None.

What changes are included in this PR?

Adding new function to get access to default size while maintaining original functionality.

Are there any user-facing changes?

One additional function try_from_sparse_iter_sized

@github-actions github-actions bot added the arrow Changes to the arrow crate label Mar 30, 2022
@yjshen yjshen changed the title Closes #1390 FixedSizeBinaryArray::try_from_sparse_iter failed when given all Nones Mar 30, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #1508 (3a06bb7) into master (4c22315) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1508      +/-   ##
==========================================
- Coverage   82.73%   82.73%   -0.01%     
==========================================
  Files         188      188              
  Lines       54354    54356       +2     
==========================================
- Hits        44970    44969       -1     
- Misses       9384     9387       +3     
Impacted Files Coverage Δ
arrow/src/array/array_binary.rs 92.64% <100.00%> (+0.02%) ⬆️
arrow/src/datatypes/field.rs 53.79% <0.00%> (-0.31%) ⬇️
parquet_derive/src/parquet_field.rs 65.98% <0.00%> (-0.23%) ⬇️
arrow/src/array/transform/mod.rs 86.35% <0.00%> (-0.12%) ⬇️

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 4c22315...3a06bb7. Read the comment docs.

Copy link
Member

@viirya viirya left a comment

Choose a reason for hiding this comment

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

Thanks for the fix. Can you also add a test for the fix?

@alamb
Copy link
Contributor

alamb commented Apr 1, 2022

I agree we should have a test prior to merging. Thanks @jamescorbett

@alamb
Copy link
Contributor

alamb commented Apr 4, 2022

We can probably just copy the test from #1390

@jamescorbett
Copy link
Author

I will do sorry I have had work commitments.

@alamb
Copy link
Contributor

alamb commented Apr 4, 2022

I will do sorry I have had work commitments.

No worries -- sorry I didn't mean to pressure you. Thank you for the contribution; I can add the tests too if you don't have time

@alamb
Copy link
Contributor

alamb commented Apr 25, 2022

Marking as a draft to signify there is still some work to do. Please feel free to mark it ready for review when it is

@alamb alamb marked this pull request as draft April 25, 2022 10:40
@tustvold
Copy link
Contributor

Closing this as #3054 has added support for this

@tustvold tustvold closed this Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants