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

Remove deprecated reverse option from sorting #17282

Merged
merged 8 commits into from Mar 30, 2016

Conversation

Projects
None yet
3 participants
@MaineC
Copy link
Contributor

commented Mar 23, 2016

This removes the "reverse" option for sorting as discussed in #17047 - instead define sort order explicitly like shown here: https://www.elastic.co/guide/en/elasticsearch/reference/current/search-request-sort.html#_sort_order

Relates to #17047

Pending: PR to deprecate the option removed above in 2.x

Isabel Drost-Fromm
@cbuescher

This comment has been minimized.

Copy link
Member

commented Mar 23, 2016

I found one mention of the 'reverse' parameter related to sort in ./reference/search/request/sort.asciidoc which should be removed. Maybe also add a not to the migrate.asciidoc, even though the option will be deprecated in 2.x. Other than that, LGTM

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2016

can we add a test that makes sure we fail is somebody uses the option?

@MaineC

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2016

Thanks for the speedy comments.

  • @cbuescher wrt. sort in docs: Will remove the mention, same for adding to migrate.asciidoc
  • @s1monw wrt. to adding said test: Makes sense, will do

Isabel Drost-Fromm added some commits Mar 24, 2016

Isabel Drost-Fromm
Merge branch 'master' into deprecation/sort-option-reverse-removal
Conflicts:
	core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java
	core/src/main/java/org/elasticsearch/search/sort/ScoreSortBuilder.java
Isabel Drost-Fromm
Disallow reverse option and check it throws an exception
This adds tests to check that using reverse as sort option is disallowed
and throws an exception.
@MaineC

This comment has been minimized.

Copy link
Contributor Author

commented Mar 24, 2016

Updated. Not sure about the change to GeoDistanceSortBuilder - added an explicit check for the token "reverse", without it parsing fell through to parsing the reference geo point for some field from a geohash string.

Would like to dig a bit more to confirm this behaviour outside the unit test with a real curl reproduction.

Isabel Drost-Fromm
Merge branch 'master' into deprecation/sort-option-reverse-removal
Conflicts:
	core/src/main/java/org/elasticsearch/search/sort/ScoreSortBuilder.java
	core/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java
@MaineC

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2016

This is what we get when a user includes the reverse sort option after removing its support in GeoDistanceSortBuilder:

{
   "error": {
      "root_cause": [
         {
            "type": "parse_exception",
            "reason": "illegal latitude value [269.97802734375] for [GeoDistanceSort]"
         }
      ],    
      "type": "search_phase_execution_exception",
      "reason": "all shards failed",
      "phase": "query",
      "grouped": true,
      "failed_shards": [
         {
            "shard": 0,
            "index": "products",
            "node": "MeeEqVKMQF2UvjooWHAGlw",
            "reason": {
               "type": "parse_exception",
               "reason": "illegal latitude value [269.97802734375] for [GeoDistanceSort]"
            }
         }
      ],
      "caused_by": {
         "type": "parse_exception",
         "reason": "illegal latitude value [269.97802734375] for [GeoDistanceSort]"
      }
   },
   "status": 400
}

When explicitly forbidding the query option like in the change above, we get something that's more readable IMHO while at the same time forbidding the user to have geo fields named "reverse":

{
   "error": {
      "root_cause": [
         {
            "type": "parsing_exception",
            "reason": "Sort option [reverse] no longer supported.",
            "line": 6,
            "col": 20
         }
      ],
      "type": "parsing_exception",
      "reason": "Sort option [reverse] no longer supported.",
      "line": 6,
      "col": 20
   },
   "status": 400
}

My personal preference would be to go with the latter option, @cbuescher any opinion?

Isabel Drost-Fromm
@cbuescher

This comment has been minimized.

Copy link
Member

commented Mar 29, 2016

@MaineC I see the problem, but I'm on the fence here with treating "reverse" as a special case that cannot be used as a field name for the geo point option. That would require special documentation (all fieldnames allowed except...) and somehow doesn't feel right to me. What if instead to mitigate the problem:

  • dissallow setting the field name for the geo points twice and throw an error with the fieldname already set and the different one we are trying to set (that would give errors sth. like `fieldname already set to "my_point" but trying to reset to "reverse")
  • dissallow token value other than VALUE_STRING in the case where we are parsing to geo hash. That would catch the cases where we have "reverse" : true in the query.
  • Add the fieldname to the error message above (I think that gets triggered in build()), so users get a hint that they are using the wrong field when accidentally specifying "reverse" : "true"|"false"
Isabel Drost-Fromm
Refine dealing with removed reverse sort option.
For geo distance sort parsing: Disallow anything but
VALUE_STRING as geo hash, disallow resetting field
name for geo fields.

Also make error message for wrong lat/lon values more
verbose by including the affected field name.
@MaineC

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2016

@cbuescher good points, I like your proposals. Changed the code accordingly. Only gotcha:

Add the fieldname to the error message above (I think that gets triggered in build()), so users get a
hint that they are using the wrong field when accidentally specifying "reverse" : "true"|"false"

While the string "false" evaluates to an invalid coordinate, the string "true" seems to evaluate to a valid one. See here: http://www.movable-type.co.uk/scripts/geohash.html

@@ -433,6 +434,13 @@ public static GeoDistanceSortBuilder fromXContent(QueryParseContext context, Str
nestedFilter = context.parseInnerQueryBuilder();
} else {
// the json in the format of -> field : { lat : 30, lon : 12 }
if (fieldName != null) {

This comment has been minimized.

Copy link
@cbuescher

cbuescher Mar 29, 2016

Member

Nit: maybe we could make this fieldName.equals(currentName) == false, so this won't fail when same name is used multiple times. Might potentially happen when generating queries?

@cbuescher

This comment has been minimized.

Copy link
Member

commented Mar 29, 2016

Okay, I'd say we have to live with the small gotcha that "true" as geohash is valid (nice, Kasachstan ;-)), better than making exceptions on allowed fieldnames only for catching (probably largely unused) deprecated options. Left one last minor nit, otherwise LGTM.

@MaineC

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2016

@cbuescher NIT fixed. Merging as soon as tests are done.

@MaineC MaineC merged commit f27399d into elastic:master Mar 30, 2016

1 check passed

CLA Commit author is a member of Elasticsearch
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.