You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sedona.apache.org by GitBox <gi...@apache.org> on 2021/05/13 12:25:26 UTC
[GitHub] [incubator-sedona] yitao-li opened a new pull request #521: R interface for Apache Sedona
yitao-li opened a new pull request #521:
URL: https://github.com/apache/incubator-sedona/pull/521
Signed-off-by: Yitao Li <yi...@rstudio.com>
## Is this PR related to a proposed Issue?
https://github.com/r-spark/sparklyr.sedona/issues/39
## What changes were proposed in this PR?
This PR will make `sparklyr.sedona` the official R interface for Apache Sedona. See https://github.com/r-spark/sparklyr.sedona#overview for an overview of the current state of `sparklyr.sedona`.
Alternatively, we can make `sparklyr.sedona` a git submodule. Suggestions are welcome.
- https://github.com/r-spark/sparklyr.sedona will become part of the apache/incubator-sedona repo
- A github actions workflow was created to run `sparklyr.sedona` with snapshot Sedona libraries built from the PR branch (instead of published ones from Maven central repository)
## How was this patch tested?
https://github.com/yitao-li/incubator-sedona/actions/runs/837316500 (i.e., it was tested by creating a PR from my own fork of Apache Sedona at the moment)
## Did this PR include necessary documentation updates?
- There is currently a summary plus some short tutorials of `sparklyr.sedona` functionalities in the README.md file.
- All exported R functions of `sparklyr.sedona` have roxygen-style documentation, per standard practice recommended by CRAN.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-sedona] yitao-li commented on a change in pull request #521: [SEDONA-31] R interface for Apache Sedona
Posted by GitBox <gi...@apache.org>.
yitao-li commented on a change in pull request #521:
URL: https://github.com/apache/incubator-sedona/pull/521#discussion_r636424058
##########
File path: R/sparklyr.sedona/inst/extdata/arealm-tiny.csv
##########
@@ -0,0 +1,5 @@
+testattribute0,-88.331492,32.324142,testattribute1,testattribute2
Review comment:
Sure. Will do.
Besides test data, there are also a small number of other data files that will be part of the R package for illustration purposes [*]. I think I can simply replace the content of those with random numbers and attributes (i.e., avoid anything with copy-right issues).
[*] Illustration purposes: because nowadays CRAN requires R packages to have runnable examples, and part of `sparklyr.sedona` involves reading spatial files of various formats, a small number of example data files will need to be packaged into sparklyr.sedona`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-sedona] jiayuasu merged pull request #521: [SEDONA-31] R interface for Apache Sedona
Posted by GitBox <gi...@apache.org>.
jiayuasu merged pull request #521:
URL: https://github.com/apache/incubator-sedona/pull/521
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-sedona] yitao-li commented on a change in pull request #521: [SEDONA-31] R interface for Apache Sedona
Posted by GitBox <gi...@apache.org>.
yitao-li commented on a change in pull request #521:
URL: https://github.com/apache/incubator-sedona/pull/521#discussion_r636107479
##########
File path: R/sparklyr.sedona/DESCRIPTION
##########
@@ -0,0 +1,29 @@
+Type: Package
+Package: sparklyr.sedona
+Title: Sparklyr Extension for Apache Sedona
+Version: 0.1.0
+Authors@R:
Review comment:
Sure. There is one logistical matter we need to think about though: when submitting this R package to CRAN, there will be an email from CRAN to the maintainer's email address containing a confirmation link, and someone will need to click that link each time. Would dev@sedona.apache.org work for that purpose?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-sedona] jiayuasu commented on a change in pull request #521: [SEDONA-31] R interface for Apache Sedona
Posted by GitBox <gi...@apache.org>.
jiayuasu commented on a change in pull request #521:
URL: https://github.com/apache/incubator-sedona/pull/521#discussion_r637222020
##########
File path: R/sparklyr.sedona/DESCRIPTION
##########
@@ -0,0 +1,31 @@
+Type: Package
+Package: sparklyr.sedona
+Title: Sparklyr Extension for Apache Sedona
+Version: 0.1.0
+Authors@R:
+ c(person(family = "Apache Sedona",
+ role = c("aut", "cre"),
+ email = "dev@sedona.apache.org"),
Review comment:
Oh I just realized the dev@s.a.o is open to the public and anyone has access to it. Would you please change this to private@sedona.apache.org? This way, only PMC members can control the publish of this doc. Once you are added to Sedona PMC (after voting), you will have access to private@sedona.apache.org.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-sedona] lorenzwalthert commented on pull request #521: [SEDONA-31] R interface for Apache Sedona
Posted by GitBox <gi...@apache.org>.
lorenzwalthert commented on pull request #521:
URL: https://github.com/apache/incubator-sedona/pull/521#issuecomment-845242643
@yitao-li great to see this happening. Maybe a good moment to run {styler} through it before merging this? I only noted a few deviations from the style guide though...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-sedona] jiayuasu commented on a change in pull request #521: [SEDONA-31] R interface for Apache Sedona
Posted by GitBox <gi...@apache.org>.
jiayuasu commented on a change in pull request #521:
URL: https://github.com/apache/incubator-sedona/pull/521#discussion_r637222020
##########
File path: R/sparklyr.sedona/DESCRIPTION
##########
@@ -0,0 +1,31 @@
+Type: Package
+Package: sparklyr.sedona
+Title: Sparklyr Extension for Apache Sedona
+Version: 0.1.0
+Authors@R:
+ c(person(family = "Apache Sedona",
+ role = c("aut", "cre"),
+ email = "dev@sedona.apache.org"),
Review comment:
Oh I just realized the dev@s.a.o is open to the public and anyone has access to it. Would you please change this to private@sedona.apache.org? This way, only PMC members can control how to publish the package. Once you are added to Sedona PMC (after voting), you will have access to private@sedona.apache.org.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-sedona] yitao-li commented on a change in pull request #521: [SEDONA-31] R interface for Apache Sedona
Posted by GitBox <gi...@apache.org>.
yitao-li commented on a change in pull request #521:
URL: https://github.com/apache/incubator-sedona/pull/521#discussion_r636424058
##########
File path: R/sparklyr.sedona/inst/extdata/arealm-tiny.csv
##########
@@ -0,0 +1,5 @@
+testattribute0,-88.331492,32.324142,testattribute1,testattribute2
Review comment:
Sure. Will do.
Besides test data, there are also a small number of other data files that will be part of the R package for illustration purposes [*]. I think I can simply replace the content of those with random numbers and strings (i.e., avoid anything with copy-right issues).
[*] Illustration purposes: because nowadays CRAN requires R packages to have runnable examples, and part of `sparklyr.sedona` involves reading spatial files of various formats, a small number of example data files will need to be packaged into `sparklyr.sedona`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-sedona] jiayuasu commented on a change in pull request #521: [SEDONA-31] R interface for Apache Sedona
Posted by GitBox <gi...@apache.org>.
jiayuasu commented on a change in pull request #521:
URL: https://github.com/apache/incubator-sedona/pull/521#discussion_r635837137
##########
File path: .github/workflows/r.yml
##########
@@ -0,0 +1,129 @@
+name: R build
+
+on:
+ push:
+ branches:
+ - master
+ pull_request:
+ branches:
+ - '*'
+
+jobs:
+ build:
+
+ runs-on: ubuntu-18.04
+ strategy:
+ fail-fast: false
+ matrix:
+ include:
+ - name: 'Spark 3.0.1 (R release)'
+ r: 'release'
+ env:
+ SPARK_VERSION: '3.0.1'
Review comment:
Could you please upgrade the CI to support Spark 3.1.1?
Please see our latest CI example https://github.com/apache/incubator-sedona/blob/master/.github/workflows/java.yml#L43
##########
File path: R/sparklyr.sedona/DESCRIPTION
##########
@@ -0,0 +1,29 @@
+Type: Package
+Package: sparklyr.sedona
+Title: Sparklyr Extension for Apache Sedona
+Version: 0.1.0
+Authors@R:
Review comment:
Could you change the maintainer name to Apache Sedona? See here: https://github.com/apache/incubator-sedona/blob/master/python/setup.py#L33
##########
File path: R/sparklyr.sedona/inst/extdata/arealm-tiny.csv
##########
@@ -0,0 +1,5 @@
+testattribute0,-88.331492,32.324142,testattribute1,testattribute2
Review comment:
Can you re-use the test data provided in https://github.com/apache/incubator-sedona/tree/master/core/src/test/resources, by referring to their relative path? Currently Sedona has too many test data files. It leads to some maintenance-wise and copyright-wise issues. We plan to reuse and remove some of them in the future. See [SEDONA-34](https://issues.apache.org/jira/browse/SEDONA-34)
##########
File path: R/sparklyr.sedona/man/sedona_read_geojson_to_typed_rdd.Rd
##########
@@ -0,0 +1,64 @@
+% Generated by roxygen2: do not edit by hand
Review comment:
For the documentation, please do not include generated docs in the PR. Please include the source code of docs in the PR. Generated R docs will be manually integrated with generated html files (based on incubator-sedona/docs/) and uploaded to https://github.com/apache/incubator-sedona-website/tree/asf-site
I believe the R docs itself could be a separate PR since it requires some structure change or doc change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-sedona] yitao-li commented on a change in pull request #521: [SEDONA-31] R interface for Apache Sedona
Posted by GitBox <gi...@apache.org>.
yitao-li commented on a change in pull request #521:
URL: https://github.com/apache/incubator-sedona/pull/521#discussion_r636424058
##########
File path: R/sparklyr.sedona/inst/extdata/arealm-tiny.csv
##########
@@ -0,0 +1,5 @@
+testattribute0,-88.331492,32.324142,testattribute1,testattribute2
Review comment:
Sure. Will do.
Besides test data, there are also small number of other data files that will be part of the R package for illustration purposes [*]. I think I can simply replace the content of those with random numbers and attributes (i.e., avoid anything with copy-right issues).
[*] Illustration purposes: because nowadays CRAN requires R packages to have runnable examples, and part of `sparklyr.sedona` involves reading spatial files of various formats, a small number of example data files will need to be packaged into sparklyr.sedona`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-sedona] yitao-li commented on a change in pull request #521: [SEDONA-31] R interface for Apache Sedona
Posted by GitBox <gi...@apache.org>.
yitao-li commented on a change in pull request #521:
URL: https://github.com/apache/incubator-sedona/pull/521#discussion_r636424058
##########
File path: R/sparklyr.sedona/inst/extdata/arealm-tiny.csv
##########
@@ -0,0 +1,5 @@
+testattribute0,-88.331492,32.324142,testattribute1,testattribute2
Review comment:
Sure. Will do.
Besides test data, there are also a small number of other data files that will be part of the R package for illustration purposes [*]. I think I can simply replace the content of those with random numbers and strings (i.e., avoid anything with copy-right issues).
[*] Illustration purposes: because nowadays CRAN requires R packages to have runnable examples, and part of `sparklyr.sedona` involves reading spatial files of various formats, a small number of example data files will need to be packaged into `sparklyr.sedona`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-sedona] jiayuasu commented on pull request #521: [SEDONA-31] R interface for Apache Sedona
Posted by GitBox <gi...@apache.org>.
jiayuasu commented on pull request #521:
URL: https://github.com/apache/incubator-sedona/pull/521#issuecomment-841466278
@yitao-li Thank you very much for this great PR. I am currently trying to cut a release for Sedona 1.0.1 in one or two days. There are some structures change may happen inside Sedona codebase due to some copyright issues.
I will give you detailed code review once I cut the release.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-sedona] yitao-li commented on pull request #521: [SEDONA-31] R interface for Apache Sedona
Posted by GitBox <gi...@apache.org>.
yitao-li commented on pull request #521:
URL: https://github.com/apache/incubator-sedona/pull/521#issuecomment-847083427
@jiayuasu Sure. Thanks for the code review! :+1:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-sedona] yitao-li commented on a change in pull request #521: [SEDONA-31] R interface for Apache Sedona
Posted by GitBox <gi...@apache.org>.
yitao-li commented on a change in pull request #521:
URL: https://github.com/apache/incubator-sedona/pull/521#discussion_r636462051
##########
File path: core/src/test/resources/points.json
##########
@@ -0,0 +1,7 @@
+{"type":"Feature","geometry":{"type":"Point","coordinates":[-88.1234,32.8888]},"properties":{"FIELD1":"testattribute0","FIELD4":"testattribute1","FIELD5":"testattribute2"}},
Review comment:
This file contains some random numbers (so, no copy right issue here)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-sedona] jiayuasu commented on pull request #521: [SEDONA-31] R interface for Apache Sedona
Posted by GitBox <gi...@apache.org>.
jiayuasu commented on pull request #521:
URL: https://github.com/apache/incubator-sedona/pull/521#issuecomment-844830313
@yitao-li Thank you again for this great PR. Please see my comments above. I believe two current TO-DOs for this PR is (1) re-use existing test data in core/test/resources by referring to their relative paths. (2) Put the R docs in a separate PR and we may need to have some discussion on it.
Moreover, once this R module gets merged into Apache Sedona, I would like to invite you to be a PMC member of Apache Sedona (of course, this needs to pass the community voting phase). We already have several great members on board: @Sarwat and I are the initial architect of Sedona. @jinxuan @zongsizhang and @netanel246 contribute to Sedona architecture, Shapefile reader and serializer. @Imbruced is in charge of Sedona Python.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-sedona] yitao-li commented on a change in pull request #521: [SEDONA-31] R interface for Apache Sedona
Posted by GitBox <gi...@apache.org>.
yitao-li commented on a change in pull request #521:
URL: https://github.com/apache/incubator-sedona/pull/521#discussion_r636424058
##########
File path: R/sparklyr.sedona/inst/extdata/arealm-tiny.csv
##########
@@ -0,0 +1,5 @@
+testattribute0,-88.331492,32.324142,testattribute1,testattribute2
Review comment:
Sure. Will do.
Besides test data, there are also a small number of other data files that will be part of the R package for illustration purposes [*]. I think I can simply replace the content of those with random numbers and strings (i.e., avoid anything with copy-right issues).
[*] Illustration purposes: because nowadays CRAN requires R packages to have runnable examples, and part of `sparklyr.sedona` involves reading spatial files of various formats, a small number of example data files will need to be packaged into sparklyr.sedona`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-sedona] yitao-li commented on a change in pull request #521: [SEDONA-31] R interface for Apache Sedona
Posted by GitBox <gi...@apache.org>.
yitao-li commented on a change in pull request #521:
URL: https://github.com/apache/incubator-sedona/pull/521#discussion_r633681332
##########
File path: R/sparklyr.sedona/tests/testthat/test-sdf-interface.R
##########
@@ -0,0 +1,43 @@
+context("sdf interface")
+
+sc <- testthat_spark_connection()
+
+pt_rdd <- sedona_read_dsv_to_typed_rdd(
+ sc,
+ location = test_data("arealm-small.csv"),
+ delimiter = ",",
+ type = "point",
+ first_spatial_col_index = 1,
+ has_non_spatial_attrs = TRUE
+)
+
+test_that("sdf_register() works as expected for Spatial RDDs", {
+ sdf_name <- random_string("spatial_sdf")
+ pt_sdf <- sdf_register(pt_rdd, name = sdf_name)
+
+ expect_equivalent(
+ pt_sdf %>% sdf_schema(),
+ list(geometry = list(name = "geometry", type = "GeometryUDT"))
+ )
+ expect_equal(pt_sdf %>% dbplyr::remote_name(), dbplyr::ident(sdf_name))
+
+ pt_sdf %>% collect()
+ succeed()
+})
+
+# TODO:
+# test_that("as.spark.dataframe() works as expected for Spatial RDDs with non-spatial attributes", {
+# sdf_name <- random_string("spatial_sdf")
+# pt_sdf <- as.spark.dataframe(
+# pt_rdd, non_spatial_cols = paste0("attr_", seq(3)), name = sdf_name
+# )
+#
+# expect_equivalent(
+# pt_sdf %>% sdf_schema(),
+# list(geometry = list(name = "geometry", type = "GeometryUDT"))
+# )
+# expect_equal(pt_sdf %>% dbplyr::remote_name(), dbplyr::ident(sdf_name))
+#
+# pt_sdf %>% collect()
+# succeed()
+# })
Review comment:
@jiayuasu The R test case that is commented out here is not working yet, but it's almost working.
I tihnk it will be fixed as soon as we address a null-pointer issue with `Adapter.toDf()` (please see https://github.com/apache/incubator-sedona/pull/525 and let me know what you think).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-sedona] jiayuasu edited a comment on pull request #521: [SEDONA-31] R interface for Apache Sedona
Posted by GitBox <gi...@apache.org>.
jiayuasu edited a comment on pull request #521:
URL: https://github.com/apache/incubator-sedona/pull/521#issuecomment-844830313
@yitao-li Thank you again for this great PR. Please see my comments above. I believe two current TO-DOs for this PR is (1) re-use existing test data in core/test/resources by referring to their relative paths. (2) Put the R docs in a separate PR and we may need to have some discussion on it.
Moreover, once this R module gets merged into Apache Sedona, I would like to invite you to be a PMC member of Apache Sedona (of course, this needs to pass the community voting phase). You will be in charge of the R module in Sedona. Currently, we don't have any R experts in Sedona PMC.
We already have several great members on board: @Sarwat and I are the initial architect of Sedona. @jinxuan @zongsizhang and @netanel246 contribute to Sedona architecture, Shapefile reader and serializer. @Imbruced is in charge of Sedona Python.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-sedona] yitao-li commented on a change in pull request #521: [SEDONA-31] R interface for Apache Sedona
Posted by GitBox <gi...@apache.org>.
yitao-li commented on a change in pull request #521:
URL: https://github.com/apache/incubator-sedona/pull/521#discussion_r636424058
##########
File path: R/sparklyr.sedona/inst/extdata/arealm-tiny.csv
##########
@@ -0,0 +1,5 @@
+testattribute0,-88.331492,32.324142,testattribute1,testattribute2
Review comment:
Sure. Will do.
Besides test data, there are also small number of other data files that will be part of the R package for illustration purposes. I think I can simply replace those ones with random numbers and avoid anything with copy-right issues.
Because nowadays CRAN requires R packages to have runnable examples, and part of `sparklyr.sedona` involves reading spatial files of various formats, a small number of example data files will need to be packaged into sparklyr.sedona`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-sedona] yitao-li commented on a change in pull request #521: [SEDONA-31] R interface for Apache Sedona
Posted by GitBox <gi...@apache.org>.
yitao-li commented on a change in pull request #521:
URL: https://github.com/apache/incubator-sedona/pull/521#discussion_r633681332
##########
File path: R/sparklyr.sedona/tests/testthat/test-sdf-interface.R
##########
@@ -0,0 +1,43 @@
+context("sdf interface")
+
+sc <- testthat_spark_connection()
+
+pt_rdd <- sedona_read_dsv_to_typed_rdd(
+ sc,
+ location = test_data("arealm-small.csv"),
+ delimiter = ",",
+ type = "point",
+ first_spatial_col_index = 1,
+ has_non_spatial_attrs = TRUE
+)
+
+test_that("sdf_register() works as expected for Spatial RDDs", {
+ sdf_name <- random_string("spatial_sdf")
+ pt_sdf <- sdf_register(pt_rdd, name = sdf_name)
+
+ expect_equivalent(
+ pt_sdf %>% sdf_schema(),
+ list(geometry = list(name = "geometry", type = "GeometryUDT"))
+ )
+ expect_equal(pt_sdf %>% dbplyr::remote_name(), dbplyr::ident(sdf_name))
+
+ pt_sdf %>% collect()
+ succeed()
+})
+
+# TODO:
+# test_that("as.spark.dataframe() works as expected for Spatial RDDs with non-spatial attributes", {
+# sdf_name <- random_string("spatial_sdf")
+# pt_sdf <- as.spark.dataframe(
+# pt_rdd, non_spatial_cols = paste0("attr_", seq(3)), name = sdf_name
+# )
+#
+# expect_equivalent(
+# pt_sdf %>% sdf_schema(),
+# list(geometry = list(name = "geometry", type = "GeometryUDT"))
+# )
+# expect_equal(pt_sdf %>% dbplyr::remote_name(), dbplyr::ident(sdf_name))
+#
+# pt_sdf %>% collect()
+# succeed()
+# })
Review comment:
@jiayuasu The test case that is commented out here is not working yet, but it's almost working.
I tihnk it will be fixed as soon as we address a null-pointer issue with `Adapter.toDf()` (please see https://github.com/apache/incubator-sedona/pull/525 and let me know what you think).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-sedona] yitao-li edited a comment on pull request #521: [SEDONA-31] R interface for Apache Sedona
Posted by GitBox <gi...@apache.org>.
yitao-li edited a comment on pull request #521:
URL: https://github.com/apache/incubator-sedona/pull/521#issuecomment-847083427
@jiayuasu Sure. Thanks for the code review! :+1:
I'm quite excited for sparklyr.sedona to be part of the Sedona project soon.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-sedona] yitao-li commented on a change in pull request #521: [SEDONA-31] R interface for Apache Sedona
Posted by GitBox <gi...@apache.org>.
yitao-li commented on a change in pull request #521:
URL: https://github.com/apache/incubator-sedona/pull/521#discussion_r636424058
##########
File path: R/sparklyr.sedona/inst/extdata/arealm-tiny.csv
##########
@@ -0,0 +1,5 @@
+testattribute0,-88.331492,32.324142,testattribute1,testattribute2
Review comment:
Sure. Will do.
Besides test data, there are also small number of other data files that will be part of the R package for illustration purposes [*]. I think I can simply replace those ones with random numbers and avoid anything with copy-right issues.
[*] Illustration purposes: because nowadays CRAN requires R packages to have runnable examples, and part of `sparklyr.sedona` involves reading spatial files of various formats, a small number of example data files will need to be packaged into sparklyr.sedona`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-sedona] yitao-li commented on a change in pull request #521: [SEDONA-31] R interface for Apache Sedona
Posted by GitBox <gi...@apache.org>.
yitao-li commented on a change in pull request #521:
URL: https://github.com/apache/incubator-sedona/pull/521#discussion_r636115842
##########
File path: R/sparklyr.sedona/DESCRIPTION
##########
@@ -0,0 +1,29 @@
+Type: Package
+Package: sparklyr.sedona
+Title: Sparklyr Extension for Apache Sedona
+Version: 0.1.0
+Authors@R:
Review comment:
If it's a mailing list then that should be fine. I imagine we will vote before starting a release of this R package to CRAN anyways.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-sedona] yitao-li commented on a change in pull request #521: [SEDONA-31] R interface for Apache Sedona
Posted by GitBox <gi...@apache.org>.
yitao-li commented on a change in pull request #521:
URL: https://github.com/apache/incubator-sedona/pull/521#discussion_r633681332
##########
File path: R/sparklyr.sedona/tests/testthat/test-sdf-interface.R
##########
@@ -0,0 +1,43 @@
+context("sdf interface")
+
+sc <- testthat_spark_connection()
+
+pt_rdd <- sedona_read_dsv_to_typed_rdd(
+ sc,
+ location = test_data("arealm-small.csv"),
+ delimiter = ",",
+ type = "point",
+ first_spatial_col_index = 1,
+ has_non_spatial_attrs = TRUE
+)
+
+test_that("sdf_register() works as expected for Spatial RDDs", {
+ sdf_name <- random_string("spatial_sdf")
+ pt_sdf <- sdf_register(pt_rdd, name = sdf_name)
+
+ expect_equivalent(
+ pt_sdf %>% sdf_schema(),
+ list(geometry = list(name = "geometry", type = "GeometryUDT"))
+ )
+ expect_equal(pt_sdf %>% dbplyr::remote_name(), dbplyr::ident(sdf_name))
+
+ pt_sdf %>% collect()
+ succeed()
+})
+
+# TODO:
+# test_that("as.spark.dataframe() works as expected for Spatial RDDs with non-spatial attributes", {
+# sdf_name <- random_string("spatial_sdf")
+# pt_sdf <- as.spark.dataframe(
+# pt_rdd, non_spatial_cols = paste0("attr_", seq(3)), name = sdf_name
+# )
+#
+# expect_equivalent(
+# pt_sdf %>% sdf_schema(),
+# list(geometry = list(name = "geometry", type = "GeometryUDT"))
+# )
+# expect_equal(pt_sdf %>% dbplyr::remote_name(), dbplyr::ident(sdf_name))
+#
+# pt_sdf %>% collect()
+# succeed()
+# })
Review comment:
@jiayuasu The test case that is commented out here is not working yet, but it's almost working.
I tihnk it will be fixed as soon as we address a null-pointer issue with `Adapter.toDf()` (please see https://github.com/apache/incubator-sedona/pull/525 and let me know what you think).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org