Skip to content

Conversation

@jdconrad
Copy link
Contributor

This fixes two bugs:

  1. toXContent was writing two arrays instead of one array for sub_searches as part of SearchSourceBuilder
  2. Removes sub_searches from ShardSearchRequestTests.setForceSyntheticSource

Closes #96896
Closes #96910

@elasticsearchmachine elasticsearchmachine added needs:triage Requires assignment of a team area label v8.10.0 labels Jun 20, 2023
@jdconrad jdconrad requested a review from javanna June 20, 2023 19:29
@jdconrad jdconrad added >bug :Search Relevance/Ranking Scoring, rescoring, rank evaluation. v8.9.0 auto-backport-and-merge and removed needs:triage Requires assignment of a team area label labels Jun 20, 2023
@elasticsearchmachine elasticsearchmachine added the Team:Search Meta label for search team label Jun 20, 2023
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine
Copy link
Collaborator

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

@benwtrent
Copy link
Member

@jdconrad since this is an unreleased feature, I don't think >bug tag is necessary. You probably don't want the changelog populated.

I suggest using >non-issue

builder.field(QUERY_FIELD.getPreferredName(), subSearchSourceBuilders.get(0).getQueryBuilder());
} else {
builder.array(SUB_SEARCHES_FIELD.getPreferredName(), subSearchSourceBuilders);
builder.xContentList(SUB_SEARCHES_FIELD.getPreferredName(), subSearchSourceBuilders);
Copy link
Member

Choose a reason for hiding this comment

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

TIL about xContentList nice.

Copy link
Member

@benwtrent benwtrent left a comment

Choose a reason for hiding this comment

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

This should be >non-issue and the changelog removed. This isn't really a bug fix on a released version. Just fixing stuff that hasn't been released yet.

SearchRequest request = createSearchRequest();
if (request.source() != null) {
request.source().rankBuilder(null);
request.source().subSearches(new ArrayList<>());
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't do this. It will remove all coverage for when there is a single query. Does this test fail when there is just one subSearch (e.g. just a query?)

Copy link
Contributor Author

@jdconrad jdconrad Jun 20, 2023

Choose a reason for hiding this comment

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

Good point! I'll only remove it if size >= 2.

@jdconrad jdconrad added >non-issue and removed >bug labels Jun 20, 2023
Copy link
Member

@benwtrent benwtrent left a comment

Choose a reason for hiding this comment

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

:shipit:

@jdconrad jdconrad removed the request for review from javanna June 20, 2023 20:00
@jdconrad
Copy link
Contributor Author

@benwtrent Thanks for the review!

@jdconrad jdconrad merged commit 56037c2 into elastic:main Jun 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>non-issue :Search Relevance/Ranking Scoring, rescoring, rank evaluation. Team:Search Meta label for search team v8.9.0 v8.10.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CI] ShardSearchRequestTests testForceSyntheticUnsupported failing [CI] SearchSourceBuilderTests testFromXContent failing

3 participants