You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/07/08 11:43:12 UTC
[GitHub] [spark] zhengruifeng opened a new pull request, #37135: [SPARK-39723][R] Make ListFunctions/functionExists in SparkR support 3L namespace
zhengruifeng opened a new pull request, #37135:
URL: https://github.com/apache/spark/pull/37135
### What changes were proposed in this pull request?
Make ListFunctions/functionExists in SparkR support 3L namespace
### Why are the changes needed?
for 3L namespace
### Does this PR introduce _any_ user-facing change?
yes, new API functionExists
### How was this patch tested?
added UT
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #37135: [SPARK-39723][R] Implement functionExists/getFunc in SparkR for 3L namespace
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on code in PR #37135:
URL: https://github.com/apache/spark/pull/37135#discussion_r918448953
##########
R/pkg/NAMESPACE:
##########
@@ -479,7 +479,9 @@ export("as.DataFrame",
"databaseExists",
"dropTempTable",
"dropTempView",
+ "functionExists",
"getDatabase",
+ "getFunction",
Review Comment:
```suggestion
"getFunc",
```
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zhengruifeng commented on pull request #37135: [SPARK-39723][R] Implement functionExists/getFunction in SparkR for 3L namespace
Posted by GitBox <gi...@apache.org>.
zhengruifeng commented on PR #37135:
URL: https://github.com/apache/spark/pull/37135#issuecomment-1180323026
an irrelevent test `[Run / Build modules: catalyst, hive-thriftserver](https://github.com/zhengruifeng/spark/runs/7281537061?check_suite_focus=true#logs)` got stuck for more than 3hours, let me retest it.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #37135: [SPARK-39723][R] Implement functionExists/getFunction in SparkR for 3L namespace
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on code in PR #37135:
URL: https://github.com/apache/spark/pull/37135#discussion_r917892592
##########
R/pkg/tests/fulltests/test_context.R:
##########
@@ -21,10 +21,11 @@ test_that("Check masked functions", {
# Check that we are not masking any new function from base, stats, testthat unexpectedly
# NOTE: We should avoid adding entries to *namesOfMaskedCompletely* as masked functions make it
# hard for users to use base R functions. Please check when in doubt.
- namesOfMaskedCompletely <- c("cov", "filter", "sample", "not")
+ namesOfMaskedCompletely <- c("cov", "filter", "sample", "not", "getFunction")
Review Comment:
Ah .. even with that the signature would likely conflict. hm
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zhengruifeng closed pull request #37135: [SPARK-39723][R] Implement functionExists/getFunc in SparkR for 3L namespace
Posted by GitBox <gi...@apache.org>.
zhengruifeng closed pull request #37135: [SPARK-39723][R] Implement functionExists/getFunc in SparkR for 3L namespace
URL: https://github.com/apache/spark/pull/37135
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #37135: [SPARK-39723][R] Implement functionExists/getFunction in SparkR for 3L namespace
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on code in PR #37135:
URL: https://github.com/apache/spark/pull/37135#discussion_r917891535
##########
R/pkg/tests/fulltests/test_context.R:
##########
@@ -21,10 +21,11 @@ test_that("Check masked functions", {
# Check that we are not masking any new function from base, stats, testthat unexpectedly
# NOTE: We should avoid adding entries to *namesOfMaskedCompletely* as masked functions make it
# hard for users to use base R functions. Please check when in doubt.
- namesOfMaskedCompletely <- c("cov", "filter", "sample", "not")
+ namesOfMaskedCompletely <- c("cov", "filter", "sample", "not", "getFunction")
Review Comment:
Should probably we should define this with `setMethod` (see `functions.R`) to avoid the complete masking.
##########
R/pkg/tests/fulltests/test_context.R:
##########
@@ -21,10 +21,11 @@ test_that("Check masked functions", {
# Check that we are not masking any new function from base, stats, testthat unexpectedly
# NOTE: We should avoid adding entries to *namesOfMaskedCompletely* as masked functions make it
# hard for users to use base R functions. Please check when in doubt.
- namesOfMaskedCompletely <- c("cov", "filter", "sample", "not")
+ namesOfMaskedCompletely <- c("cov", "filter", "sample", "not", "getFunction")
Review Comment:
Should probably we should define this with `setMethod` with a signature (see `functions.R`) to avoid the complete masking.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #37135: [SPARK-39723][R] Implement functionExists/getFunction in SparkR for 3L namespace
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on code in PR #37135:
URL: https://github.com/apache/spark/pull/37135#discussion_r917898330
##########
R/pkg/tests/fulltests/test_context.R:
##########
@@ -21,10 +21,11 @@ test_that("Check masked functions", {
# Check that we are not masking any new function from base, stats, testthat unexpectedly
# NOTE: We should avoid adding entries to *namesOfMaskedCompletely* as masked functions make it
# hard for users to use base R functions. Please check when in doubt.
- namesOfMaskedCompletely <- c("cov", "filter", "sample", "not")
+ namesOfMaskedCompletely <- c("cov", "filter", "sample", "not", "getFunction")
Review Comment:
Let's use a different name for now. e.g., `getFunc` and explain it in the docs.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zhengruifeng commented on a diff in pull request #37135: [SPARK-39723][R] Implement functionExists/getFunction in SparkR for 3L namespace
Posted by GitBox <gi...@apache.org>.
zhengruifeng commented on code in PR #37135:
URL: https://github.com/apache/spark/pull/37135#discussion_r918440246
##########
R/pkg/tests/fulltests/test_context.R:
##########
@@ -21,10 +21,11 @@ test_that("Check masked functions", {
# Check that we are not masking any new function from base, stats, testthat unexpectedly
# NOTE: We should avoid adding entries to *namesOfMaskedCompletely* as masked functions make it
# hard for users to use base R functions. Please check when in doubt.
- namesOfMaskedCompletely <- c("cov", "filter", "sample", "not")
+ namesOfMaskedCompletely <- c("cov", "filter", "sample", "not", "getFunction")
Review Comment:
got it, thanks. let me update the pr.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zhengruifeng commented on pull request #37135: [SPARK-39723][R] Implement functionExists/getFunc in SparkR for 3L namespace
Posted by GitBox <gi...@apache.org>.
zhengruifeng commented on PR #37135:
URL: https://github.com/apache/spark/pull/37135#issuecomment-1181262398
Merged to master, thanks @HyukjinKwon
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #37135: [SPARK-39723][R] Implement functionExists/getFunction in SparkR for 3L namespace
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on code in PR #37135:
URL: https://github.com/apache/spark/pull/37135#discussion_r918440696
##########
R/pkg/tests/fulltests/test_context.R:
##########
@@ -21,10 +21,11 @@ test_that("Check masked functions", {
# Check that we are not masking any new function from base, stats, testthat unexpectedly
# NOTE: We should avoid adding entries to *namesOfMaskedCompletely* as masked functions make it
# hard for users to use base R functions. Please check when in doubt.
- namesOfMaskedCompletely <- c("cov", "filter", "sample", "not")
+ namesOfMaskedCompletely <- c("cov", "filter", "sample", "not", "getFunction")
Review Comment:
cc @falaki @felixcheung @shivaram FYI
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zhengruifeng commented on a diff in pull request #37135: [SPARK-39723][R] Implement functionExists/getFunction in SparkR for 3L namespace
Posted by GitBox <gi...@apache.org>.
zhengruifeng commented on code in PR #37135:
URL: https://github.com/apache/spark/pull/37135#discussion_r917851222
##########
R/pkg/tests/fulltests/test_context.R:
##########
@@ -21,10 +21,11 @@ test_that("Check masked functions", {
# Check that we are not masking any new function from base, stats, testthat unexpectedly
# NOTE: We should avoid adding entries to *namesOfMaskedCompletely* as masked functions make it
# hard for users to use base R functions. Please check when in doubt.
- namesOfMaskedCompletely <- c("cov", "filter", "sample", "not")
+ namesOfMaskedCompletely <- c("cov", "filter", "sample", "not", "getFunction")
Review Comment:
to pass `test_context`, I need to mask the build-in [`getFunction`](https://www.rdocumentation.org/packages/methods/versions/3.6.2/topics/methodUtilities) , is this OK? @HyukjinKwon
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #37135: [SPARK-39723][R] Implement functionExists/getFunc in SparkR for 3L namespace
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on code in PR #37135:
URL: https://github.com/apache/spark/pull/37135#discussion_r918449024
##########
R/pkg/pkgdown/_pkgdown_template.yml:
##########
@@ -266,7 +266,9 @@ reference:
- databaseExists
- dropTempTable
- dropTempView
+ - functionExists
- getDatabase
+ - getFunction
Review Comment:
```suggestion
- getFunc
```
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org