Multi-level Nested Sort with Filters #26395

mattweber · 2017-08-25T22:27:16Z

Allow multiple levels of nested sorting where each level can have it's own filter. This allows us to have complex nested sort criteria just like we have with queries and aggregations. This is backwards compatible with previous single-level nested sort.

@martijnvg @jpountz Can you please review?

Allow multple levels of nested sorting where each level can have it's own filter. Backward compatible with previous single-level nested sort.

elasticmachine · 2017-08-25T22:27:17Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

martijnvg

@mattweber This looks great and I like the new syntax for nested sorting! I left a few comments.

martijnvg · 2017-08-28T10:49:17Z

core/src/test/java/org/elasticsearch/search/nested/SimpleNestedIT.java

-//        assertThat(searchResponse.getHits().getHits()[1].sortValues()[0].toString(), equalTo("-2"));
-//        assertThat(searchResponse.getHits().getHits()[2].getId(), equalTo("1"));
-//        assertThat(searchResponse.getHits().getHits()[2].sortValues()[0].toString(), equalTo("-1"));
+        assertThat(searchResponse.getHits().getHits()[0].getId(), equalTo("3"));


martijnvg · 2017-08-28T10:50:44Z

core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java

        order = in.readOptionalWriteable(SortOrder::readFromStream);
        sortMode = in.readOptionalWriteable(SortMode::readFromStream);
        unmappedType = in.readOptionalString();
