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

[Refactor] Remove CollectionUtils.sortAndDedup and use java TreeSet instead of hppc ObjectArrayList #6884

Merged
merged 2 commits into from
Mar 30, 2023

Conversation

nknize
Copy link
Collaborator

@nknize nknize commented Mar 29, 2023

HPPC ObjectArrayLists provide no benefit in CollectionUtils.sortAndDedup and adds an unnecessary library dependency. This commit removes that dependency on hppc by switching the sortAndDeup method to use java.util.Lists. BinaryFieldMapper logic is updated to use the generic java list.

relates #5910

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.indices.replication.SegmentReplicationIT.testCancellation
      1 org.opensearch.client.ReindexIT.testReindexTask
      1 org.opensearch.backwards.MixedClusterClientYamlTestSuiteIT.test {p0=indices.shrink/30_copy_settings/Copy settings during shrink index}

@codecov-commenter
Copy link

Codecov Report

Merging #6884 (d8daa42) into main (7b708c4) will increase coverage by 0.27%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff              @@
##               main    #6884      +/-   ##
============================================
+ Coverage     70.52%   70.79%   +0.27%     
- Complexity    59112    59255     +143     
============================================
  Files          4814     4811       -3     
  Lines        283743   283702      -41     
  Branches      40915    40912       -3     
============================================
+ Hits         200110   200850     +740     
+ Misses        67154    66297     -857     
- Partials      16479    16555      +76     
Impacted Files Coverage Δ
...va/org/opensearch/common/util/CollectionUtils.java 79.57% <100.00%> (+1.51%) ⬆️
...org/opensearch/index/mapper/BinaryFieldMapper.java 80.00% <100.00%> (-0.25%) ⬇️

... and 491 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@nknize nknize changed the title [Refactor] CollectionUtils.sortAndDedup to use java lists instead of hppc ObjectArrayList [Refactor] Remove CollectionUtils.sortAndDedup to use java TreeSet instead of hppc ObjectArrayList Mar 30, 2023
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@nknize nknize force-pushed the refactorCollectionUtils branch from 4b9d24d to 33943f5 Compare March 30, 2023 05:10
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@nknize nknize changed the title [Refactor] Remove CollectionUtils.sortAndDedup to use java TreeSet instead of hppc ObjectArrayList [Refactor] Remove CollectionUtils.sortAndDedup and use java TreeSet instead of hppc ObjectArrayList Mar 30, 2023
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

HPPC ObjectArrayLists provide no benefit in CollectionUtils.sortAndDedup and
adds an unnecessary library dependency. This commit removes that dependency on
hppc by switching the sortAndDeup method to use java.util.Lists.
BinaryFieldMapper logic is updated to use the generic java list.

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
@nknize
Copy link
Collaborator Author

nknize commented Mar 30, 2023

I opened a separate PR to explore improving the bytes() method in CustomBinaryDocValuesField.

Note also that I have an upstream Lucene issue opened for multi value support for binary doc values since the size limit was removed. We have a community member interested in contributing their implementation so it's partially stalled. I'd like to not overwork this here in hopes that it will be migrated as a top level Lucene capability.

I'll poke that issue again. If there's no movement we'll float the implementation upstream. @navneet1v has expressed desire to work on this as a first Lucene contribution so we could move this conversation there for his awareness if/when he works on adding it.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@nknize nknize requested review from reta and andrross March 30, 2023 17:25
@reta
Copy link
Collaborator

reta commented Mar 30, 2023

LGTM from my side, @andrross wdyt?

@nknize
Copy link
Collaborator Author

nknize commented Mar 30, 2023

Thanks all! I've moved the performance conversation to #6897. Getting this multi-value support in Lucene will be a big step (especially for geo_shape doc values! and geo_ aggs).

@andrross
Copy link
Member

@nknize I see you tagged this for 3.0. Any reason not to backport? I don't see any usage of sortAndDedup beyond this code.

@andrross andrross merged commit 4859e15 into opensearch-project:main Mar 30, 2023
@nknize
Copy link
Collaborator Author

nknize commented Mar 30, 2023

I see you tagged this for 3.0. Any reason not to backport?

No we can retag as v2.7.0 and backport

@nknize nknize added v2.7.0 backport 2.x Backport to 2.x branch and removed v3.0.0 Issues and PRs related to version 3.0.0 labels Mar 30, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request Mar 30, 2023
…nstead of hppc ObjectArrayList (#6884)

* [Refactor] CollectionUtils.sortAndDedup to use java lists

HPPC ObjectArrayLists provide no benefit in CollectionUtils.sortAndDedup and
adds an unnecessary library dependency. This commit removes that dependency on
hppc by switching the sortAndDeup method to use java.util.Lists.
BinaryFieldMapper logic is updated to use the generic java list.

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>

* close BytesStreamOutput in CustomBinaryDocValuesField.bytes

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>

---------

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
(cherry picked from commit 4859e15)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
reta pushed a commit that referenced this pull request Apr 1, 2023
…nstead of hppc ObjectArrayList (#6884) (#6902)

* [Refactor] CollectionUtils.sortAndDedup to use java lists

HPPC ObjectArrayLists provide no benefit in CollectionUtils.sortAndDedup and
adds an unnecessary library dependency. This commit removes that dependency on
hppc by switching the sortAndDeup method to use java.util.Lists.
BinaryFieldMapper logic is updated to use the generic java list.



* close BytesStreamOutput in CustomBinaryDocValuesField.bytes



---------


(cherry picked from commit 4859e15)

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
mitrofmep pushed a commit to mitrofmep/OpenSearch that referenced this pull request Apr 5, 2023
…nstead of hppc ObjectArrayList (opensearch-project#6884)

* [Refactor] CollectionUtils.sortAndDedup to use java lists

HPPC ObjectArrayLists provide no benefit in CollectionUtils.sortAndDedup and
adds an unnecessary library dependency. This commit removes that dependency on
hppc by switching the sortAndDeup method to use java.util.Lists.
BinaryFieldMapper logic is updated to use the generic java list.

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>

* close BytesStreamOutput in CustomBinaryDocValuesField.bytes

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>

---------

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
Signed-off-by: Valentin Mitrofanov <mitrofmep@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants