Skip to content

Conversation

@jimczi
Copy link
Contributor

@jimczi jimczi commented Jun 3, 2019

This commit removes the nested_path and nested_filter options deprecated in 6x.
This change also checks that the sort field has a [nested] option if it is under a nested
object and throws an exception if it's not the case.

Closes #27098

This commit removes the nested_path and nested_filter options deprecated in 6x.
This change also checks that the sort field has a [nested] option if it is under a nested
object and throws an exception if it's not the case.

Closes elastic#27098
@jimczi jimczi added >breaking :Search/Search Search-related issues that do not fall into other categories v8.0.0 labels Jun 3, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

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

Left some small nits, nothing that needs another round of review I think once the tests pass.

[float]
===== Removal of sort parameters

* The `nested_filter` and `nested_path` options, deprecated in 6.x, has been removed in favor of the `nested` context.
Copy link
Member

Choose a reason for hiding this comment

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

nit: s/has/have

// new nested sorts takes priority
nested = resolveNested(context, nestedSort);
} else {
nested = resolveNested(context, nestedPath, nestedFilter);
Copy link
Member

Choose a reason for hiding this comment

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

Looks like SortBuilder#resolveNested(QueryShardContext context, String nestedPath, QueryBuilder nestedFilter) is unused after this now and can be removed. I quickly tried it and didn't get compile errors, I might be missign sth. though.

/**
* Throws an exception if the provided <code>field</code> requires a nested context.
*/
static void validateMissingNestedPath(QueryShardContext context, String field) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe worth adding a unit test for this.

Copy link
Member

Choose a reason for hiding this comment

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

Scratch that, just was that FieldSortIT#testNestedSort covers at least the exception. I'll leave it up to you to decide if there are other cases that need checking.

@jimczi jimczi merged commit a614415 into elastic:master Jun 27, 2019
@jimczi jimczi deleted the nested_sort_options branch June 27, 2019 15:30
pgomulka added a commit that referenced this pull request Aug 5, 2021
previously removed in #42809. nested_path and nested_filter sort options will now throw exceptions suggesting to use nested option
relates #51816
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incorrect Documentation: nested_path

4 participants