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 sort values coming from unrelated documents in certain multi-level sorts #31554 #31776

Closed
wants to merge 0 commits into from

Conversation

dbevacqua
Copy link
Contributor

Root filter of Nested returned from SortBuilder produces no docs due to incorrect MUST clause. By removing it and just using ToParentBlockJoinQuery on its own the correct docs are produced.

@martijnvg martijnvg added >bug :Search/Search Search-related issues that do not fall into other categories labels Jul 4, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

@martijnvg
Copy link
Member

test this please

@dbevacqua
Copy link
Contributor Author

I don't think the build failure is related to my commit. Failing test is an x-pack one:

10:56:05 > Task :x-pack:qa:rolling-upgrade:without-system-key:v6.4.0-SNAPSHOT#twoThirdsUpgradedTestRunner FAILED
10:56:05 Tests with failures:
10:56:05   - org.elasticsearch.upgrades.UpgradeClusterClientYamlTestSuiteIT.test {p0=mixed_cluster/30_ml_jobs_crud/Test get old cluster job}

@martijnvg
Copy link
Member

@dbevacqua Yes, that is a ml test and is completely unrelated to the change. Can you merge master into your branch and push? This should trigger a new build.

@dbevacqua
Copy link
Contributor Author

@martijnvg done (though no new build so far...)

@martijnvg
Copy link
Member

test this please

@martijnvg
Copy link
Member

@dbevacqua Thanks! I'm going to add some more tests, so that there is more test coverage for multi level nested sorting. Right now that test coverage is poor. I think your fix is good, but having more tests should also increase confidence in this fix and whether multi level nested sorting is actual working.

@martijnvg
Copy link
Member

test this please

@martijnvg
Copy link
Member

@jpountz Would you like to take a look at this when you have time?

@jimczi
Copy link
Contributor

jimczi commented Jul 19, 2018

I am very sorry @dbevacqua, I tried to push to your branch to update this pr like @martijnvg did but I made something wrong and the pr got closed automatically. We discussed with @martijnvg about a simplification that should also fix #31783.
I opened a new pr with your commits and this additional change in #32204 which should add you as a co-author. Again sorry for the trouble.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Search/Search Search-related issues that do not fall into other categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants