Skip to content

Add parent join support for lucene knn#1182

Merged
heemin32 merged 1 commit intoopensearch-project:mainfrom
heemin32:join
Oct 10, 2023
Merged

Add parent join support for lucene knn#1182
heemin32 merged 1 commit intoopensearch-project:mainfrom
heemin32:join

Conversation

@heemin32
Copy link
Copy Markdown
Collaborator

@heemin32 heemin32 commented Oct 2, 2023

Description

Call DiversifyingChildren[Byte|Float]KnnVectorQuery for nested field so that k number of parent document can be returned in search result

Issues Resolved

#1065

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.

This was referenced Oct 2, 2023
Closed
Merged
@navneet1v
Copy link
Copy Markdown
Collaborator

navneet1v commented Oct 2, 2023

@heemin32 can you add an example how you tested this change?

martin-gaievski
martin-gaievski reviewed Oct 2, 2023
View reviewed changes
@heemin32
Copy link
Copy Markdown
Collaborator Author

heemin32 commented Oct 3, 2023

@heemin32 can you add an example how you tested this change?

Added test scenario in #1065

@heemin32 heemin32 force-pushed the join branch 2 times, most recently from 3536368 to 91c200d Compare October 3, 2023 21:20
@heemin32
Add parent join support for lucene knn
9cc75c7 
Call DiversifyingChildren[Byte|Float]KnnVectorQuery for nested field so that k number of parent document can be returned in search result

Signed-off-by: Heemin Kim <heemin@amazon.com>
@codecov
Copy link
Copy Markdown

codecov bot commented Oct 3, 2023 •
edited
Loading

Codecov Report

Merging #1182 (9cc75c7) into main (78aba55) will increase coverage by 0.06%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #1182      +/-   ##
============================================
+ Coverage     84.95%   85.01%   +0.06%     
- Complexity     1190     1210      +20     
============================================
  Files           159      160       +1     
  Lines          4838     4932      +94     
  Branches        440      449       +9     
============================================
+ Hits           4110     4193      +83     
- Misses          530      540      +10     
- Partials        198      199       +1
Files Coverage Δ
...n/java/org/opensearch/knn/common/KNNConstants.java 94.11% <ø> (ø)
...rg/opensearch/knn/index/query/KNNQueryFactory.java 86.27% <100.00%> (-1.45%) ⬇️

... and 6 files with indirect coverage changes

navneet1v
navneet1v reviewed Oct 10, 2023
View reviewed changes
src/main/java/org/opensearch/knn/index/query/KNNQueryFactory.java
}

log.debug(String.format("Creating Lucene k-NN query for index: %s \"\", field: %s \"\", k: %d", indexName, fieldName, k));
BitSetProducer parentFilter = createQueryRequest.context == null ? null : createQueryRequest.context.getParentFilter();
Copy link
Copy Markdown
Collaborator

@navneet1v navneet1v Oct 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

who is setting this parent filter in the CreateQueryRequest?

Copy link
Copy Markdown
Collaborator Author

@heemin32 heemin32 Oct 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

opensearch-project/OpenSearch#10246

@navneet1v
Copy link
Copy Markdown
Collaborator

navneet1v commented Oct 10, 2023

@heemin32 can we add benchmarks around this feature, what your thought?

@heemin32
Copy link
Copy Markdown
Collaborator Author

heemin32 commented Oct 10, 2023

@heemin32 can we add benchmarks around this feature, what your thought?

That is a great idea. However, I would like to defer the effort until there is strong needs for the benchmark as we need to construct test data by ourselves for the nested field which might take some amount of efforts.

@heemin32 heemin32 merged commit a496926 into opensearch-project:main Oct 10, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 10, 2023
@heemin32 @github-actions
Add parent join support for lucene knn (#1182)
e366ffd 
Call DiversifyingChildren[Byte|Float]KnnVectorQuery for nested field so that k number of parent document can be returned in search result

Signed-off-by: Heemin Kim <heemin@amazon.com>
(cherry picked from commit a496926)
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Oct 10, 2023
Merged
heemin32 added a commit to heemin32/k-NN that referenced this pull request Oct 11, 2023
@heemin32
Add parent join support for lucene knn (opensearch-project#1182)
288f120 
Call DiversifyingChildren[Byte|Float]KnnVectorQuery for nested field so that k number of parent document can be returned in search result

Signed-off-by: Heemin Kim <heemin@amazon.com>
(cherry picked from commit a496926)
heemin32 added a commit to heemin32/k-NN that referenced this pull request Nov 7, 2023
@heemin32
Add parent join support for lucene knn (opensearch-project#1182)
055ce17 
Call DiversifyingChildren[Byte|Float]KnnVectorQuery for nested field so that k number of parent document can be returned in search result

Signed-off-by: Heemin Kim <heemin@amazon.com>
(cherry picked from commit a496926)
heemin32 added a commit to heemin32/k-NN that referenced this pull request Nov 7, 2023
@heemin32
Add parent join support for lucene knn (opensearch-project#1182)
b7539b9 
Call DiversifyingChildren[Byte|Float]KnnVectorQuery for nested field so that k number of parent document can be returned in search result

Signed-off-by: Heemin Kim <heemin@amazon.com>
(cherry picked from commit a496926)
heemin32 added a commit that referenced this pull request Nov 7, 2023
@opensearch-trigger-bot @heemin32
Add parent join support for lucene knn (#1182) (#1230)
f32f25a 
Call DiversifyingChildren[Byte|Float]KnnVectorQuery for nested field so that k number of parent document can be returned in search result

Signed-off-by: Heemin Kim <heemin@amazon.com>
(cherry picked from commit a496926)

Co-authored-by: Heemin Kim <heemin@amazon.com>
@heemin32 heemin32 deleted the join branch July 18, 2024 17:18
@martin-gaievski martin-gaievski mentioned this pull request Sep 12, 2024
Closed
jingqimao77-spec pushed a commit to jingqimao77-spec/k-NN that referenced this pull request Mar 15, 2026
@heemin32
Add parent join support for lucene knn (opensearch-project#1182)
1443a48 
Call DiversifyingChildren[Byte|Float]KnnVectorQuery for nested field so that k number of parent document can be returned in search result

Signed-off-by: Heemin Kim <heemin@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@navneet1v navneet1v navneet1v approved these changes

@martin-gaievski martin-gaievski martin-gaievski approved these changes

@VijayanB VijayanB Awaiting requested review from VijayanB VijayanB is a code owner

@vamshin vamshin Awaiting requested review from vamshin vamshin is a code owner

@jmazanec15 jmazanec15 Awaiting requested review from jmazanec15 jmazanec15 is a code owner

@naveentatikonda naveentatikonda Awaiting requested review from naveentatikonda naveentatikonda is a code owner

@junqiu-lei junqiu-lei Awaiting requested review from junqiu-lei junqiu-lei is a code owner

Assignees

No one assigned

Labels

backport 2.x v2.12.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@heemin32 @navneet1v @martin-gaievski