+        if (in.getVersion().onOrAfter(Version.V_5_6_0)) {


5.6 and 6.0 branches are in feature freeze, so the next version this feature can be included would be 6.1

martijnvg · 2017-08-28T12:39:50Z

docs/reference/search/request/sort.asciidoc

-existing sort options:
+field support has a `nested` sort option with the following properties:

-`nested_path`::


The migration should document that nested_path and nested_filter are deprecated.

martijnvg · 2017-08-28T12:54:00Z

core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java

        PARSER.declareString(FieldSortBuilder::unmappedType , UNMAPPED_TYPE);
        PARSER.declareString((b, v) -> b.order(SortOrder.fromString(v)) , ORDER_FIELD);
        PARSER.declareString((b, v) -> b.sortMode(SortMode.fromString(v)), SORT_MODE);
        PARSER.declareObject(FieldSortBuilder::setNestedFilter, (p, c) -> SortBuilder.parseNestedFilter(p), NESTED_FILTER_FIELD);


I think we should add some deprecation logging here too for the nested_filter and nested_path parameters.

private static final DeprecationLogger DEPRECATION_LOGGER = new DeprecationLogger(Loggers.getLogger(ParseField.class)); ... PARSER.declareString((fieldSortBuilder, nestedPath) -> { DEPRECATION_LOGGER.deprecated("[nested_path] has been deprecated in favor of the [nested] parameter"); fieldSortBuilder.setNestedPath(nestedPath); } , NESTED_PATH_FIELD); ... PARSER.declareObject(FieldSortBuilder::setNestedFilter, (p, c) -> { DEPRECATION_LOGGER.deprecated("[nested_filter] has been deprecated in favour for the [nested] parameter"); return SortBuilder.parseNestedFilter(p);}, NESTED_FILTER_FIELD); ...

martijnvg · 2017-08-28T12:58:19Z

test this please

mattweber · 2017-08-28T15:53:08Z

@martijnvg Thanks for the review! Thats a bummer about waiting for 6.1, I was hoping it could make a 5.x release since it is backward compatible. I have addressed your comments, please let me know if you would like any other changes.

jpountz

Even thoug I think nested is often abused, I don't like the fact that queries and aggregations allow for multiple levels of nesting but sorting doesn't, so +1 to expose a way to make it work.

jpountz · 2017-08-28T14:54:38Z

core/src/main/java/org/elasticsearch/search/MultiValueMode.java

-                assert rootDocs.get(rootDoc) : "can only sort root documents";
                assert rootDoc >= lastSeenRootDoc : "can only evaluate current and upcoming root docs";
                if (rootDoc == lastSeenRootDoc) {
                    return lastEmittedOrd != -1;


let's rename rootDoc and lastSeenRootDoc to parentDoc and lastSeenParentDoc in that file?

jpountz · 2017-08-28T15:52:45Z

core/src/main/java/org/elasticsearch/search/sort/SortBuilder.java

        PARSERS = unmodifiableMap(parsers);
    }

+    public static class NestedSort implements Writeable, Reader<NestedSort>, ToXContentObject {


Even though it is not strictly a sort builder, can you rename it NestedSortBuilder for consistency and move it to its own class?

jpountz · 2017-08-28T15:53:39Z

core/src/main/java/org/elasticsearch/search/sort/SortBuilder.java

+        private QueryBuilder filter;
+        private NestedSort nestedSort;
+
+        public NestedSort() {


should we requite the path in the constructor since it is mandatory?

mattweber · 2017-08-28T19:19:31Z

Thanks @jpountz, I addressed your comments. How does this look?

martijnvg · 2017-08-29T09:02:50Z

retest this please

martijnvg · 2017-08-29T09:40:24Z

@mattweber Thanks for fixing the comments. The PR looks good! Only the build failed with checkstyle violations. Can you fix this?

mattweber · 2017-08-29T17:46:10Z

@martijnvg @jpountz Thank you for reviewing everything. Just pushed checkstyle and test fixes. I think this will do it.

martijnvg · 2017-08-30T06:41:09Z

retest this please

martijnvg · 2017-08-30T10:16:43Z

The mixed cluster test failures that occurred in the build this PR triggered are related to the fact that the changes aren't yet in the 6.x branch. This causes serialization errors when a 7.0.0-alph1 node reads the shard level search request from a 6.1.0-SNAPSHOT node. So I'll merge this change both to master and 6.x branches at the same time to minimize the regular builds from running into this failure.

jasontedor · 2017-08-30T10:26:26Z

The versions and skips should be set so that they cause no failures, backported with version updates, then another commit to master moving the version to the 6.x version they are compatible with.

jasontedor · 2017-08-30T10:28:56Z

Here's another PR where I expand on this strategy; this is the build-failure minimizing strategy, not merging as quickly as possible. Instead, we should take our time to get a green build on master, then backport (with the version bumps) and getting a green build again, and then a final commit to get the versions right still maintaining green status.

martijnvg · 2017-08-30T10:47:10Z

@jasontedor Good point. I'll follow the same steps mentioned in that PR.

martijnvg · 2017-08-30T12:33:55Z

I merged squashed the change into the master branch via 6377afa
and set the version to 7.0.0-alph1 via 472a5dd

I'll backport this change to 6.x branch later today unless a build failure related to this change occurs.

@mattweber Thanks for this contribution to nested sorting!

martijnvg · 2017-08-30T12:43:45Z

I just realized that as part of the git merge (git merge --squash) that I became the author of this change :(
Not sure what would be best to restore this mistake. (I can only think of reverting the commit and then commit it again with the @mattweber as author)

jasontedor · 2017-08-30T12:46:26Z

@martijnvg For future reference, you can push commits to PRs so you could have pushed the version-changing commit to this PR, then merged from the GitHub UI (preserving @mattweber as the author). As far as correcting, I think that you can:

revert the commits that you pushed to master
reopen this PR
push the version-changing commit to this PR
merge this PR from the GitHub UI

elasticmachine · 2017-08-30T12:56:54Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

martijnvg · 2017-08-30T13:42:54Z

@mattweber Can you change the 6.1.0 version check to 7.0.0-alpha1? (as it turns out I can't make this change in your pr) After that I'll hit the squash and merge button and do they backporting to 6.x branch.

jpountz · 2017-08-30T14:10:09Z

@martijnvg I'm wondering whether that would work with the online editor, eg. https://github.com/mattweber/elasticsearch/edit/multi_nested_sort/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java?pr=/elastic/elasticsearch/pull/26395

…til the change has been backported to 6.x branch.

martijnvg · 2017-08-30T14:24:41Z

retest this please

martijnvg · 2017-08-30T14:26:01Z

@jpountz Your idea worked :)

I'll squash and merge once the build is green.

mattweber · 2017-08-30T14:38:05Z

Wow lots going on. Thanks @martijnvg @jpountz @jasontedor! Let me know if you need me to do anything.

Allow multiple levels of nested sorting where each level can have it's own filter. Backward compatible with previous single-level nested sort.

martijnvg · 2017-08-30T18:00:06Z

backported to 6.x branch via: 2425da1 and 42c7745

Changed the version check in master via ea3fa76

Multi-level Nested Sort with Filters

a73f76c

Allow multple levels of nested sorting where each level can have it's own filter. Backward compatible with previous single-level nested sort.

mattweber added 2 commits August 27, 2017 10:17

Multi-level nested filter on script sort.

b80c1f2

Multi-level nested filter on geo sort.

4abb221

martijnvg reviewed Aug 28, 2017

View reviewed changes

martijnvg added :Search/Search Search-related issues that do not fall into other categories >feature v6.1.0 v7.0.0 :Nested Docs and removed :Search/Search Search-related issues that do not fall into other categories labels Aug 28, 2017

Deprecation warnings + documentation, 6.1 version check.

67c6e2b

jpountz reviewed Aug 28, 2017

View reviewed changes

mattweber added 4 commits August 28, 2017 09:54

NestedSort -> NestedSortBuilder

dbd6c37

Move NestedSortBuilder to its own class.

3f0517e

rootDocs -> parentDocs, innerDocs -> childDocs, etc

b9d4d6d

NestedSortBuilder with path in constructor.

789b402

jpountz approved these changes Aug 29, 2017

View reviewed changes

Checkstyle and test fixes.

9d169da

martijnvg closed this Aug 30, 2017

martijnvg added the backport pending label Aug 30, 2017

martijnvg reopened this Aug 30, 2017

martijnvg added 3 commits August 30, 2017 16:21

Temporarily set bwc version for new nested sorting to 7.0.0-alpha1 un…

9009a5a

…til the change has been backported to 6.x branch.

Temporarily set bwc version for new nested sorting to 7.0.0-alpha1 un…

2d07db4

…til the change has been backported to 6.x branch.

Temporarily set bwc version for new nested sorting to 7.0.0-alpha1 un…

d7c2316

…til the change has been backported to 6.x branch.

mattweber mentioned this pull request Aug 30, 2017

[Tests] Improve testing of FieldSortBuilder #26437

Merged

martijnvg merged commit 140395c into elastic:master Aug 30, 2017

martijnvg pushed a commit that referenced this pull request Aug 30, 2017

Multi-level Nested Sort with Filters (#26395)

2425da1

Allow multiple levels of nested sorting where each level can have it's own filter. Backward compatible with previous single-level nested sort.

martijnvg removed the backport pending label Aug 30, 2017

clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Nested Docs labels Feb 14, 2018

andyb-elastic mentioned this pull request Mar 21, 2018

Support multi level nested filters inside sort #15797

Closed

jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Multi-level Nested Sort with Filters #26395

Multi-level Nested Sort with Filters #26395

Uh oh!

Conversation

mattweber commented Aug 25, 2017

Uh oh!

elasticmachine commented Aug 25, 2017

Uh oh!

martijnvg left a comment

Choose a reason for hiding this comment

Uh oh!

martijnvg Aug 28, 2017

Choose a reason for hiding this comment

Uh oh!

martijnvg Aug 28, 2017

Choose a reason for hiding this comment

Uh oh!

martijnvg Aug 28, 2017

Choose a reason for hiding this comment

Uh oh!

martijnvg Aug 28, 2017

Choose a reason for hiding this comment

Uh oh!

martijnvg commented Aug 28, 2017

Uh oh!

mattweber commented Aug 28, 2017

Uh oh!

jpountz left a comment

Choose a reason for hiding this comment

Uh oh!

jpountz Aug 28, 2017

Choose a reason for hiding this comment

Uh oh!

jpountz Aug 28, 2017

Choose a reason for hiding this comment

Uh oh!

jpountz Aug 28, 2017

Choose a reason for hiding this comment

Uh oh!

mattweber commented Aug 28, 2017

Uh oh!

martijnvg commented Aug 29, 2017

Uh oh!

martijnvg commented Aug 29, 2017

Uh oh!

mattweber commented Aug 29, 2017

Uh oh!

martijnvg commented Aug 30, 2017

Uh oh!

martijnvg commented Aug 30, 2017

Uh oh!

jasontedor commented Aug 30, 2017

Uh oh!

jasontedor commented Aug 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martijnvg commented Aug 30, 2017

Uh oh!

martijnvg commented Aug 30, 2017

Uh oh!

martijnvg commented Aug 30, 2017

Uh oh!

jasontedor commented Aug 30, 2017

Uh oh!

elasticmachine commented Aug 30, 2017

Uh oh!

martijnvg commented Aug 30, 2017

Uh oh!

jpountz commented Aug 30, 2017

Uh oh!

martijnvg commented Aug 30, 2017

Uh oh!

martijnvg commented Aug 30, 2017

Uh oh!

mattweber commented Aug 30, 2017

Uh oh!

martijnvg commented Aug 30, 2017

Uh oh!

Reviewers

Assignees

jasontedor commented Aug 30, 2017 •

edited

Loading