Skip to content

Conversation

@erayarslan
Copy link
Contributor

FIX
related with (#33587 (comment))

If we have query like that;

GET /products/_search
{
  "sort": [
    {
      "variants.listings.price": {
        "order": "desc",
        "nested": {
          "path": "variants",
          "nested": {
            "max_children": 1,
            "path": "variants.listings"
          }
        }
      }
    }
  ]
}

Elastic going to ignore max_children=1, cause we passed top level nested sort object in here (example flow);

if (nestedSort != null) {
if (nestedSort.getNestedSort() != null && nestedSort.getMaxChildren() != Integer.MAX_VALUE) {
throw new QueryShardException(context,
"max_children is only supported on last level of nested sort");
}
nested = resolveNested(context, nestedSort);
} else {

protected static Nested resolveNested(QueryShardContext context, NestedSortBuilder nestedSort) throws IOException {
final Query childQuery = resolveNestedQuery(context, nestedSort, null);
if (childQuery == null) {
return null;
}
final ObjectMapper objectMapper = context.nestedScope().getObjectMapper();
final Query parentQuery;
if (objectMapper == null) {
parentQuery = Queries.newNonNestedFilter();
} else {
parentQuery = objectMapper.nestedTypeFilter();
}
return new Nested(context.bitsetFilter(parentQuery), childQuery, nestedSort, context.searcher());
}

} else {
final BitSet rootDocs = nested.rootDocs(context);
final DocIdSetIterator innerDocs = nested.innerDocs(context);
final int maxChildren = nested.getNestedSort() != null ? nested.getNestedSort().getMaxChildren() : Integer.MAX_VALUE;
selectedValues = sortMode.select(values, dMissingValue, rootDocs, innerDocs, context.reader().maxDoc(), maxChildren);
}

So i think we must use max_children on top level nested sort or we need to pass last level nested sort.

In this PR, I allow to max_children on top level nested sort.

@jimczi jimczi added :Search/Search Search-related issues that do not fall into other categories >enhancement v7.5.0 v8.0.0 labels Sep 18, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

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 @erayarslan , I left a minor comment but the change looks good.

@erayarslan erayarslan force-pushed the max-children-on-top-level-nested-sort branch from 2a8ad79 to 46cb29b Compare September 18, 2019 10:30
@erayarslan
Copy link
Contributor Author

Thanks @jimczi , I updated integration test.

Can you please take a look?

@erayarslan erayarslan requested a review from jimczi September 18, 2019 10:36
@jimczi
Copy link
Contributor

jimczi commented Sep 20, 2019

@elasticmachine test this please

@erayarslan erayarslan force-pushed the max-children-on-top-level-nested-sort branch from 46cb29b to 61370d0 Compare September 20, 2019 07:14
@erayarslan
Copy link
Contributor Author

CI failed. I think my branch outdated. Now, its okay.
Can you trigger CI again? @jimczi

@jimczi
Copy link
Contributor

jimczi commented Sep 20, 2019

@elasticmachine test this please

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.

LGTM, thanks @erayarslan

@jimczi jimczi merged commit 88a4a86 into elastic:master Sep 23, 2019
jimczi pushed a commit that referenced this pull request Sep 23, 2019
This commit restricts the usage of max_children to the top level nested sort since it is ignored on the other levels.
@erayarslan
Copy link
Contributor Author

thank you for your time @jimczi

@jimczi jimczi mentioned this pull request Feb 25, 2020
jimczi added a commit that referenced this pull request Feb 26, 2020
This change fixes the incomplete backport of #46731 in 7.x (as of 7.5).
We now check if `max_children` is set on the top level nested sort and fails with an
exception if it's not the case.

Relates #46731
Closes #52202
jimczi added a commit that referenced this pull request Feb 26, 2020
This change fixes the incomplete backport of #46731 in 7.x (as of 7.5).
We now check if `max_children` is set on the top level nested sort and fails with an
exception if it's not the case.

Relates #46731
Closes #52202
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>enhancement :Search/Search Search-related issues that do not fall into other categories v7.5.0 v8.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants