Skip to content
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.

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

Fix redundant assignment in QueryStringQueryBuilder #119775

Merged
merged 1 commit into from
Jan 8, 2025

Conversation

ioanatia
Copy link
Contributor

@ioanatia ioanatia commented Jan 8, 2025

We have a redundant assignment in QueryStringQueryBuilder for extractedFields:

if (allFields && this.field != null && this.field.equals(field)) {
// "*" is the default field
extractedFields = fieldsAndWeights;
}
boolean multiFields = Regex.isSimpleMatchPattern(field);
// Filters unsupported fields if a pattern is requested
// Filters metadata fields if all fields are requested
extractedFields = resolveMappingField(
context,
field,
1.0f,
allFields == false,
multiFields == false,
quoted ? quoteFieldSuffix : null
);

To fix this we can remove the extractedFields = fieldsAndWeights assignment altogether.
However fieldsAndWeights already contains the same result of the second assignment.
fieldsAndWeights is actually initialised with the exact same value (when the redundant assignment is executed) - see the call to resolveMappingField:

public QueryStringQueryParser(SearchExecutionContext context, boolean lenient) {
this(context, "*", resolveMappingField(context, "*", 1.0f, false, false, null), lenient);
}

When matching all fields I think it was always the intention to just reuse fieldsAndWeights, but the redundant assignment and extra computation was added as a regression in #55158

In the context of a single query_string execution, extractMultiFields can be called multiple times. Some examples where the redundant assignment is triggered and we end up extracting the fields for wildcard queries multiple times:

  • something OR nothing OR anything - 3 redundant assignments and recomputing the available fields, instead of relying on the stored fieldsAndWeights.
  • *:something AND nothing - 2 redundant assignments

This is why I decided to keep the extractedFields = fieldsAndWeights assignment and just to make sure we don't call resolveMappingField unnecessarily.

@ioanatia ioanatia added >non-issue auto-backport Automatically create backport pull requests when merged :Search Relevance/Search Catch all for Search Relevance v8.18.0 labels Jan 8, 2025
@ioanatia ioanatia requested a review from jimczi January 8, 2025 16:28
Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

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

Good catch, LGTM

@ioanatia ioanatia marked this pull request as ready for review January 8, 2025 19:31
@elasticsearchmachine elasticsearchmachine added the Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch label Jan 8, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-search-relevance (Team:Search Relevance)

@ioanatia ioanatia merged commit b05ab7a into elastic:main Jan 8, 2025
16 checks passed
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.x

ioanatia added a commit to ioanatia/elasticsearch that referenced this pull request Jan 8, 2025
@ioanatia ioanatia deleted the query_string_query_fix branch January 9, 2025 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Automatically create backport pull requests when merged >non-issue :Search Relevance/Search Catch all for Search Relevance Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants