Fix graph merge stats size calculation by ryanbogan · Pull Request #1844 · opensearch-project/k-NN

ryanbogan · 2024-07-17T16:37:25Z

Description

Fixes the calculations for size of merges in the graph stats section of KNNStats API. This PR changes the logic to properly round values to the correct number of bytes.

Continuation of #1818, which had too many merge conflicts to fix cleanly.

Issues Resolved

#1789

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed as per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Ryan Bogan <rbogan@amazon.com>

ryanbogan · 2024-07-17T16:57:00Z

BWC failures are unrelated to this PR

luyuncheng

LGTM

Signed-off-by: Ryan Bogan <rbogan@amazon.com>

navneet1v · 2024-07-26T20:51:17Z

-                vectorsSize += vectorsSize % JAVA_ROUNDING_NUMBER;
-            }
-            return vectorsSize;
+        if (serializationMode == SerializationMode.COLLECTIONS_OF_BYTES) {


can we get rid of this serializationMode attribute completely?

This method is used to calculate array size from a Pair class typically. The issue is that the KNNCodecUtil.Pair class only has doc id's, a vector address, dimension, and serialization mode as instance variables. Therefore, without reading memory from the vector address I don't think it's possible to differentiate whether the data is floats or bytes without the serialization mode.

Can we use vector datatype to know if the vector is byte or float?

Yeah I think that would work, binary type would be the same calculation as byte right?

yes same thing

Signed-off-by: Ryan Bogan <rbogan@amazon.com>

opensearch-trigger-bot · 2024-08-08T00:18:19Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-1844-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 e3158f990d058b02568da617688fd4857d0d521b
# Push it to GitHub
git push --set-upstream origin backport/backport-1844-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-1844-to-2.x.

* Fix graph merge stats size calculation Signed-off-by: Ryan Bogan <rbogan@amazon.com> * Add changelog entry Signed-off-by: Ryan Bogan <rbogan@amazon.com> * Add javadocs Signed-off-by: Ryan Bogan <rbogan@amazon.com> * Make calculations easier to read Signed-off-by: Ryan Bogan <rbogan@amazon.com> * Remove java overhead from calculations Signed-off-by: Ryan Bogan <rbogan@amazon.com> * Change from serialization mode to vector data type for calculations Signed-off-by: Ryan Bogan <rbogan@amazon.com> * Minor change to if statements Signed-off-by: Ryan Bogan <rbogan@amazon.com> --------- Signed-off-by: Ryan Bogan <rbogan@amazon.com> (cherry picked from commit e3158f9)

* Fix graph merge stats size calculation Signed-off-by: Ryan Bogan <rbogan@amazon.com> * Add changelog entry Signed-off-by: Ryan Bogan <rbogan@amazon.com> * Add javadocs Signed-off-by: Ryan Bogan <rbogan@amazon.com> * Make calculations easier to read Signed-off-by: Ryan Bogan <rbogan@amazon.com> * Remove java overhead from calculations Signed-off-by: Ryan Bogan <rbogan@amazon.com> * Change from serialization mode to vector data type for calculations Signed-off-by: Ryan Bogan <rbogan@amazon.com> * Minor change to if statements Signed-off-by: Ryan Bogan <rbogan@amazon.com> --------- Signed-off-by: Ryan Bogan <rbogan@amazon.com>

Fix graph merge stats size calculation

6b27be7

Signed-off-by: Ryan Bogan <rbogan@amazon.com>

ryanbogan requested review from VijayanB, heemin32, jmazanec15, junqiu-lei, luyuncheng, martin-gaievski, naveentatikonda, navneet1v and vamshin as code owners July 17, 2024 16:37

Add changelog entry

b0863f2

Signed-off-by: Ryan Bogan <rbogan@amazon.com>

luyuncheng previously approved these changes Jul 18, 2024

View reviewed changes

ryanbogan added backport 2.x v2.16.0 labels Jul 18, 2024

Add javadocs

609301e

Signed-off-by: Ryan Bogan <rbogan@amazon.com>

ryanbogan dismissed luyuncheng’s stale review via 609301e July 18, 2024 17:51

ryanbogan requested a review from luyuncheng July 18, 2024 17:52

heemin32 reviewed Jul 18, 2024

View reviewed changes

Comment thread src/main/java/org/opensearch/knn/index/codec/util/KNNCodecUtil.java Outdated

Make calculations easier to read

c1e3109

Signed-off-by: Ryan Bogan <rbogan@amazon.com>

ryanbogan requested a review from heemin32 July 18, 2024 19:14

ryanbogan added v2.17.0 and removed v2.16.0 labels Jul 23, 2024

ryanbogan added 2 commits July 26, 2024 10:26

Rebase with main

472ec21

Signed-off-by: Ryan Bogan <rbogan@amazon.com>

Remove java overhead from calculations

1db8365

Signed-off-by: Ryan Bogan <rbogan@amazon.com>

heemin32 previously approved these changes Jul 26, 2024

View reviewed changes

navneet1v reviewed Jul 26, 2024

View reviewed changes

Merge branch 'main' into graph_size_bug_v2

e3b1e9d

ryanbogan dismissed heemin32’s stale review via e3b1e9d August 6, 2024 18:47

Change from serialization mode to vector data type for calculations

a3a1dc1

Signed-off-by: Ryan Bogan <rbogan@amazon.com>

navneet1v reviewed Aug 6, 2024

View reviewed changes

Comment thread CHANGELOG.md Outdated

Comment thread src/main/java/org/opensearch/knn/index/codec/util/KNNCodecUtil.java

Minor change to if statements

acddd87

Signed-off-by: Ryan Bogan <rbogan@amazon.com>

heemin32 approved these changes Aug 7, 2024

View reviewed changes

navneet1v approved these changes Aug 7, 2024

View reviewed changes

ryanbogan merged commit e3158f9 into opensearch-project:main Aug 8, 2024

ryanbogan deleted the graph_size_bug_v2 branch August 8, 2024 00:18

ryanbogan mentioned this pull request Aug 8, 2024

[Backport 2.x] Fix graph merge stats size calculation #1941

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix graph merge stats size calculation#1844

Fix graph merge stats size calculation#1844
ryanbogan merged 9 commits intoopensearch-project:mainfrom
ryanbogan:graph_size_bug_v2

ryanbogan commented Jul 17, 2024 •

edited

Loading

Uh oh!

ryanbogan commented Jul 17, 2024

Uh oh!

luyuncheng left a comment

Uh oh!

Uh oh!

navneet1v Jul 26, 2024

Uh oh!

ryanbogan Jul 29, 2024 •

edited

Loading

Uh oh!

navneet1v Aug 5, 2024

Uh oh!

ryanbogan Aug 5, 2024

Uh oh!

navneet1v Aug 6, 2024

Uh oh!

Uh oh!

Uh oh!

opensearch-trigger-bot bot commented Aug 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

ryanbogan commented Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Check List

Uh oh!

ryanbogan commented Jul 17, 2024

Uh oh!

luyuncheng left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

navneet1v Jul 26, 2024

Choose a reason for hiding this comment

Uh oh!

ryanbogan Jul 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

navneet1v Aug 5, 2024

Choose a reason for hiding this comment

Uh oh!

ryanbogan Aug 5, 2024

Choose a reason for hiding this comment

Uh oh!

navneet1v Aug 6, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

opensearch-trigger-bot bot commented Aug 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ryanbogan commented Jul 17, 2024 •

edited

Loading

ryanbogan Jul 29, 2024 •

edited

Loading