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

ARROW-11082: [Rust] C data interface to largeUTF8 #9054

Closed
wants to merge 2 commits into from
Closed

ARROW-11082: [Rust] C data interface to largeUTF8 #9054

wants to merge 2 commits into from

Conversation

jorgecarleitao
Copy link
Member

This also simplifies some code and adds a test for the boolean case, which is special due to bit-packing.

@github-actions
Copy link

@alamb
Copy link
Contributor

alamb commented Dec 31, 2020

The full set of Rust CI tests did not run on this PR :(

Can you please rebase this PR against apache/master to pick up the changes in #9056 so that they do?

I apologize for the inconvenience.

Copy link
Contributor

@nevi-me nevi-me left a comment

Choose a reason for hiding this comment

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

One small suggestion

@@ -624,10 +650,10 @@ mod tests {
}
// case with nulls is tested in the docs, through the example on this module.

#[test]
fn test_string() -> Result<()> {
fn test_genetic_string<Offset: StringOffsetSizeTrait>() -> Result<()> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fn test_genetic_string<Offset: StringOffsetSizeTrait>() -> Result<()> {
fn test_generic_string<Offset: StringOffsetSizeTrait>() -> Result<()> {

}

#[test]
fn test_bool() -> Result<()> {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice and neat :)

@codecov-io
Copy link

Codecov Report

Merging #9054 (3695980) into master (51672b2) will increase coverage by 0.01%.
The diff coverage is 85.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9054      +/-   ##
==========================================
+ Coverage   82.61%   82.62%   +0.01%     
==========================================
  Files         202      202              
  Lines       50048    50068      +20     
==========================================
+ Hits        41347    41370      +23     
+ Misses       8701     8698       -3     
Impacted Files Coverage Δ
rust/arrow/src/ffi.rs 72.45% <83.33%> (+2.17%) ⬆️
rust/arrow/src/array/array_string.rs 90.05% <100.00%> (-0.22%) ⬇️
rust/arrow/src/array/transform/mod.rs 88.93% <0.00%> (+0.73%) ⬆️

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 51672b2...3695980. Read the comment docs.

@nevi-me nevi-me closed this in dfef236 Jan 3, 2021
GeorgeAp pushed a commit to sirensolutions/arrow that referenced this pull request Jun 7, 2021
This also simplifies some code and adds a test for the boolean case, which is special due to bit-packing.

Closes apache#9054 from jorgecarleitao/ffi_large_string

Authored-by: Jorge C. Leitao <jorgecarleitao@gmail.com>
Signed-off-by: Neville Dipale <nevilledips@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants