{"action":"created","issue":{"url":"https://api.github.com/repos/elastic/elasticsearch/issues/30320","repository_url":"https://api.github.com/repos/elastic/elasticsearch","labels_url":"https://api.github.com/repos/elastic/elasticsearch/issues/30320/labels{/name}","comments_url":"https://api.github.com/repos/elastic/elasticsearch/issues/30320/comments","events_url":"https://api.github.com/repos/elastic/elasticsearch/issues/30320/events","html_url":"https://github.com/elastic/elasticsearch/pull/30320","id":319387393,"node_id":"MDExOlB1bGxSZXF1ZXN0MTg1MzE4OTY5","number":30320,"title":"Refactored geogrid to support multiple hash types","user":{"login":"nyurik","id":1641515,"node_id":"MDQ6VXNlcjE2NDE1MTU=","avatar_url":"https://avatars0.githubusercontent.com/u/1641515?v=4","gravatar_id":"","url":"https://api.github.com/users/nyurik","html_url":"https://github.com/nyurik","followers_url":"https://api.github.com/users/nyurik/followers","following_url":"https://api.github.com/users/nyurik/following{/other_user}","gists_url":"https://api.github.com/users/nyurik/gists{/gist_id}","starred_url":"https://api.github.com/users/nyurik/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/nyurik/subscriptions","organizations_url":"https://api.github.com/users/nyurik/orgs","repos_url":"https://api.github.com/users/nyurik/repos","events_url":"https://api.github.com/users/nyurik/events{/privacy}","received_events_url":"https://api.github.com/users/nyurik/received_events","type":"User","site_admin":false},"labels":[{"id":141079527,"node_id":"MDU6TGFiZWwxNDEwNzk1Mjc=","url":"https://api.github.com/repos/elastic/elasticsearch/labels/:Search/Geo","name":":Search/Geo","color":"0e8a16","default":false},{"id":23172,"node_id":"MDU6TGFiZWwyMzE3Mg==","url":"https://api.github.com/repos/elastic/elasticsearch/labels/%3Efeature","name":">feature","color":"006b75","default":false},{"id":929267538,"node_id":"MDU6TGFiZWw5MjkyNjc1Mzg=","url":"https://api.github.com/repos/elastic/elasticsearch/labels/team-discuss","name":"team-discuss","color":"fbca04","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":10,"created_at":"2018-05-02T02:09:59Z","updated_at":"2018-06-25T20:37:47Z","closed_at":null,"author_association":"NONE","pull_request":{"url":"https://api.github.com/repos/elastic/elasticsearch/pulls/30320","html_url":"https://github.com/elastic/elasticsearch/pull/30320","diff_url":"https://github.com/elastic/elasticsearch/pull/30320.diff","patch_url":"https://github.com/elastic/elasticsearch/pull/30320.patch"},"body":"This patch should not introduce any changes in the existing ES behavior. Its only goal is to allow subsequent addition of various hashing algorithms, such as quadkey, pluscode, hex, ....  The only user-visible change is the addition of the new `type` parameter to the `geohash_grid` aggregation. The only allowed value is `geohash` (default).\r\n\r\nThis PR is required for #30231 and #30240 \r\n\r\n### Open Questions\r\n  - [x] Builder parses each parameter independently.  `innerBuild()` seems to run after parameter parsing, but before the values are serialized by `innerWriteTo()`. Is it a good approach to do the cross-param validation and update the builder's state (set integer precision), so that serialization would only use int, not the parameter string?\r\n\r\n### TODO\r\n  - [x] Test for invalid precision: Test `testParseErrorOnPrecisionOutOfRange` and similar ones currently fail because the code no longer throws an error immediately when parsing `precision` input parameter. Instead, the error is thrown in  `GeoGridAggregationBuilder.innerBuild()` at a slightly later stage, once all parameters have been parsed, and the hashing type is known.\r\n  - ~Rename `geohash_grid` into `geo_grid`. Keep `geohash_grid` as a deprecated alias to `geo_grid`.~ (decided to postpone this for a follow up refactoring PR to reduce reviewing complexity)\r\n  - [ ] Modify `ParsedGeoHashGrid` to support the new type.  For some reason, other hashing algorithms worked fine without any modifications to that class and no unit tests have complained about this (bug?) cc: @cbuescher @javanna \r\nhttps://github.com/elastic/elasticsearch/blob/99f88f15c5febbca2d13b5b5fda27b844153bf1a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/ParsedGeoHashGrid.java#L31\r\n\r\nCC: @colings86 "},"comment":{"url":"https://api.github.com/repos/elastic/elasticsearch/issues/comments/400087327","html_url":"https://github.com/elastic/elasticsearch/pull/30320#issuecomment-400087327","issue_url":"https://api.github.com/repos/elastic/elasticsearch/issues/30320","id":400087327,"node_id":"MDEyOklzc3VlQ29tbWVudDQwMDA4NzMyNw==","user":{"login":"cbuescher","id":10398885,"node_id":"MDQ6VXNlcjEwMzk4ODg1","avatar_url":"https://avatars0.githubusercontent.com/u/10398885?v=4","gravatar_id":"","url":"https://api.github.com/users/cbuescher","html_url":"https://github.com/cbuescher","followers_url":"https://api.github.com/users/cbuescher/followers","following_url":"https://api.github.com/users/cbuescher/following{/other_user}","gists_url":"https://api.github.com/users/cbuescher/gists{/gist_id}","starred_url":"https://api.github.com/users/cbuescher/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/cbuescher/subscriptions","organizations_url":"https://api.github.com/users/cbuescher/orgs","repos_url":"https://api.github.com/users/cbuescher/repos","events_url":"https://api.github.com/users/cbuescher/events{/privacy}","received_events_url":"https://api.github.com/users/cbuescher/received_events","type":"User","site_admin":false},"created_at":"2018-06-25T20:37:47Z","updated_at":"2018-06-25T20:37:47Z","author_association":"MEMBER","body":"@nyurik after my discussion last week with @colings86 I took the time to check one of the options of how we can make this work with the high level rest client. We talked about using something similar to the RestSearchAction.TYPED_KEYS_PARAM flag that we already need for aggregations and suggestions to be parsable by the java rest client in general. I whipped something up along these lines in https://github.com/cbuescher/elasticsearch/commit/a11f0fac89d88ba3fdc5f5a4d7d89a48713d2331 but while doing this ran into several related problems. The commit is a quick and dirty hack to illustrate what I mean but there is a lot of whacky workarounds in there and it wouldn't work atm because some parts are missing. Some of the problems I encountered are:\r\n\r\n* the geoHashType really should be a property of the InternalGeoHashGrid/ParsedHashGrid aggregation. I left a comment about this in this PR. However, this doesn't seem to be a straight-forward change, since buckets currently cannot reference the aggregation they sit in. Not sure what the options would be about this\r\n* if we need to store the geoHash type in the buckets, this leads to several complications on the parsing side, e.g. we need to set the type in the buckets then but cannot be sure when we parsed back that information from the rest layer. I added a dirty hack to the commit above but I don't think its a good solution\r\n* the GeoHashType should have a stable id/name that we send across the rest layer. Using the enum name is too brittle IMHO (it can be the name, but it should be set at ctor time, not break when enums are renamed)\r\n* similarly, the GeoHashType needs a way to parse these ids back\r\n* finally: the GeoHashTypeProvider needs to convert back from String -> GeoPoint. I have a feeling thats doable but isn't implemented yet"}}
{"actor":{"display_login":"cbuescher"}}