Skip to content
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

[SEDONA-508] Add ST_CrossesDateLine #1259

Merged
merged 5 commits into from
Feb 29, 2024

Conversation

prantogg
Copy link
Contributor

Did you read the Contributor Guide?

Is this PR related to a JIRA ticket?

What changes were proposed in this PR?

Introduces a predicate function, ST_CrossesDateLine to determines if a given geometry crosses the International Date Line.

  • Checks if the difference in longitude between any pair of consecutive points in the geometry exceeds 180 degrees. If so, returns true, else false.
  • For multi-geometries (e.g., MultiPolygon, MultiLineString), this function will return true if any one of the geometries within the multi-geometry crosses the International Date Line.

How was this patch tested?

  • Passes new and existing tests

Did this PR include necessary documentation updates?

@@ -13,9 +13,11 @@
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please put this one in Functions. Predicates are for functions that take two geometries as input and return true/false.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@prantogg Can you address this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

return false;
}

AtomicBoolean crossesDateLine = new AtomicBoolean(false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why use AtomicBoolean? I believe the filter functions will be called in a sequential order?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CoordinateSequenceFilter being an anonymous inner class could not modify a local variable outside its filter scope unless its final. AtomicBoolean was a work around because final variable's cannot be modified. I have removed separate handling of GeometryCollection so I can define a private local variable crossesDateLine within the filter.

}
};

if (geometry instanceof GeometryCollection) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why explicitly do this for GeometryCollection? Note that: GeometryCollection can have nested GeometryCollection inside. We should avoid implementing our own logic to iterate GeometryCollection. If we really want to do it, this should be done via a recursive function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially wanted to make sure Every geometry within the GeometryCollection is handled separately because crossesDateLine should return true if any geometry is crossing the dateline.
You're right, explicit handling for GeometryCollection was not required, CoordinateSequenceFilter is able to handle this well. Have pushed the changes.

}


// public static boolean crossesDateLine(Geometry geometry) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the comments.

@jiayuasu jiayuasu linked an issue Feb 29, 2024 that may be closed by this pull request
@jiayuasu jiayuasu removed a link to an issue Feb 29, 2024
@jiayuasu jiayuasu merged commit 7d293df into apache:master Feb 29, 2024
49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants