Add && operator for BOX_2D
#601
Conversation
docs/functions.md
Outdated
| #### Signatures | ||
|
|
||
| ```sql | ||
| BOOLEAN && (l1 ANY[], l2 ANY[]) |
There was a problem hiding this comment.
I hope I can remove this, but I couldn't find the way...
There was a problem hiding this comment.
Yeah I can't think of an easy way to avoid this right now, but I think it's fine.
There was a problem hiding this comment.
Since I first made this attempt at function auto doc generation, duckdb has added a FunctionDescription with a vector<string> categories field thats separate for each overload, so in the future we could use this to identify spatial-added functions instead of relying on the tags. But that will be part of a much larger change for the future.
There was a problem hiding this comment.
Oh, good to know. Thanks!
| const auto box_min_x_data = FlatVector::GetData<double>(*bbox_vec[0]); | ||
| const auto box_min_y_data = FlatVector::GetData<double>(*bbox_vec[1]); | ||
| const auto box_max_x_data = FlatVector::GetData<double>(*bbox_vec[2]); | ||
| const auto box_max_y_data = FlatVector::GetData<double>(*bbox_vec[3]); |
There was a problem hiding this comment.
STRUCT's are not guaranteed to be flat, so this isn't safe to do unless you call .Flatten() on the input args chunk (at which point the geom vector is also flat, so you don't need unified format). Or you can setup unified formats for both the struct vector and each component vector too - you need to check the validity of the struct and each component.
| variant.AddParameter("point1", GeoTypes::GEOMETRY()); | ||
| variant.AddParameter("point2", GeoTypes::GEOMETRY()); | ||
| variant.SetReturnType(GeoTypes::BOX_2D()); |
There was a problem hiding this comment.
I think having a overload for POINT_2D in the future (not this PR) would be great too!
There was a problem hiding this comment.
Sorry, I forgot to reply here. It seems it already accepts POINT_2D. Do you mean the overload is still useful to avoid the type conversion from POINT_2D to GEOMETRY? (I don't know how this works, but I guess this coercion happens under the hood)
D SELECT ST_MakeBox2D(ST_POINT2D(0, 0), ST_POINT2D(1, 1));
┌──────────────────────────────────────────────────┐
│ st_makebox2d(st_point2d(0, 0), st_point2d(1, 1)) │
│ box_2d │
├──────────────────────────────────────────────────┤
│ BOX(0 0, 1 1) │
└──────────────────────────────────────────────────┘
|
|
||
| // Try to get the cached bounding box from the blob | ||
| Box2D<float> geom_bbox; | ||
| if (geom.TryGetCachedBounds(geom_bbox)) { |
There was a problem hiding this comment.
Im trying to avoid using TryGetCachedBounds because it returns float boxes, not double, which might lead to imprecise results when comparing against doubles. But I think for now this is fine, if I remember right && in PostGIS is documented as being "approximate".
There was a problem hiding this comment.
if I remember right
&&in PostGIS is documented as being "approximate".
Yeah, I thought TryGetCachedBounds is fine for this reason. But, honestly, I'm not sure if the approximation is nice. So, if you want to change this, I'm fine to follow.
There was a problem hiding this comment.
Let me correct a bit. The document doesn't say "approximate", but it surely uses float instead of double, so comparing by float precision is the right choice if we follow PostGIS's behavior.
| -- function-specific tweaks | ||
| AND CASE function_name | ||
| -- TODO: https://github.com/duckdb/duckdb-spatial/pull/601#discussion_r2144753435 | ||
| WHEN '&&' THEN 'box' IN parameters | ||
| ELSE true | ||
| END |
There was a problem hiding this comment.
This isn't nice, but I think &&'s document should really note that the operation is not very accurate.
|
Okay, I think I addressed the comments! |
This pull request is my attempt to address #4, although I'm yet to figure out the proper scope of the issue. Also, I'm not sure how this is useful in terms of the performance. So, I'd appreciate feedbacks.
This pull request adds two functions:
ST_MakeBox2D()&&forBOX_2DandGEOMETRY