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

feat: Implement new api for nested sorting for elasticsearch >= 6.1 #71

Merged
merged 3 commits into from
Oct 3, 2018
Merged

feat: Implement new api for nested sorting for elasticsearch >= 6.1 #71

merged 3 commits into from
Oct 3, 2018

Conversation

dimatill
Copy link
Contributor

Adds compatibility of new nested api for elasticsearch >= 6.1
Removes deprecated nested_path and nested_filter options.
https://www.elastic.co/guide/en/elasticsearch/reference/6.3/search-request-sort.html#nested-sorting

WARNING: This is breaking change and this will NOT work with elasticsearch < 6.1

Usage example:

const sort = esb.sort('offer.price', 'asc')
    .nested({
        path: 'offer',
        filter: esb.termQuery('offer.color', 'blue')
    });

or with old style:

const sort = esb.sort('offer.price', 'asc')
    .nestedPath('offer')
    .nestedFilter(esb.termQuery('offer.color', 'blue'));

both examples produce the same output compatible with new elasticsearch api:

{
    'offer.price': {
        order: 'asc',
        nested: {
            path: 'offer',
            filter: {
                term: {
                    'offer.color': 'blue'
                }
            }
        }
    }
}

@sudo-suhas
Copy link
Owner

Since the options are only deprecated, I am leaning towards not making this breaking change for now. But I think a better system is required for managing compatibility with different versions of elasticsearch. For now, how about the compromise of only adding the nested method on Sort for elasticsearch >=6.1 without modifying the existing methods?

@XBeg9
Copy link

XBeg9 commented Sep 27, 2018

@dimatill could you please update PR with requested changes? Thanks

@dimatill
Copy link
Contributor Author

Sounds good! Will update

@sudo-suhas
Copy link
Owner

@dimatill While you are making the changes, also add some documentation notes for the following:

  • Indicate that the existing methods are deprecated starting with elasticsearch 6.1
  • On the newly added method, clearly call out that it would work only on elasticsearch >= 6.1

@dimatill
Copy link
Contributor Author

@sudo-suhas updated

Copy link
Owner

@sudo-suhas sudo-suhas left a comment

Choose a reason for hiding this comment

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

Could you please oblige the minor change requests for docs?

On a side note, having to maintain docs in 2 places is not ideal. Maybe I should do a rewrite using typescript?

src/core/sort.js Outdated Show resolved Hide resolved
src/index.d.ts Show resolved Hide resolved
@sudo-suhas sudo-suhas merged commit df07e59 into sudo-suhas:master Oct 3, 2018
@sudo-suhas
Copy link
Owner

Thank you @dimatill for the PR 👍 The changes have been published in elastic-builder@2.2.0 🎉

@XBeg9
Copy link

XBeg9 commented Oct 3, 2018

Thank you, @sudo-suhas

@dimatill
Copy link
Contributor Author

dimatill commented Oct 3, 2018

Thank you @sudo-suhas

@pierremalletneo9
Copy link

Hello, Thx for the amazing work!
Just to be sure, this PR doesn't handle multiple nested sort? Like described here https://www.elastic.co/guide/en/elasticsearch/reference/6.2/search-request-sort.html#nested-sorting

@dimatill
Copy link
Contributor Author

dimatill commented Apr 3, 2019

@pierremalletneo9 multiple nested sort should work

const sort = esb.sort('offer.price', 'asc')
    .nested({
        path: 'offer',
        filter: esb.termQuery('offer.color', 'blue'),
        nested: {
          path: 'offer',
          filter: esb.termQuery('offer.color', 'blue')
        }
    });

this code produces such output:

{
  "offer.price": {
    "order": "asc",
    "nested": {
      "path": "offer",
      "filter": {
        "term": {
          "offer.color": "blue"
        }
      },
      "nested": {
        "path": "offer",
        "filter": {
          "term": {
            "offer.color": "blue"
          }
        }
      }
    }
  }
}

@sudo-suhas
Copy link
Owner

Thank you @dimatill for taking the time to answer. I appreciate it.

@pierremalletneo9
Copy link

pierremalletneo9 commented Apr 3, 2019

Wow thanks for the fast answer @dimatill . It works fine with your solution. Its just that the typescript definition does not allow sub nested.
But with a "as any" workaround its all good.

esb.sort(path, sort.order).nested({
	path: 'nestedPath',
	filter: esb.boolQuery().filter(filters1),
	nested: {
		path: 'subNestedPath',
		filter: esb.boolQuery().filter(filters1)
	}
} as any).missing("_last");

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants