ARROW-6080: [Java] Support search operation for BaseRepeatedValueVector #4974
Conversation
| @@ -230,6 +235,50 @@ public int compareNotNull(int index1, int index2) { | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * Default comparator for {@link BaseRepeatedValueVector}. | |||
There was a problem hiding this comment.
can you document how it compares elements?
There was a problem hiding this comment.
Good suggestion. Thanks.
liyafan82
force-pushed
the
fly_0731_listcmp
branch
from
af41ad3
to
5243662
Compare
Codecov Report
@@ Coverage Diff @@
## master #4974 +/- ##
==========================================
- Coverage 87.64% 87.28% -0.36%
==========================================
Files 1014 714 -300
Lines 145922 107403 -38519
Branches 1437 0 -1437
==========================================
- Hits 127887 93751 -34136
+ Misses 17673 13652 -4021
+ Partials 362 0 -362
Continue to review full report at Codecov.
|
| @@ -190,4 +196,105 @@ public void testLinearSearchVarChar() { | |||
| assertEquals(-1, VectorSearcher.linearSearch(rawVector, comparator, negVector, 0)); | |||
| } | |||
| } | |||
|
|
|||
| private ListVector createListVector() { | |||
There was a problem hiding this comment.
sorry, taking a look at these unit tests, I'm not sure they provide good coverage. I think it would be better to have tests against the comparator directly. For:
- First N elements equal, with one vector longer then the other.
- N elements equal, and the N + 1 unequal
- Equal vectors.
There was a problem hiding this comment.
No problem. We appreciate your careful review.
Tests added as you suggested.
liyafan82
force-pushed
the
fly_0731_listcmp
branch
from
5243662
to
c0fa2e0
Compare
liyafan82
force-pushed
the
fly_0731_listcmp
branch
from
c0fa2e0
to
63c7519
Compare
|
@liyafan82 thanks for additional tests. Looks good just trying to rerun the failing CI job before merging. |
@emkornfield thank you for your effort. |
|
+1, thank you. |
Support comparison & search for sub-classes of BaseRepeatedValueVector.