Skip to content

Conversation

@petern48
Copy link
Collaborator

@petern48 petern48 commented Nov 1, 2025

Part of #198

@petern48
Copy link
Collaborator Author

petern48 commented Nov 1, 2025

image

Comment on lines 101 to 102
let first_xy = header.first_xy();
if first_xy.0.is_nan() && first_xy.1.is_nan() {
Copy link
Collaborator Author

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)

Copy link
Collaborator Author

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.

Copy link
Member

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!

@petern48 petern48 marked this pull request as ready for review November 1, 2025 17:01
@petern48 petern48 requested a review from paleolimbot November 1, 2025 17:01
Copy link
Member

@paleolimbot paleolimbot left a 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!)

@petern48 petern48 requested a review from paleolimbot November 2, 2025 05:10
@petern48 petern48 merged commit 1954402 into apache:main Nov 3, 2025
12 checks passed
@petern48 petern48 deleted the st_numgeometries branch November 3, 2025 04:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants