Add Z value support to geo_point and geo_shape

[nknize]

This enhancement adds Z value support (source only) to geo_shape fields. If vertices are provided with a third dimension, the third dimension is ignored for indexing but returned as part of source. Like before, any values greater than the 3rd dimension are ignored.

closes #22917

[jpountz]

you need to add a version check here?

[jpountz]

you need to add a version check here?

[jpountz]

we should test parsing too?

[nknize]

added testParse3DPolygon to GeoJSONShapeParserTests

[hanoch]

@nknize #22917 talks about both geo_point and geo_shape. does this PR include support for store-and-retrieve Z values for geo_point as well, or will that support be separate from this PR?

[nknize]

@hanoch geo_point already supports Z values in source.

This PR effectively fixes a bug where Z values for geo_shape coordinates were omitted in source after indexing.

[javanna]

@nknize are you still working on this?

[hanoch]

@nknize we are still waiting for this functionality to be merged into the master branch.

[hanoch]

cc: @sherry-ger

[nknize]

@colings86 This should be good to go. Could you give it a final once over since @jpountz is out? Thx

[clintongormley]

Discussed this in FixItFriday. We're not crazy about accepting parameters that we will silently ignore, but given that Z values are part of the geojson spec, we should probably make an effort to extract what info we can from valid geojson files.

It should, however, be enabled by opting in using a mapping parameter like accept_but_ignore_z_values, which makes clear to the user what they are opting in to.

[colings86]

@nknize this looks ok to me but could you add the option that @clintongormley mentioned and a tests with 3 dimensional polygons with and without that setting?

[hanoch]

are we finally ready to merge this to master anytime soon?

[hanoch]

@colings86 @jpountz @nknize is this now ready to be merged to master to be able to address #22917?

[nknize]

Rebased to master and cleaned up some geo_point parsing. This is ready for another review and should be just about ready to merge. /cc @jpountz @colings86

[colings86]

LGTM, its a shame the changes are so pervasive in the code and the acceptZValue option has to be passed around so much but unfortunately I don't think there is a better way to do it.

[jpountz]

Thanks for making the docs very explicit about how the 3rd dimension is handled. I left some comments but it looks like it is on the right track to me.

[jpountz]

beware it might not do what you think: https://stackoverflow.com/questions/8819738/why-does-double-nan-double-nan-return-false

[nknize]

ah, yes... thank you. I still slip up w/ this one occasionally.

[jpountz]

Can you add the expected number of dims and the new coordinate to the error message?

[nknize]

👍

[jpountz]

we don't seem to be making decisions based on this option? or do I miss it?

[jpountz]

should it be called something like ignore_z_value instead to make it clear it's ignored?

[nknize]

👍

[jpountz]

reading the rest of the PR, it doesn't seem like we really need this method? We could just check whether the string contains a comma to know whether it is a geohash and then use the length of the array after splitting to know the number of dimensions?

[nknize]

👍

[jpountz]

no need for this alt variable

[jpountz]

this needs to be version 6.3 now

[jpountz]

should be 6.3

[jpountz]

remove todo

[jpountz]

true by default? shouldn't it be false like the docs say? (we might need some logic to make it true for pre-6.3 indices in case we used to accept it in the past)

[jpountz]

Logic looks good to me overall. I think we should make the default of ignore_z_value false as it could otherwise be a bit trappy? For shapes we can do it directly in this PR since they never supported 3 dimensions (if my understanding is correct). And for points which were always lenient until now (again, if my understanding is correct), we would make the default true in this PR (like you did) but do a follow-up PR to change the default to false on 7.0+ indices

[jpountz]

please fail if vals.length > 3

[hanoch]

@jpountz @nknize Wouldn't it be better to be more lenient and accept the first 3 (or 2) dimensions, rather than completely fail? Adding failure now might break compatibility with previous behavior.

[jpountz]

s/ACCEPT/IGNORE/

[jpountz]

let's remove this leniency?

[hanoch]

@jpountz @nknize what is the overall design goal wrt the default behavior with Z values with geo_shape and geo_point?
It would be best if the default setting and behavior will continue to be that if the data value (csv-string/array/GeoJson/etc.) has a third Z dimension - it will be stored without needing to set any special settings. Also, it will be best to continue to auto ignore any 4th (e.g. M value) and above dimension in the data by default (without adjusting setting to accept the data) and store the first 3 (or 2 if the Z value is not present) dimensions, without failing if there is a 4th or more dimension.
That behavior will be consistent with the existing geo_point [x,y,z] or [x,y] array data values (and we should match the csv-string values with a Z value "y,x,z" or "y,x" as well) of existing and previous releases, where if the data has a Z value it will be stored, without needing to set some special settings, or modify the default settings.
Is that the design?

[colings86]

@hanoch we do not want the Z Values to be accepted by default because it can be surprising to users. The fact that the z value is accepted but is not indexed and therefore does not affect search results is not what a user would expect OOTB so we feel it is a better user experience for the user to opt-in to accepting but ignoring the Z-Value.

[hanoch]

is there anything else pending or are we ready to merge this PR?