[GEO] Add WKT Support to GeoBoundingBoxQueryBuilder

[nknize]

#9120 added WKT support for BBOX (...
This PR adds WKT parsing support to GeoBoundingBoxQueryBuilder

closes #27690

[nknize]

@cbuescher This is the separate PR we talked about creating for added BBOX support to GeoBoundingBoxQueryBuilder This needs to be done before opening the PR for the new geo_bounding_box field. I incorporated the feedback you gave when I first added it to #27417

[cbuescher]

@nknize great, I left a few comments but mostly minor stuff. I'm in favour of the inner "field" parameter, but can you double check that we didn't document it at some point? Maybe we should keep it at least in 6.2 (with deprecation) and remove in 7.0 just to be save, not sure how much this complicates things though.

[cbuescher]

I'm not sure we need to support WKT in toXContent if only for testing. The idea so far was to have one "cannonical" form the builder represents itself and the information is the same. Just my 2cents here atm.

[nknize]

OK, that keeps things simple. Then if users really want the original WKT representation they can always add a stored field, multifield, or pull from source.

[cbuescher]

Yes, however I might be overlooking something but I think that should be enough. We are not talking about documents here now, where the wkt representation should be available from "_source", no? A query builders xContent representation shouldn't normally be something you store, but I might be wrong. But I would revisit this only if the need arises.

[cbuescher]

Can the delta be lower here? Just wondering if there is a reason for the relatively high error margin, this is only testing parsing, or do we already loose that much precision when storing the coords in the builder?

[cbuescher]

+1 for dropping this variant in 7.0, but was this already deprecated at some point? I didn't see it being documented anywhere but we might still need to officially deprecate it in 6.x

[nknize]

I don't think this has been deprecated in 6.x yet. I'll do that when I merge to 6.x in lieu of removal

[cbuescher]

Can you change to the regular cast operator unless there is a specific reason for the Class.cast here? I think its more obvious whats happening and doesn't hide potential warnings etc... at compile time.

[cbuescher]

sparse doesn't seem to get used before, why not just create a new point here?

[cbuescher]

I think it would be good to have a small test for this kind of type check.

[nknize]

👍

[nknize]

Sorry for the long delay here @cbuescher. Holidays and end of year funzies. I've gone ahead and fixed up the PR per last round of feedback and its ready for another review. I think this should be close.

[cbuescher]

@nknize thanks for updating, I left two minor nits but the rest looks good to me, feel free to adress or drop those comments, no need for another review if nothing else changes I think.

[cbuescher]

nit: maybe use expectThrows() like we do in many other places

[cbuescher]

nit: maybe you can use GeoShapeType.GEOMETRYCOLLECTION here instead of null and then check for that enum in the method where the exception might be thrown?