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-6464: [Java] Refactor FixedSizeListVector#splitAndTransfer with slice API #5293

Merged
merged 3 commits into from Oct 12, 2019

Conversation

tianchen92
Copy link
Contributor

Related to ARROW-6464.

Currently FixedSizeListVector#splitAndTransfer actually use copyValueSafe which has memory copy, we should use slice API instead.
Meanwhile, splitAndTransfer in all classes should position index check at beginning.

@codecov-io
Copy link

codecov-io commented Sep 5, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@d2be6a5). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #5293   +/-   ##
=========================================
  Coverage          ?   89.81%           
=========================================
  Files             ?      697           
  Lines             ?   105908           
  Branches          ?        0           
=========================================
  Hits              ?    95116           
  Misses            ?    10792           
  Partials          ?        0

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 d2be6a5...e612059. Read the comment docs.

@emkornfield
Copy link
Contributor

@jacques-n where you going to take another look at this?

@wesm
Copy link
Member

wesm commented Oct 3, 2019

@jacques-n ping?

@siddharthteotia
Copy link
Contributor

siddharthteotia commented Oct 12, 2019

LGTM. However, I would suggest to add as many tests as possible to cover exhaustive set of corner cases with validity transfer. In the past there were so many boundary cases that were found as bugs and were incredibly hard to debug/reproduce. It would be great if you can do that in this or follow-up PR.

@tianchen92
Copy link
Contributor Author

LFGTM. However, I would suggest to add as many tests as possible to cover exhaustive set of corner cases with validity transfer. In the past there were so many boundary cases that were found as bugs and were incredibly hard to debug/reproduce. It would be great if you can do that in this or follow-up PR.

Sure, I could do this in a follow-up PR, thanks!

@siddharthteotia
Copy link
Contributor

LFGTM. However, I would suggest to add as many tests as possible to cover exhaustive set of corner cases with validity transfer. In the past there were so many boundary cases that were found as bugs and were incredibly hard to debug/reproduce. It would be great if you can do that in this or follow-up PR.

Sure, I could do this in a follow-up PR, thanks!

Sorry LFGTM was a typo.

@siddharthteotia siddharthteotia merged commit 8621a5c into apache:master Oct 12, 2019
kszucs pushed a commit to kszucs/arrow that referenced this pull request Oct 21, 2019
… slice API (apache#5293)

* ARROW-6464: [Java] Refactor FixedSizeListVector#splitAndTransfer with slice API

* replace assert with Preconditions

* simplify length compute
kszucs pushed a commit to kszucs/arrow that referenced this pull request Oct 21, 2019
… slice API (apache#5293)

* ARROW-6464: [Java] Refactor FixedSizeListVector#splitAndTransfer with slice API

* replace assert with Preconditions

* simplify length compute
kszucs pushed a commit to kszucs/arrow that referenced this pull request Oct 21, 2019
… slice API (apache#5293)

* ARROW-6464: [Java] Refactor FixedSizeListVector#splitAndTransfer with slice API

* replace assert with Preconditions

* simplify length compute
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

7 participants