Geo: Makes coordinate validator in libs/geo plugable #43657
Conversation
Moves coordinate validation from Geometry constructors into parser. Relates elastic#40908
imotov
added
:Analytics/Geo
|
Pinging @elastic/es-analytics-geo |
| * Validator that checks that lats are between -90 and +90 and lons are between -180 and +180 and altitude is present only if | ||
| * ignoreZValue is set to true | ||
| */ | ||
| public class GeographyValidator implements GeometryValidator { |
There was a problem hiding this comment.
I ran into this in #40869 Because we don't include lucene dependencies in our library plugins I had to copy much of Lucene's BitUtils and GeoHashUtils implementation. Here we're having to copy some of Lucene's GeoUtils functionality. Starting to get a little concerned w/ the code duplication and am curious as to whether or not it makes sense to start allowing lucene dependencies in our Libraries? Will this break the build configuration or just our library philosophy? /cc @rjernst @atorok
There was a problem hiding this comment.
If we want to reuse lucene pieces we would have to jump through a lot of hoops and probably move validation to server since we need libs/geo in SQL client and we really don't want SQL client to depend on Lucene. I don't think it is worth bringing lucene here because of 2 constants.
There was a problem hiding this comment.
Yeah. I'm not so concerned about the two constants here as I am about the growing duplication of code, like IOUtils, BitUtils, GeoHashUtils, and now GeoUtils. Perhaps the concern is premature, but I'm starting to see this become a bit of a trend. I just don't want to wake up one day and see that it got out of hand and future maintenance becomes a nightmare. It doesn't have to hold up this PR but I think it does warrant an async discussion.
There was a problem hiding this comment.
Worst case, in a distant future if it gets out of hand, we could have a compile time shading/copying of relevant lucene classes, if we want to include these in our lib without adding an external jar dep. I agree with Igor we should not add deps to SQL client.
There was a problem hiding this comment.
Would it be conceivable to have something like lucene-utils produced upstream on which we could depend on ? That could be a long term solution too, would be the same as @rjernst suggested, but would allow us to add a clean dependency.
There was a problem hiding this comment.
I don't think this would be likely, since from Lucene's perspective there is no reason for this extra dependency. Lucene doesn't want to be a general purpose utility library.
|
|
||
| public WellKnownText(boolean coerce, boolean ignoreZValue) { | ||
| public WellKnownText(boolean coerce, GeometryValidator validator) { |
There was a problem hiding this comment.
Should coerce also be part of the validator? Or are we trying to ensure the Validator treats incoming Geometry as "immutable"? It certainly makes the API cleaner, and XY geometry will coerce in a slightly different manner than LatLon geometry.
There was a problem hiding this comment.
Hmm... or maybe coerce is specific to the builders? And closeLinearRingIfCoerced is called there instead?
There was a problem hiding this comment.
Yeah here coerce is basically close linear ring if they are open. I am not really sure what to do with it to be honest. I kind of hope that we can deprecate that and remove it eventually. But I am ok with allowing validator to do coercing if needed but I am not totally sold that it belongs there to be honest.
There was a problem hiding this comment.
Sounds good. I don't think it needs to hold up this PR by any means. Let's see where the full refactoring goes and the solution may become more clear down the line.
|
@nknize I think we have a solution for dependency - we just move such pieces to the server project. Hopefully, we will be able to keep libs/geo dependency free. I don't think we will need to add more logic there, since libs/geo is targeting the client side. However, I agree, if we find ourselves wanting to add some logic there, we need to revisit this question. |
Moves coordinate validation from Geometry constructors into parser. Relates #43644
|
@imotov Since seems to be on 7.3 already, is there anything more to backport? |
|
@jpountz No. I just forgot to remove backport pending label. Thanks! |
Moves coordinate validation from Geometry constructors into
parser.
Relates #43644