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

Avoid panics in GEOS geometry construction #217

Merged
merged 8 commits into from
Nov 4, 2023

Conversation

lewiszlw
Copy link
Contributor

@lewiszlw lewiszlw commented Nov 2, 2023

No description provided.

Copy link
Member

@kylebarron kylebarron left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

It looks like it's still failing CI...

@@ -125,7 +125,7 @@ impl<'a, O: OffsetSizeTrait> GeometryArrayTrait<'a> for GeometryCollectionArray<
/// This operation is `O(1)` as it amounts to increase two ref counts.
/// # Examples
/// ```
/// use arrow2::array::PrimitiveArray;
/// use arrow::array::PrimitiveArray;
Copy link
Member

Choose a reason for hiding this comment

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

I just finished a migration from arrow2 to arrow-rs and the docstrings haven't been updated yet (not that they were accurate before either; they were copied from arrow2). Could you revert these specific changes to make it clear that these docstring examples don't work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

Copy link
Member

@kylebarron kylebarron Nov 3, 2023

Choose a reason for hiding this comment

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

Thanks, there's a whole bunch of these docstrings that need to be updated, so ideally we'll be intentional about fixing them in some sort of dedicated PR, ideally restoring doctest = true as well

geoarrow-rs/Cargo.toml

Lines 58 to 60 in 29930b0

[lib]
# TODO: fix docstrings
doctest = false

@@ -130,7 +130,7 @@ impl<'a, O: OffsetSizeTrait> GeometryArrayTrait<'a> for LineStringArray<O> {
"ARROW:extension:name".to_string(),
"geoarrow.linestring".to_string(),
);
Arc::new(Field::new("", self.storage_type(), true).with_metadata(field_metadata))
Arc::new(Field::new("geometry", self.storage_type(), true).with_metadata(field_metadata))
Copy link
Member

Choose a reason for hiding this comment

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

I'm reluctant to call this "geometry" because we don't know the actual name of the field referring to the data in this column (and e.g. there could be two geometry columns in one table). I might switch this to return the field metadata directly instead of a full FieldRef (ref #218)

@lewiszlw
Copy link
Contributor Author

lewiszlw commented Nov 3, 2023

Sorry I don't realize that i didn't enable geos feature to test changes locally...
Revert some geos changes as I can't test them locally.

P.S. I'm new to geo, so some changes might be stupid. I'm now working on adding geo support for datafusion, just find this great crate, thanks for your work.

@kylebarron kylebarron changed the title Some minor updates Avoid panics in GEOS geometry construction Nov 4, 2023
@kylebarron
Copy link
Member

Thanks!

@kylebarron kylebarron merged commit 9f5a2b3 into geoarrow:main Nov 4, 2023
3 checks passed
@kylebarron
Copy link
Member

Revert some geos changes as I can't test them locally.

depending on your OS, you might be able to install geos through a package manager

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