[GEO] Prevent PrefixTree strategies on new geo_shape indexes

[nknize]

6.6 introduced BKD based indexing for geo_shape field types. For bwc purposes PrefixTree strategies and parameters were deprecated but still allowed on newly created indexes. This PR prohibits using any/all PrefixTrees strategies and parameters for 7.0+ newly created geo_shape indexes.

TODO

• fix remaining failing tests
• add explicit tests for exception handling when creating a geo_shape index using PrefixTree parameters
• update docs to remove all prefix tree parameters (and references to)
• update breaking changes doc

[elasticmachine]

Pinging @elastic/es-analytics-geo

[elasticmachine]

Pinging @elastic/es-search

[jpountz]

In the past we tried to be a bit lenient with changes in mappings due to the fact that mappings may be used in templates that are a bit hard to migrate. Maybe we should issue a deprecation warning and ignore deprecated settings instead of rejecting them. It would make all existing templates keep working and also still give us the ability to remove support for legacy shapes in 8.0 since no 7.x index is going to use them?

[jpountz]

s/prohibit/Prohibit/?

[jpountz]

let's maybe keep it in the docs since the plan is to have it back by 7.0?

[imotov]

I am somewhat concerned about merging this before we achieve parity in BKD implementation, unless me make this as a blocker for 7.0. Otherwise we don't really provide a clear migration path for users who are using the currently missing functionality.

[polyfractal]

I agree, this feels like a fairly big regression.

If we throw an exception, all existing templates using non-default parameters will break. If we just log a deprecation, the user is silently opted into functionality that may break their existing queries. Which is arguably a worse situation since they won't know until a CONTAINS/MULTIPOINT/LINESTRING + WITHIN query is used and then it's a runtime break far away from the mapping change.

In both cases, anyone depending on the missing functionality will be prevented from upgrading until whatever 7.x version implements it. :(

This is a bit tangential to this PR, but defaulting to the new geoshapes in 6.6 feels like a big break too? E.g. existing templates (that only use default geoshape params) will opt into the new geoshape and could break queries due to reduced functionality? Is there something I'm missing there?

[nknize]

I am somewhat concerned about merging this before we achieve parity in BKD implementation, unless me make this as a blocker for 7.0.

I agree. I wanted to get this opened as a WIP PR to solicit feedback on the migration path forward. Some great feedback here and I do think we should hold on this until:

1. https://issues.apache.org/jira/browse/LUCENE-8620 from @iverase is added to Lucene (hoping 8.0). That will give feature parity for CONTAINS queries.
2. Adaptive orientation is integrated w/ Lucene to ensure accurate point in triangle calculation; which is needed for MultiPoint query parity (Add support for MultiPoint Queries to new BKD geo_shape indexing approach #37318).

I don't think we need to wait for linestring + within queries. This is more of an "accidental feature" that really shouldn't be available as its somewhat misleading (its not actually performing a true buffer query based on distance, only exploiting the resolution of the quadcells).

defaulting to the new geoshapes in 6.6 feels like a big break too?

You're not missing anything w.r.t templates. Its something that was discussed and added to the 6.6 breaking changes doc.

[jpountz]

Adaptive orientation is integrated w/ Lucene to ensure accurate point in triangle calculation; which is needed for MultiPoint query parity

@nknize Can you clarify why this is needed?

[nknize]

@nknize Can you clarify why this is needed?

After digging into this for MultiPoint queries I'll retract my statement and say its not needed. So long as we stay in encoded space overflow a non issue (but will likely need to be readdressed for supporting different projections using XYShape). I'm wrapping up testing on the MultiPoint query feature branch and will open a lucene patch shortly. That should help close the gap for feature parity on MultiPoint and CONTAINS.

[blkbltjns]

I've hit a couple roadblocks trying to upgrade from 6.3 to 7.5 having to do with non feature parity and performance regressions of the new BKD indexing.

The largest roadblock so far has been the inability to spatially intersect with the circle type. Converting the circles to a polygons first is not an option due to my use case involving thousands of circles which kills performance if converted to polygons and intersected from there. I would not need to intersect with the circle type if there was supported capability to filter by distance to an arbitrary geo_shape (point, line, polygon, etc...).

The next largest roadblock is a significant performance regression when intersecting an index containing a few million polygons (land parcels shapes which will often overlap).

If the old indexing method is not carried forward, I will not be able to upgrade to 8.0 without some significant rework in my application.

[jimczi]

The PR is outdated, hence closing.