[SEDONA-664] Add native GeoPackage reader #1603

Imbruced · 2024-09-24T16:51:21Z

Did you read the Contributor Guide?

Yes, I have read the Contributor Rules and Contributor Development Guide

Is this PR related to a JIRA ticket?

No

What changes were proposed in this PR?

Geopackage datasource

How was this patch tested?

integration tests

Did this PR include necessary documentation updates?

I ll adjust the docs

Imbruced · 2024-09-24T16:54:32Z

As a follow up to this mr, I need to add

ewkb parsing (I think reading from circular geometries wont work)
support for parallel loading via limit and offset
add kindoff filtering based on envelope offset
direct transformation from geopackage serialization to sedona

Kontinuation · 2024-09-25T02:45:04Z

Thank you for this great work! My major concern is whether it works with GeoPackage files stored on cloud storage such as HDFS or S3. As far as I know org.xerial:sqlite-jdbc only works with local files, so we may have to download the GeoPackage file from cloud storage to the local file system before reading it.

Imbruced · 2024-09-25T07:23:58Z

@Kontinuation good point, I ll write the test to make sure it works. Integration test with minio would be enough I think.

Imbruced · 2024-09-29T21:40:53Z

WIP

Kontinuation · 2024-10-02T06:45:43Z

...n/src/main/scala/org/apache/sedona/sql/datasources/geopackage/transform/GeometryReader.scala

+    // skip srid for now
+    reader.getInt()
+
+    skipEnvelope(resolvedFlags._1, reader)
+
+    val wkb = new Array[Byte](reader.remaining())
+    reader.get(wkb)
+
+    val wkbReader = new org.locationtech.jts.io.WKBReader()
+    val geom = wkbReader.read(wkb)


I suggest that we take SRID into consideration. val wkbReader = new WKBReader(new GeometryFactory(new PrecisionModel(), srid)) would be sufficient.

Kontinuation · 2024-10-02T06:52:44Z

...k-3.5/src/main/scala/org/apache/sedona/sql/datasources/geopackage/GeoPackageDataSource.scala

