-
Notifications
You must be signed in to change notification settings - Fork 30
feat(sedona-functions): Implement native ST_NumGeometries using WKBHeader #270
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| let first_xy = header.first_xy(); | ||
| if first_xy.0.is_nan() && first_xy.1.is_nan() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look! This came in handy 🤠 (I didn't have a great idea of what you wanted first_xy for, but this was neat)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was also thinking about pulling out this logic into a new is_empty function, since there are other places we'd want this (e.g ST_IsEmpty). I was thinking about something like one of these in sedona-geometry
A new method in WkbHeader:
// WkbHeader
pub fn is_empty(&self) -> Result<bool>, SedonaGeometryError> {OR a new function in sedona_geometry::is_empty.rs that takes a WkbHeader
// proposed function
pub fn is_geometry_empty_header(
header: WkbHeader,
) -> Result<bool, SedonaGeometryError> {
// The Existing function
pub fn is_geometry_empty<G: GeometryTrait<T = f64>>(
geometry: &G,
) -> Result<bool, SedonaGeometryError> {The former is cleaner imo, but I wasn't sure if you'd agree that it makes sense for WkbHeader to have that method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense for the WkbHeader to have an is_empty() method!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Will be slightly cleaner with the is_empty() bit you suggested (feel free to do that here or some other time!)
Co-authored-by: Dewey Dunnington <[email protected]>

Part of #198