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

Fix TopHitsAggregationBuilder adding duplicate _score sort clauses #42179

Merged
merged 2 commits into from
May 22, 2019
Merged

Fix TopHitsAggregationBuilder adding duplicate _score sort clauses #42179

merged 2 commits into from
May 22, 2019

Conversation

Hohol
Copy link
Contributor

@Hohol Hohol commented May 16, 2019

When using High Level Rest Client Java API to produce search query, using AggregationBuilders.topHits("th").sort("_score", SortOrder.DESC)
caused query to contain duplicate sort clauses:

"sort": [
    {
        "_score": {
            "order": "desc"
        }
    },
    {
        "_score": {
            "order": "desc"
        }
    }
],

Fixes #42154

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo

@iverase
Copy link
Contributor

iverase commented May 20, 2019

Thanks for the PR! Changes look good, would you mind adding a test for this specific bug (always good to test explicitly :). After that, I can start the merging process.

@Hohol
Copy link
Contributor Author

Hohol commented May 20, 2019

I added a unit test, but I'm not sure if it's good enough.
I checked what happens if we use the old version of sorting by _score in TopHitsAggregatorTests and it just causes a search to fail with exceptions. So I've made a test that simply checks if there are no exceptions.

If that's not enough, could you suggest how to test it more properly?

@iverase
Copy link
Contributor

iverase commented May 21, 2019

The test is enough, I will start now the CI checks and if successful I merge the PR. Thanks again!

@elasticmachine test this please

Copy link
Contributor

@iverase iverase left a comment

Choose a reason for hiding this comment

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

LGTM

@Hohol
Copy link
Contributor Author

Hohol commented May 21, 2019

Looks like these CI failures are not caused by my changes.

@iverase
Copy link
Contributor

iverase commented May 21, 2019

Indeed, there were some problems with version bumps and a PR has just been merged to address it. May I ask you to merge master into your branch so I can restart the Jobs?

Sorry about this extra work.

@Hohol
Copy link
Contributor Author

Hohol commented May 21, 2019

Done.

@iverase
Copy link
Contributor

iverase commented May 21, 2019

@elasticmachine test this please

@iverase iverase added the >bug label May 21, 2019
@Hohol
Copy link
Contributor Author

Hohol commented May 21, 2019

Again, some weird error in elasticsearch-ci/bwc.

@iverase
Copy link
Contributor

iverase commented May 21, 2019

@elasticmachine run elasticsearch-ci/bwc

@Hohol
Copy link
Contributor Author

Hohol commented May 21, 2019

Failed again.
"stack_trace" : "ResourceNotFoundException[No datafeed with id [old-cluster-datafeed-with-aggs] exists]
Still doesn't look like caused by this PR.

@iverase
Copy link
Contributor

iverase commented May 21, 2019

Yes, I do not see any relation between this PR and the errors in the job. I will trigger it again and if it fails again I will look into it more in depth.

@iverase
Copy link
Contributor

iverase commented May 21, 2019

@elasticmachine run elasticsearch-ci/bwc

@polyfractal
Copy link
Contributor

Builds have been relatively unhealthy today. There was a broken bwc test (muted here: 455cf8b) which wasn't obvious for a while because there were other errors from the version bump.

Not sure at what point you merged master in, but you'll need to get past 455cf8b at the very least. Might be easiest to just let the builds settle down a day or so and try again once they are green.

Apologies for the hassle! We can lay some of the red builds at my feet 😳

@Hohol
Copy link
Contributor Author

Hohol commented May 21, 2019

Merge was before 455cf8b
I've rebased PR

@polyfractal
Copy link
Contributor

<3 thanks @Hohol

@elasticmachine test this please

@iverase
Copy link
Contributor

iverase commented May 22, 2019

@elasticmachine run elasticsearch-ci/1
@elasticmachine run elasticsearch-ci/2

@iverase iverase merged commit 9609497 into elastic:master May 22, 2019
@iverase
Copy link
Contributor

iverase commented May 22, 2019

Thanks for the PR @Hohol, I will back port it now to 7.x.

iverase pushed a commit to iverase/elasticsearch that referenced this pull request May 22, 2019
…lastic#42179)

When using High Level Rest Client Java API to produce search query, using AggregationBuilders.topHits("th").sort("_score", SortOrder.DESC)
caused query to contain duplicate sort clauses.
iverase added a commit that referenced this pull request May 22, 2019
…42179) (#42343)

When using High Level Rest Client Java API to produce search query, using AggregationBuilders.topHits("th").sort("_score", SortOrder.DESC)
caused query to contain duplicate sort clauses.
gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this pull request May 27, 2019
…lastic#42179)

When using High Level Rest Client Java API to produce search query, using AggregationBuilders.topHits("th").sort("_score", SortOrder.DESC)
caused query to contain duplicate sort clauses.
@Hohol Hohol deleted the fix-duplicate-score branch July 9, 2019 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TopHitsAggregationBuilder adds duplicate _score sort clauses
6 participants