Skip to content

Use IndexOrDocValuesQuery in NumberFieldType#termQuery implementations #128293

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
merged 12 commits into from
May 23, 2025
Merged

Use IndexOrDocValuesQuery in NumberFieldType#termQuery implementations #128293

merged 12 commits into from
May 23, 2025

Conversation

iverase
Copy link
Contributor

@iverase iverase commented May 22, 2025 •
edited
Loading

Use IndexOrDocValuesQuery whenever possible so for low cardinality fields we don't need to allocated a FixedBitSet if the query is together with more restrictive filters.

@iverase iverase requested a review from javanna May 22, 2025 10:05
@elasticsearchmachine elasticsearchmachine added the Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch label May 22, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-search-foundations (Team:Search Foundations)

@elasticsearchmachine
Copy link
Collaborator

Hi @iverase, I've created a changelog YAML for you.

javanna
javanna reviewed May 23, 2025
View reviewed changes
Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

I left a couple of small comments. thanks a lot, this makes sense to me. I wonder if our nightlies already cover the scenarios that will be improved by this change, and whether there are scenarios where there could be regressions.

@iverase
Copy link
Contributor Author

iverase commented May 23, 2025

I wonder if our nightlies already cover the scenarios that will be improved by this change, and whether there are scenarios where there could be regressions.

IndexOrDocValues queries is used in other parts and in my experience it seems pretty solid implementation so any regressions should be considered a bug in the IndexOrDocValues implementation.

javanna
javanna approved these changes May 23, 2025
View reviewed changes
Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

LGTM, thanks again!

@iverase
Copy link
Contributor Author

iverase commented May 23, 2025

It seems this change is making PushQueriesIT to fail in an unexpected way. I am having a hard time understanding what's going on.

@iverase iverase added the auto-backport Automatically create backport pull requests when merged label May 23, 2025
@iverase iverase merged commit de7c91c into elastic:main May 23, 2025
18 checks passed
@iverase iverase deleted the termsQueryNum branch May 23, 2025 14:58
@iverase iverase mentioned this pull request May 23, 2025
Merged
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
✅ 8.19

elasticsearchmachine pushed a commit that referenced this pull request May 23, 2025
@iverase
[8.19] Use IndexOrDocValuesQuery in NumberFieldType#termQuery impleme…
b94cd56 
…ntations (#128293) (#128394)

* Use IndexOrDocValuesQuery in NumberFieldType#termQuery implementations (#128293)

* Update x-pack/plugin/esql/qa/server/single-node/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/single_node/PushQueriesIT.java
@iverase iverase mentioned this pull request May 24, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javanna javanna javanna approved these changes

Assignees
No one assigned
Labels
auto-backport Automatically create backport pull requests when merged >enhancement :Search Foundations/Search Catch all for Search Foundations Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch v8.19.0 v9.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@iverase @elasticsearchmachine @javanna