Add build() method to SortBuilder implementations #17146
Conversation
For the current refactoring of SortBuilders related to elastic#10217, each SortBuilder should get a build() method that produces a SortField according to the SortBuilder parameters on the shard. This change also slightly refactors the current parse method in SortParseElement to extract an internal parse method that returns a list of sort fields only needs a QueryShardContext as input instead of a full SearchContext. This allows using this internal parse method for testing.
|
@MaineC could you take a look at this? The generic test added to AbstractSortTestCase doesn't do many assertions on the two SortFields it compares, maybe you have an idea how to add to those tests. Also, those tests will be temporary and will be obsolete once we remove the old SortParseElement. |
| throw new QueryShardException(context, "Sorting not supported for field[" + fieldName + "]"); | ||
| } | ||
|
|
||
| // Enable when we also know how to detect fields that do tokenize, but only emit one token |
There was a problem hiding this comment.
Instead of having code commented out here - would it be better to track that in some follow up issue? (I know the comment was there before the refactoring, but code that's commented out always smells strange IMHO)
There was a problem hiding this comment.
Just talked to @martijnvg, this is old code and can be removed.
|
Overall looks awesome! Left a few minor comments and questions. About testing: I'm not sure we can test a whole lot more without knowing which type of sort we are dealing with. One idea (that would also be valid after removing the old parsing code) would be to add tests to the individual builders that have a few deeper assertions. With this change it should already be possible to parse more than one sort element, no? This could be an addition to the tests. |
|
@MaineC thanks, I adressed most of your comments, still need to look into the Median-SortMode support and maybe open an issue about sorting with missing values.
I will see if we can add a "assertSortField" method to each individual test that does some more inspection, maybe that works
No, the SortParseElement will parse a whole |
Also adding checks for median SortMode on non-numeric field types to FieldSortBuilder, removing some unused code and switching GeoDistanceSortBuilder to using ParseField.
|
@MaineC I also adressed the rest of your comments, can you take another look if this looks good now? |
|
Looked over your changes and the comments you added - LGTM |
|
@MaineC thanks, I needed to move over few more lines from NestedInnerQueryParseSupport to the part in SortBuilder that is building the nested filters. I added those and merged in master once again. |
cbuescher
force-pushed
the
sort-add-build
branch
from
6473eed to
dfce045
Compare
cbuescher
force-pushed
the
sort-add-build
branch
from
dfce045 to
697174d
Compare
For the refactoring of SortBuilders related to #10217, each SortBuilder needs to get a build() method that produces a SortField according to the SortBuilder parameters on the shard.
clintongormley
added
:Search/Search
For the current refactoring of SortBuilders related to #10217,
each SortBuilder should get a build() method that produces a
SortField according to the SortBuilder parameters on the shard.
This change also slightly refactors the current parse method in
SortParseElement to extract an internal parse method that returns
a list of sort fields only needs a QueryShardContext as input
instead of a full SearchContext. This allows using this internal
parse method for testing.