Skip to content

ARROW-10002: [Rust] Remove default fn from PrimitiveArrayOps#8206

Closed
BatmanAoD wants to merge 8 commits intoapache:masterfrom
BatmanAoD:IndexViaPrimitive
Closed

ARROW-10002: [Rust] Remove default fn from PrimitiveArrayOps#8206
BatmanAoD wants to merge 8 commits intoapache:masterfrom
BatmanAoD:IndexViaPrimitive

Conversation

@BatmanAoD
Copy link
Copy Markdown
Contributor

@BatmanAoD BatmanAoD commented Sep 16, 2020

Problem description

This is based on my proof-of-concept solution.

@github-actions
Copy link
Copy Markdown

Comment thread rust/arrow/src/datatypes.rs Outdated
@BatmanAoD BatmanAoD marked this pull request as ready for review September 16, 2020 22:30
Copy link
Copy Markdown
Member

@andygrove andygrove left a comment

Choose a reason for hiding this comment

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

Thanks @BatmanAoD this LGTM but I would like to see reviews from other other Rust contributors before merging this, in case there are subtle details that I am missing. Perhaps @paddyhoran or @nevi-me could take a look too?

Copy link
Copy Markdown
Contributor

@paddyhoran paddyhoran left a comment

Choose a reason for hiding this comment

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

Nice. LGTM. Thanks @BatmanAoD

@andygrove
Copy link
Copy Markdown
Member

@BatmanAoD Could you rebase against master (not merge) and I can merge this.

@BatmanAoD BatmanAoD force-pushed the IndexViaPrimitive branch 2 times, most recently from 93ca62a to c906467 Compare September 17, 2020 21:36
@andygrove
Copy link
Copy Markdown
Member

Test failures are unrelated (conda integration tests)

@andygrove andygrove closed this in 1f30466 Sep 18, 2020
@jorgecarleitao
Copy link
Copy Markdown
Member

🎉 this is highly appreciated, and the API is also easier to work with.

@sunchao
Copy link
Copy Markdown
Member

sunchao commented Sep 30, 2020

Just found out this... Nice work @BatmanAoD !

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.

5 participants