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-18] error reading shapefile #569

Merged
merged 4 commits into from
Dec 9, 2021
Merged

Conversation

madzik555
Copy link
Contributor

Is this PR related to a proposed Issue?

https://issues.apache.org/jira/browse/SEDONA-18

What changes were proposed in this PR?

In function getSplits() (core/src/main/java/org/apache/sedona/core/formatMapper/shapefileParser/shapes/ShapeInputFormat.java), the condition that splits file names by name only has been changed. Before the change, files with a different name and a different extension were treated as a separate data package, in which specific files could not be found.

How was this patch tested?

New java unit test.

Did this PR include necessary documentation updates?

No

@JassonHua
Copy link

JassonHua commented Dec 9, 2021 via email

@madzik555 madzik555 changed the title [Sedona 18] error reading shapefile [Sedona-18] error reading shapefile Dec 9, 2021
@jiayuasu jiayuasu changed the title [Sedona-18] error reading shapefile [SEDONA-18] error reading shapefile Dec 9, 2021
@jiayuasu jiayuasu added this to the sedona-1.2.0 milestone Dec 9, 2021
Copy link
Member

@jiayuasu jiayuasu 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 for your contribution! Can you use the existing shapefile (with an additional file inside that has a different file extension) in the resources and use the existing test cases to validate the new patch, instead of introducing a new shapefile?

There are two reasons behind this:

  1. A new shapefile introduces to additional copyright issue
  2. This new shapefile increases the source code size.

@madzik555
Copy link
Contributor Author

No problem. I can do this

@jiayuasu jiayuasu merged commit 890a8d9 into apache:master Dec 9, 2021
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

3 participants