kgyrtkirk commented on code in PR #16241: URL: https://github.com/apache/druid/pull/16241#discussion_r1604609689
########## processing/src/main/java/org/apache/druid/collections/spatial/search/Bound.java: ########## @@ -43,6 +45,8 @@ public interface Bound<TCoordinateArray, TPoint extends ImmutableNode<TCoordinat boolean contains(TCoordinateArray coords); + boolean containsObj(@Nullable Object input); Review Comment: I wonder if changing the original `contains` method to have a `TCoordinateArray` could help? if that's not enough I still don't think that supporting different types like `float[],int[],double[]` should flow down entirely to the shapes themselves....instead of `Object` it could be `GenericPoint` or something like that - which could then handle comparision differently... it seems to me that this class and its methods are a little bit skewed ; adding `containsObj` here - and implementing it only in `RectangularBound` makes it also avaialable for `Polygon` - which is wrong... a way to fix this could be to change the method's name to reflect its intention - do you want to extend the system to enable a quick `AABB` based check - in that case you could name this method as `isInsideAABB` or something which reflects its desired use - but then there must be a followup check which will be done to ensure the real contains relation. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
