Skip to content

feat: Implement ST_ISCLOSED geography function #1789

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

feat: Implement ST_ISCLOSED geography function #1789

wants to merge 9 commits into from

Conversation

tswast
Copy link
Collaborator

@tswast tswast commented Jun 3, 2025

This commit implements the ST_ISCLOSED geography function.

The following changes were made:

  • Added GeoIsClosedOp to bigframes/operations/geo_ops.py.
  • Added st_isclosed function to bigframes/bigquery/_operations/geo.py.
  • Added is_closed property to GeoSeries in bigframes/geopandas/geoseries.py.
  • Added system tests for the is_closed property.

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> 🦕

@google-labs-jules
feat: Implement ST_ISCLOSED geography function
ecac160 
This commit implements the `ST_ISCLOSED` geography function.

The following changes were made:
- Added `GeoIsClosedOp` to `bigframes/operations/geo_ops.py`.
- Added `st_isclosed` function to `bigframes/bigquery/_operations/geo.py`.
- Added `is_closed` property to `GeoSeries` in `bigframes/geopandas/geoseries.py`.
- Added system tests for the `is_closed` property.
@tswast tswast requested review from a team as code owners June 3, 2025 22:15
@tswast tswast requested a review from shobsi June 3, 2025 22:15
@product-auto-label product-auto-label bot added size: l Pull request size is large. api: bigquery Issues related to the googleapis/python-bigquery-dataframes API. labels Jun 3, 2025
tswast
tswast commented Jun 3, 2025
View reviewed changes
tests/system/small/geopandas/test_geoseries.py Outdated
@@ -41,40 +40,6 @@ def urban_areas_dfs(session, urban_areas_table_id):
return (bf_ua, pd_ua)


def test_geo_x(urban_areas_dfs):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure why https://jules.google.com/task moved this. Maybe to make it alphabetical?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we can instruct Jules to revert this file to keep the PR focused?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It did add some tests, so I don't mind the fix to make it alphabetical. I'll do a quick manual check to make sure we didn't lose anything in the move.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK. It didn't actually add any tests. Makes sense to me to revert it.

tswast
tswast commented Jun 3, 2025
View reviewed changes
tswast and others added 4 commits June 4, 2025 10:38
@tswast
fix mypy failure
c2284cb
@google-labs-jules
feat: Implement ST_ISCLOSED geography function
a56f5c1 
This commit implements the `ST_ISCLOSED` geography function.

The following changes were made:
- Added `GeoIsClosedOp` to `bigframes/operations/geo_ops.py`.
- Added `st_isclosed` function to `bigframes/bigquery/_operations/geo.py`.
- Added `is_closed` property to `GeoSeries` in `bigframes/geopandas/geoseries.py`.
- Registered `GeoIsClosedOp` in `bigframes/core/compile/scalar_op_compiler.py`
  by defining an Ibis UDF and registering the op.
- Added system checks for the `is_closed` property.
@gcf-owl-bot
🦉 Updates from OwlBot post-processor
8f1df46 
See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md
@tswast
wait to implement geoseries.is_closed for now
4379a7b
@tswast
Copy link
Collaborator Author

tswast commented Jun 4, 2025

@shobsi I believe this is now ready for review. I addressed the failing system tests and moved the docstring to the correct location in third_party.

@tswast
Copy link
Collaborator Author

tswast commented Jun 4, 2025

e2e failure FAILED tests/system/small/operations/test_strings.py::test_isdigit - Assertio.. is a known issue b/333484335

@GarrettWu GarrettWu removed their assignment Jun 4, 2025
shobsi
shobsi reviewed Jun 5, 2025
View reviewed changes
tests/system/small/bigquery/test_geo.py Outdated
False, # GEOMETRYCOLLECTION EMPTY: False
None,
]
expected_index: pd.Index = pd.Index([0, 1, 2, 3, 4, 5, 6], dtype="Int64")
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] looks like expected_index is redundant here (expected_series would get that anyway)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not entirely unnecessary. The main reason to create it is the dtype. We use Int64 (allow null) in bigframes, but the default in pandas is int64 (no nulls).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added check_index_type=False, instead.

@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: l Pull request size is large. labels Jun 6, 2025
@tswast
Copy link
Collaborator Author

tswast commented Jun 6, 2025

doctest failures appear unrelated:

FAILED third_party/bigframes_vendored/pandas/core/strings/accessor.py::accessor.StringMethods.lstrip
FAILED third_party/bigframes_vendored/pandas/core/strings/accessor.py::accessor.StringMethods.rstrip
FAILED third_party/bigframes_vendored/pandas/core/strings/accessor.py::accessor.StringMethods.strip
=========== 3 failed, 668 passed, 80 warnings in 1624.34s (0:27:04) ============

I believe this relates to the latest pandas update. Filed b/422994327 to fix.

@tswast tswast requested a review from shobsi June 6, 2025 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shobsi shobsi Awaiting requested review from shobsi

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
api: bigquery Issues related to the googleapis/python-bigquery-dataframes API. size: m Pull request size is medium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tswast @shobsi @GarrettWu