Skip to content

ARROW-11420: [Rust] Added support to length of Binary and List. - #9353

Closed
jorgecarleitao wants to merge 3 commits into
apache:masterfrom
jorgecarleitao:remove_unsafe
Closed

ARROW-11420: [Rust] Added support to length of Binary and List.#9353
jorgecarleitao wants to merge 3 commits into
apache:masterfrom
jorgecarleitao:remove_unsafe

Conversation

@jorgecarleitao

@jorgecarleitaojorgecarleitao commented Jan 28, 2021

Copy link
Copy Markdown
Member

This adds [Large]Binary and [Large]List support to length.

Also removes unneeded unsafe enabled by the recent PR #9215.

@github-actions

Copy link
Copy Markdown

@jorgecarleitaojorgecarleitao changed the title ARROW-11420: [Rust] Removed usage of unsafe.ARROW-11420: [Rust] Added support to length of Binary and List.Jan 28, 2021
@codecov-io

Copy link
Copy Markdown

Codecov Report

Merging #9353 (48d78ca) into master (2fd5857) will decrease coverage by 0.02%.
The diff coverage is 74.54%.

Impacted file tree graph

@@ Coverage Diff @@## master #9353 +/- ##
==========================================
- Coverage 81.97% 81.95% -0.03% 
==========================================
Files 216 216 Lines 53227 53270 +43 ==========================================
+ Hits 43633 43657 +24 - Misses 9594 9613 +19 
Impacted FilesCoverage Δ
rust/datafusion/src/physical_plan/functions.rs71.80% <55.55%> (-0.50%)⬇️
rust/arrow/src/compute/kernels/length.rs88.61% <78.26%> (-11.39%)⬇️
rust/parquet/src/encodings/encoding.rs94.86% <0.00%> (-0.20%)⬇️

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 2fd5857...48d78ca. Read the comment docs.

@alambalamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

looks really nice @jorgecarleitao 👍

@alamb

Copy link
Copy Markdown
Contributor

@jorgecarleitao -- this one needs a rebase and then I think it is ready to go

@alambalamb added the needs-rebase A PR that needs to be rebased by the author label Feb 12, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

.null_buffer().cloned() ?

@alambalamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I went through this again. I think it is good -- is there anything else you want to add @jorgecarleitao (tests? ) or shall we merge it?

@alamb

Copy link
Copy Markdown
Contributor

@jorgecarleitao I think this one is ready to go -- it just needs a rebase and then merge

@alamb

Copy link
Copy Markdown
Contributor

The Apache Arrow Rust community is moving the Rust implementation into its own dedicated github repositories arrow-rs and arrow-datafusion. It is likely we will not merge this PR into this repository

Please see the mailing-list thread for more details

We expect the process to take a few days and will follow up with a migration plan for the in-flight PRs.

@jorgecarleitao
jorgecarleitao deleted the remove_unsafe branch April 30, 2021 15:04
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component: Rust - DataFusionComponent: Rustneeds-rebaseA PR that needs to be rebased by the author

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@jorgecarleitao@codecov-io@alamb@nevi-me