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

Allow creation of String arrays from &Option<&str> iterators #680

Merged
merged 2 commits into from
Aug 12, 2021

Conversation

koomen
Copy link
Contributor

@koomen koomen commented Aug 8, 2021

Which issue does this PR close?

Closes #598.

Rationale for this change

What changes are included in this PR?

Added an implementation of FromIterator<&Option<Ptr>> where Ptr: AsRef<str> for GenericStringArray<OffsetSize>. The new implementation converts iterated values into &str and wraps in an owned Option, then uses the existing FromIterator<Option<Ptr>> implementation to collect into a GenericStringArray.

Are there any user-facing changes?

Users can now create string arrays from &Option<&str> iterators. No breaking changes.

@github-actions github-actions bot added the arrow Changes to the arrow crate label Aug 8, 2021
@codecov-commenter
Copy link

codecov-commenter commented Aug 8, 2021

Codecov Report

Merging #680 (ccb3638) into master (857dbaf) will increase coverage by 0.00%.
The diff coverage is 83.33%.

❗ Current head ccb3638 differs from pull request most recent head 866ced1. Consider uploading reports for the commit 866ced1 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #680   +/-   ##
=======================================
  Coverage   82.44%   82.44%           
=======================================
  Files         168      168           
  Lines       47326    47347   +21     
=======================================
+ Hits        39016    39034   +18     
- Misses       8310     8313    +3     
Impacted Files Coverage Δ
parquet/src/column/writer.rs 92.81% <73.68%> (-0.22%) ⬇️
arrow/src/array/array_string.rs 97.88% <100.00%> (+0.06%) ⬆️
arrow/src/ffi.rs 86.54% <0.00%> (-0.27%) ⬇️
parquet/src/encodings/encoding.rs 94.67% <0.00%> (+0.19%) ⬆️

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 857dbaf...866ced1. Read the comment docs.

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you very much @koomen this looks awesome. In addition to looking at the code, I also tried out the example from #598 and it works like a charm ❤️

fn main() {
    let string_array: ArrayRef = Arc::new([Some("foo"), None, Some("bar")].into_iter().collect::<StringArray>());
    println!("My awesome string array: {:?}", string_array);
}

Output

My awesome string array: StringArray
[
  "foo",
  null,
  "bar",
]

FYI @nevi-me / @houqp

@koomen
Copy link
Contributor Author

koomen commented Aug 11, 2021

@alamb Thank you for taking a look! Noob question: Do I need to do anything else to merge this? I'm assuming that someone with permission will do so at some point, but don't want to leave this dangling if it is in fact gating on me :)

@alamb
Copy link
Contributor

alamb commented Aug 11, 2021

Nope no worry @koomen -- nothing is waiting on you. I am waiting to see if any of the other maintainers wants to comment and if not I plan to merge it in later today or tomorrow.

Also, FWIW I plan to backport this for inclusion in 5.2.0 (eta release early next week)

Copy link
Member

@jorgecarleitao jorgecarleitao left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for your contribution!

arrow/src/array/array_string.rs Outdated Show resolved Hide resolved
arrow/src/array/array_string.rs Outdated Show resolved Hide resolved
Co-authored-by: Jorge Leitao <jorgecarleitao@gmail.com>
@alamb
Copy link
Contributor

alamb commented Aug 12, 2021

Given I plan to make a 5.2.0 release candidate today I took the liberty of applying @jorgecarleitao 's suggestions to add links to the doc comments to this PR and will merge when it passes CI. cc @koomen

Thanks again

@alamb alamb merged commit 1a5c26b into apache:master Aug 12, 2021
alamb added a commit that referenced this pull request Aug 12, 2021
* Allow creation of String arrays from &Option<&str> iterators

* Add links in doc comments

Co-authored-by: Jorge Leitao <jorgecarleitao@gmail.com>

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Co-authored-by: Jorge Leitao <jorgecarleitao@gmail.com>
alamb added a commit that referenced this pull request Aug 12, 2021
…686)

* Allow creation of String arrays from &Option<&str> iterators

* Add links in doc comments

Co-authored-by: Jorge Leitao <jorgecarleitao@gmail.com>

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Co-authored-by: Jorge Leitao <jorgecarleitao@gmail.com>

Co-authored-by: Pete Koomen <koomen@gmail.com>
Co-authored-by: Jorge Leitao <jorgecarleitao@gmail.com>
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.

Allow the creation of String arrays from an interator of &Option<&str>
4 participants