Skip to content

KeywordField.newSetQuery() to reuse prefixed terms in IndexOrDocValuesQuery #14435

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.

Sign up for GitHub

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

Jump to bottom
Merged
mkhludnev merged 2 commits into from
Apr 5, 2025
Merged

KeywordField.newSetQuery() to reuse prefixed terms in IndexOrDocValuesQuery #14435

mkhludnev merged 2 commits into from
Apr 5, 2025

Conversation

@mkhludnev
Copy link
Member

@mkhludnev mkhludnev commented Apr 3, 2025

fix #14425

Description

jainankitk
jainankitk reviewed Apr 3, 2025
View reviewed changes
lucene/core/src/java/org/apache/lucene/document/KeywordField.java
Comment on lines +177 to +178
return TermInSetQuery.newIndexOrDocValuesQuery(
MultiTermQuery.CONSTANT_SCORE_BLENDED_REWRITE, field, values);
Copy link
Contributor

@jainankitk jainankitk Apr 3, 2025

Choose a reason for hiding this comment

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

Probably we can use TermInSetQuery.newIndexOrDocValuesQuery(field, values) here?

Copy link
Member Author

@mkhludnev mkhludnev Apr 4, 2025

Choose a reason for hiding this comment

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

Right. Just want to make it explicit.

Copy link
Contributor

@jainankitk jainankitk Apr 4, 2025

Choose a reason for hiding this comment

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

Then we don't need that method, right?

Copy link
Contributor

@jpountz jpountz Apr 4, 2025

Choose a reason for hiding this comment

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

I would agree with this. KeywordField#newSetQuery is the convenience method, so it's nice to not have to pass a rewrite, TermInSetQuery#newIndexOrDocValuesQuery is the expert method, and experts can figure out how they want their multi-term queries to be rewritten.

Copy link
Contributor

@jainankitk jainankitk Apr 4, 2025

Choose a reason for hiding this comment

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

We should remove the unused method TermInSetQuery#newIndexOrDocValuesQuery(String field, Collection<BytesRef> terms) in that case, as anyone invoking TermInSetQuery#newIndexOrDocValuesQuery should use the expert method TermInSetQuery#newIndexOrDocValuesQuery(RewriteMethod indexRewriteMethod, String field, Collection<BytesRef> terms)?

Copy link
Member Author

@mkhludnev mkhludnev Apr 5, 2025

Choose a reason for hiding this comment

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

ok. Got it. Since we add something, let's add as least as possible.

Copy link
Contributor

@jainankitk jainankitk Apr 5, 2025

Choose a reason for hiding this comment

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

Thanks for making the change. I know its minor, but important for keeping it clean!

@mkhludnev mkhludnev changed the title KeywordField.newSetQuery() to uses prefixed terms for IndexOrDocValuesQuery KeywordField.newSetQuery() to reuse prefixed terms in IndexOrDocValuesQuery Apr 4, 2025
jpountz
jpountz approved these changes Apr 4, 2025
View reviewed changes
lucene/core/src/java/org/apache/lucene/document/KeywordField.java
Comment on lines +177 to +178
return TermInSetQuery.newIndexOrDocValuesQuery(
MultiTermQuery.CONSTANT_SCORE_BLENDED_REWRITE, field, values);
Copy link
Contributor

@jpountz jpountz Apr 4, 2025

Choose a reason for hiding this comment

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

I would agree with this. KeywordField#newSetQuery is the convenience method, so it's nice to not have to pass a rewrite, TermInSetQuery#newIndexOrDocValuesQuery is the expert method, and experts can figure out how they want their multi-term queries to be rewritten.

@mkhludnev mkhludnev merged commit 219cd5e into apache:main Apr 5, 2025
7 checks passed
@mkhludnev mkhludnev deleted the idx-or-dv-term-inset-q branch April 5, 2025 19:09
mkhludnev added a commit to mkhludnev/lucene that referenced this pull request Apr 5, 2025
@mkhludnev
KeywordField.newSetQuery() to reuse prefixed terms in IndexOrDocValue…
b0d04e7 
…sQuery (apache#14435)

* KeywordField.newSetQuery() reuses prefixed terms.

fix apache#14425
@mkhludnev mkhludnev mentioned this pull request Apr 5, 2025
Merged
mkhludnev added a commit that referenced this pull request Apr 5, 2025
@mkhludnev
KeywordField.newSetQuery() to reuse prefixed terms in IndexOrDocValue…
5b27d44 
…sQuery (#14435) (#14442)

* KeywordField.newSetQuery() reuses prefixed terms.

fix #14425
This was referenced Apr 12, 2025
Merged
Closed
mkhludnev added a commit that referenced this pull request Apr 12, 2025
@mkhludnev
KeywordField.newSetQuery() to reuse prefixed terms in IndexOrDocValue…
dec50d6 
…sQuery (#14435) (#14478)

* KeywordField.newSetQuery() reuses prefixed terms.

fix #14425
@mkhludnev mkhludnev mentioned this pull request Sep 20, 2025
Merged
@tkykenmt tkykenmt mentioned this pull request Jan 11, 2026
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jpountz jpountz jpountz approved these changes

@jainankitk jainankitk jainankitk approved these changes

Assignees

No one assigned

Labels

module:core/search

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Reuse packedTerms between two TermInSetQuery which are combined by IndexOrDocValuesQuery

3 participants

@mkhludnev @jpountz @jainankitk