Skip to content

ARROW-6871: [Java] Enhance TransferPair related parameters check and tests - #5645

Closed
tianchen92 wants to merge 4 commits into
apache:masterfrom
tianchen92:ARROW-6871
Closed

ARROW-6871: [Java] Enhance TransferPair related parameters check and tests#5645
tianchen92 wants to merge 4 commits into
apache:masterfrom
tianchen92:ARROW-6871

Conversation

@tianchen92

@tianchen92tianchen92 commented Oct 14, 2019

Copy link
Copy Markdown
Contributor

Related to ARROW-6871.

TransferPair related param checks in different classes have potential problems:

i. splitAndTansfer has no indices check in classes like VarcharVector
ii. splitAndTranser indices check in classes like UnionVector is not correct (Preconditions.checkArgument(startIndex + length <= valueCount)), should check params separately.
iii. should add more UT to cover corner cases.

@tianchen92

Copy link
Copy Markdown
ContributorAuthor

cc @siddharthteotia

@github-actions

Copy link
Copy Markdown

@tianchen92

Copy link
Copy Markdown
ContributorAuthor

also cc @praveenbingo appreciate if you have time to take a look.

Comment threadjava/vector/src/main/codegen/templates/UnionVector.java Outdated
@emkornfield

Copy link
Copy Markdown
Contributor

@tianchen92 did you add the benchmark you mentioned in the JIRA?

@tianchen92

tianchen92 commented Jan 11, 2020

Copy link
Copy Markdown
ContributorAuthor

@tianchen92 did you add the benchmark you mentioned in the JIRA?

I added a benchmark for copyValueSafe and shows no regression, however, due to #5645 (comment), I finally revert the check in copyValueSafe API.
Need to add benchmark for other API related to this PR?

@tianchen92

Copy link
Copy Markdown
ContributorAuthor

I add a benchmark and shows no regression.

Before:
Benchmark Mode Cnt Score Error Units
TransferPairBenchmarks.splitAndTransferIntVector avgt 5 1.801 ± 0.119 us/op
TransferPairBenchmarks.splitAndTransferVarcharVector avgt 5 12.794 ± 0.812 us/op

After:
Benchmark Mode Cnt Score Error Units
TransferPairBenchmarks.splitAndTransferIntVector avgt 5 1.800 ± 0.102 us/op
TransferPairBenchmarks.splitAndTransferVarcharVector avgt 5 12.570 ± 1.339 us/op

@emkornfield

Copy link
Copy Markdown
Contributor

+1 thanks.

kszucs pushed a commit that referenced this pull request Feb 7, 2020
…tests
Related to [ARROW-6871](https://issues.apache.org/jira/browse/ARROW-6871).
TransferPair related param checks in different classes have potential problems:
i. splitAndTansfer has no indices check in classes like VarcharVector
ii. splitAndTranser indices check in classes like UnionVector is not correct (Preconditions.checkArgument(startIndex + length <= valueCount)), should check params separately.
iii. should add more UT to cover corner cases.
Closes#5645 from tianchen92/ARROW-6871 and squashes the following commits:
f3b897d <tianchen> fix style
0d3c7ea <tianchen> add benchmark
01f9a48 <tianchen> revert changes in copyFrom
a22d58a <tianchen> ARROW-6871: Enhance TransferPair related parameters check and tests
Authored-by: tianchen <niki.lj@alibaba-inc.com>
Signed-off-by: Micah Kornfield <emkornfield@gmail.com>
pribor pushed a commit to GlobalWebIndex/arrow that referenced this pull request Oct 24, 2025
…tests
Related to [ARROW-6871](https://issues.apache.org/jira/browse/ARROW-6871).
TransferPair related param checks in different classes have potential problems:
i. splitAndTansfer has no indices check in classes like VarcharVector
ii. splitAndTranser indices check in classes like UnionVector is not correct (Preconditions.checkArgument(startIndex + length <= valueCount)), should check params separately.
iii. should add more UT to cover corner cases.
Closesapache#5645 from tianchen92/ARROW-6871 and squashes the following commits:
f3b897d <tianchen> fix style
0d3c7ea <tianchen> add benchmark
01f9a48 <tianchen> revert changes in copyFrom
a22d58a <tianchen> ARROW-6871: Enhance TransferPair related parameters check and tests
Authored-by: tianchen <niki.lj@alibaba-inc.com>
Signed-off-by: Micah Kornfield <emkornfield@gmail.com>
Sign up for freeto 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.

3 participants

@tianchen92@emkornfield@liyafan82