+      if (pathString.toLowerCase(Locale.ROOT).endsWith(".geopackage")) {
+        val path = new Path(pathString)
+        val fs = path.getFileSystem(hadoopConf)
+
+        val isDirectory = Try(fs.getFileStatus(path).isDirectory).getOrElse(false)
+        if (isDirectory) {
+          pathString
+        } else {
+          pathString.substring(0, pathString.length - 3) + "???"
+        }


If I understand it correctly, if pathString ends with ".geopackage", and it is not a directory, it will be transformed to "****.geopack???". I cannot grasp the idea of this transformation.

yeah you are right, I was sure that in this part I already have list of files in the directory, I am wondering how it would behave if I am passing list of files and if its actually needed (user specifying paths with different file formats).

Kontinuation · 2024-10-02T06:57:26Z

.../spark-3.5/src/main/scala/org/apache/sedona/sql/datasources/geopackage/GeoPackageTable.scala

+    val serializableConf = new SerializableConfiguration(
+      sparkSession.sessionState.newHadoopConfWithOptions(options.asScala.toMap))
+
+    val tempFile = FileSystemUtils.copyToLocal(serializableConf.value, files.head.getPath)


Can we detect if the path is a local path skip calling copyToLocal?

Imbruced · 2024-10-02T21:27:52Z

@Kontinuation thanks for the review !

jiayuasu · 2024-10-07T13:21:45Z

spark/spark-3.3/pom.xml

@@ -98,6 +97,36 @@
            <groupId>org.locationtech.jts</groupId>
            <artifactId>jts-core</artifactId>
        </dependency>
+        <dependency>
+            <groupId>org.testcontainers</groupId>


Is there a reason why these dependencies only appear in the spark-3.3 profile?

test testing loading from s3 (I used minio with test containers) is only in spark 3.3. I can duplicate it to other versions of spark as well

Please move these to the spark-common pom.xml so all Spark 3.X pom.xml will share it

I think deps with test scope is not propagating when you put it in common and I would avoid putting it as normal deps, I copied them for each of the spark version.

jiayuasu · 2024-10-07T13:22:29Z

spark/spark-3.3/pom.xml

Do we support other Spark versions 3.0, 3.1, and 3.2?

I didn't test it with those I can add data sources

Yes, if the implementation differs among the versions, we should definitely replicate the tests and test them for each version

atiannicelli · 2024-10-11T17:44:35Z

@jiayuasu I can't wait to be able to use this for Overture Buildings Conflation. We are looking at using a couple datasets that are packaged as gpkg files.

jiayuasu · 2024-10-14T16:59:47Z

spark/spark-3.1/src/test/scala/org/apache/sedona/sql/GeoPackageReaderTest.scala

+        .option("showMetadata", "true")
+        .load(path)
+
+      df.where("data_type = 'tiles'").show(false)


Please remove or comment out all show() functions

I think we might add linter for that cases, sorry for that.

docs/tutorial/sql.md

Imbruced · 2024-10-14T18:52:30Z

@jiayuasu remove show method calls

spark/spark-3.0/src/test/scala/org/apache/sedona/sql/GeoPackageReaderTest.scala

spark/spark-3.1/src/test/scala/org/apache/sedona/sql/GeoPackageReaderTest.scala

spark/spark-3.2/src/test/scala/org/apache/sedona/sql/GeoPackageReaderTest.scala

spark/spark-3.3/src/test/scala/org/apache/sedona/sql/GeoPackageReaderTest.scala

spark/spark-3.4/src/test/scala/org/apache/sedona/sql/GeoPackageReaderTest.scala

spark/spark-3.5/src/test/scala/org/apache/sedona/sql/GeoPackageReaderTest.scala

spark/spark-3.0/src/test/scala/org/apache/sedona/sql/GeoPackageReaderTest.scala

Imbruced · 2024-10-14T20:48:11Z

@jiayuasu I applied changes, and what I saw is that we are running previous pipelines even if new are starting I think it's worth adding concurrency mechanism in github actions
https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#concurrency

spark/common/pom.xml

github-actions bot added the sedona-spark label Sep 24, 2024

github-actions bot added the docs label Sep 30, 2024

Imbruced marked this pull request as ready for review October 1, 2024 21:15

Imbruced requested a review from jiayuasu as a code owner October 1, 2024 21:15

Imbruced requested a review from Kontinuation October 1, 2024 21:51

Kontinuation reviewed Oct 2, 2024

View reviewed changes

Imbruced force-pushed the feature/geopackage-reader branch from 767b8ca to 5b88709 Compare October 6, 2024 16:56

jiayuasu reviewed Oct 7, 2024

View reviewed changes

Imbruced force-pushed the feature/geopackage-reader branch 4 times, most recently from 41d4965 to 18186b9 Compare October 12, 2024 22:39

jiayuasu changed the title ~~Feature/geopackage reader~~ [SEDONA-664] Add native GeoPackage reader Oct 14, 2024

jiayuasu requested changes Oct 14, 2024

View reviewed changes

jiayuasu added this to the sedona-1.7.0 milestone Oct 14, 2024

jiayuasu requested a review from Kontinuation October 14, 2024 17:00

jbampton reviewed Oct 14, 2024

View reviewed changes

docs/tutorial/sql.md Outdated Show resolved Hide resolved

Imbruced force-pushed the feature/geopackage-reader branch from fc24a6e to c73aab3 Compare October 14, 2024 18:51

jbampton reviewed Oct 14, 2024

View reviewed changes

spark/spark-3.0/src/test/scala/org/apache/sedona/sql/GeoPackageReaderTest.scala Outdated Show resolved Hide resolved

Add geopackage data source.

4468d67

Imbruced force-pushed the feature/geopackage-reader branch from 5964204 to 4468d67 Compare October 14, 2024 20:42

jiayuasu reviewed Oct 14, 2024

View reviewed changes

spark/common/pom.xml Show resolved Hide resolved

jiayuasu approved these changes Oct 15, 2024

View reviewed changes

jiayuasu merged commit 4c7da62 into master Oct 15, 2024
50 checks passed

jiayuasu deleted the feature/geopackage-reader branch October 15, 2024 18:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SEDONA-664] Add native GeoPackage reader #1603

[SEDONA-664] Add native GeoPackage reader #1603

Imbruced commented Sep 24, 2024 •

edited

Loading

Imbruced commented Sep 24, 2024

Kontinuation commented Sep 25, 2024 •

edited

Loading

Imbruced commented Sep 25, 2024

Imbruced commented Sep 29, 2024

Kontinuation Oct 2, 2024

Kontinuation Oct 2, 2024

Imbruced Oct 2, 2024

Kontinuation Oct 2, 2024

Imbruced Oct 2, 2024

Imbruced commented Oct 2, 2024

jiayuasu Oct 7, 2024

Imbruced Oct 7, 2024

jiayuasu Oct 8, 2024

Imbruced Oct 11, 2024

jiayuasu Oct 7, 2024

Imbruced Oct 7, 2024

jiayuasu Oct 8, 2024

atiannicelli commented Oct 11, 2024

jiayuasu Oct 14, 2024

Imbruced Oct 14, 2024

Imbruced commented Oct 14, 2024

Imbruced commented Oct 14, 2024

[SEDONA-664] Add native GeoPackage reader #1603

[SEDONA-664] Add native GeoPackage reader #1603

Conversation

Imbruced commented Sep 24, 2024 • edited Loading

Did you read the Contributor Guide?

Is this PR related to a JIRA ticket?

What changes were proposed in this PR?

How was this patch tested?

Did this PR include necessary documentation updates?

Imbruced commented Sep 24, 2024

Kontinuation commented Sep 25, 2024 • edited Loading

Imbruced commented Sep 25, 2024

Imbruced commented Sep 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Imbruced commented Oct 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atiannicelli commented Oct 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Imbruced commented Oct 14, 2024

Imbruced commented Oct 14, 2024

Imbruced commented Sep 24, 2024 •

edited

Loading

Kontinuation commented Sep 25, 2024 •

edited

Loading