Skip to content

Lucene BBQ Integration WIP#3144

Open
jack-hung-lgtm wants to merge 3 commits intoopensearch-project:mainfrom
jack-hung-lgtm:jack-bbq-integration-feature-fix
Open

Lucene BBQ Integration WIP#3144
jack-hung-lgtm wants to merge 3 commits intoopensearch-project:mainfrom
jack-hung-lgtm:jack-bbq-integration-feature-fix

Conversation

@jack-hung-lgtm
Copy link

@jack-hung-lgtm jack-hung-lgtm commented Mar 5, 2026 •
edited
Loading

Description

Integrates Lucene's BBQ and makes it the default when possible. Building off of this PR from @adityamachiroutu: https://github.com/opensearch-project/k-NN/pull/2838/changes#diff-98b317fe02be43dadfabe498a20936d7a56046c43fef3c9533b39d01e4ab4248

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

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.

@jack-hung-lgtm jack-hung-lgtm changed the title Lucene BBQ Integration Lucene BBQ Integration WIP Mar 5, 2026
@jack-hung-lgtm jack-hung-lgtm marked this pull request as draft March 5, 2026 21:01
@Vikasht34
Copy link
Collaborator

Vikasht34 commented Mar 6, 2026

Will review this PR once we have all integs test successfully run.

@jack-hung-lgtm
Copy link
Author

jack-hung-lgtm commented Mar 6, 2026

Will review this PR once we have all integs test successfully run.

@jack-hung-lgtm jack-hung-lgtm reopened this Mar 6, 2026
@jack-hung-lgtm jack-hung-lgtm force-pushed the jack-bbq-integration-feature-fix branch 6 times, most recently from 2d0fe3a to ecf7046 Compare March 10, 2026 23:32
@jack-hung-lgtm jack-hung-lgtm marked this pull request as ready for review March 11, 2026 00:38
navneet1v
navneet1v reviewed Mar 11, 2026
View reviewed changes
navneet1v
navneet1v reviewed Mar 11, 2026
View reviewed changes
@navneet1v
Copy link
Collaborator

navneet1v commented Mar 11, 2026 •
edited
Loading

Please make sure that you pass all the CIs

naveentatikonda
naveentatikonda reviewed Mar 11, 2026
View reviewed changes
src/main/java/org/opensearch/knn/index/mapper/CompressionLevel.java Outdated
if (this == x32 && engine == KNNEngine.LUCENE && version.onOrAfter(Version.V_3_6_0)) {
if (dimension <= RescoreContext.DIMENSION_THRESHOLD) {
return RescoreContext.builder()
.oversampleFactor(RescoreContext.OVERSAMPLE_FACTOR_BELOW_DIMENSION_THRESHOLD)
Copy link
Member

@naveentatikonda naveentatikonda Mar 11, 2026

Choose a reason for hiding this comment

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

I think based on our discussion after reviewing the benchmarking results we want to use a default oversampling factor of 2.0 irrespective of the dimension of the vector.

Also, why are we adding this outside of the above if condition ?
if (modesForRescore.contains(mode)) {

@jack-hung-lgtm jack-hung-lgtm marked this pull request as draft March 11, 2026 23:04
@jack-hung-lgtm jack-hung-lgtm marked this pull request as draft March 11, 2026 23:04
@jack-hung-lgtm
Copy link
Author

jack-hung-lgtm commented Mar 11, 2026

Temporarily putting as draft - streamlining implementation for the new KNN1040 codec

@navneet1v navneet1v mentioned this pull request Mar 16, 2026
Open
12 tasks
@jack-hung-lgtm jack-hung-lgtm force-pushed the jack-bbq-integration-feature-fix branch 4 times, most recently from 1967ab9 to 43ba471 Compare March 18, 2026 00:40
@jack-hung-lgtm jack-hung-lgtm marked this pull request as ready for review March 18, 2026 00:42
@jack-hung-lgtm
Copy link
Author

jack-hung-lgtm commented Mar 18, 2026

Opening pull request for early review. However, some CIs are still failing so I'll be debugging that in the meantime

@jack-hung-lgtm jack-hung-lgtm force-pushed the jack-bbq-integration-feature-fix branch 3 times, most recently from 867af09 to 08c4d8b Compare March 18, 2026 17:33
kotwanikunal
kotwanikunal reviewed Mar 18, 2026
View reviewed changes
...in/java/org/opensearch/knn/index/codec/params/KNN1040ScalarQuantizedVectorsFormatParams.java Outdated

@Override
public boolean validate(final Map<String, Object> params) {
return encoderName != null && SUPPORTED_ENCODERS.contains(encoderName);
Copy link
Member

@kotwanikunal kotwanikunal Mar 18, 2026

Choose a reason for hiding this comment

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

We would also need to validate the bits right? We only support 1-bit currently

Copy link
Author

@jack-hung-lgtm jack-hung-lgtm Mar 19, 2026

Choose a reason for hiding this comment

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

There is a validation for the LuceneBBQEncoder class that ensures the user only provides bits = 1 with BBQ, is that enough? I have a test case that also makes sure it throws exception if the user provides an invalid number of bits for BBQ

@jack-hung-lgtm jack-hung-lgtm force-pushed the jack-bbq-integration-feature-fix branch from 08c4d8b to 2fad3cc Compare March 18, 2026 18:57
@0ctopus13prime 0ctopus13prime force-pushed the jack-bbq-integration-feature-fix branch from 2fad3cc to db0a8c8 Compare March 19, 2026 00:43
@jack-hung-lgtm jack-hung-lgtm force-pushed the jack-bbq-integration-feature-fix branch from db0a8c8 to 4fc9797 Compare March 19, 2026 01:05
@jack-hung-lgtm jack-hung-lgtm force-pushed the jack-bbq-integration-feature-fix branch from 4fc9797 to 84116a0 Compare March 19, 2026 01:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@navneet1v navneet1v navneet1v left review comments

@naveentatikonda naveentatikonda naveentatikonda left review comments

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

@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

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

@martin-gaievski martin-gaievski Awaiting requested review from martin-gaievski martin-gaievski is a code owner

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

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

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

@0ctopus13prime 0ctopus13prime Awaiting requested review from 0ctopus13prime 0ctopus13prime is a code owner

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

+1 more reviewer

@kotwanikunal kotwanikunal kotwanikunal left review comments

Reviewers whose approvals may not affect merge requirements

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jack-hung-lgtm @Vikasht34 @navneet1v @kotwanikunal @naveentatikonda @adityamachiroutu