-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Refactor geo_point validate* and normalize* for 2.x #12742
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
Conversation
|
I like this change but it does mean that the meaning of |
|
@colings86 The same applies. (e.g., see |
|
oh ok, great. Thanks for clarifying :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this bwc member. It is non-descript, and requires jumping to where it is set to figure out under what version this setting is no longer allowed. I think we should put the version check with the setting parsing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also just name it, as a local, so that the name implies what version is being checked for, but I would personally just put the version check directly here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually now that I see the rest of how this works, I think we should just be checking version here, and set the equivalent setting using ignoreMalformed/coerce. You can also add the version check into the conditional next to the name check, and let it fall through on newer indexes (this will cause an unknown setting exception). All of the old names can then be removed from the fieldtype/builder/mapper.
d8055d8 to
6df27a5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to make this needs to be change, it can't be beta1 anymore?
|
@nknize I left a couple more comments. Mappings look good, I think we should add some more tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the name a little confusing, since we already have this exact same name in FieldMapper, and it means before beta1. A better name would be good...I don't have a suggestion though...
|
This looks great. I left couple more comments about the tests to make sure we have real backcompat checks there. |
|
Thanks for the review @rjernst! Updated bwc testing and other minor edits from the comments. |
|
LGTM, 1 more minor suggestion for the tests. |
596592b to
5db4a93
Compare
5fb3b8d to
c490170
Compare
…ed and coerce* For consistency geo_point mapper's validate and normalize options are converted to ignore_malformed and coerced
c490170 to
b2ba384
Compare
This PR refactors the old
validateandnormalizeoptions forgeo_pointfield type toignore_malformedandcoerce, respectively. Geo queries are also updated toignore_malformedGeoPoints. This PR is for 2.0 only since mapping implementation has drastically changed from 1.x. For 1.7 changes see PR #12300closes #10170