[GitHub] spark issue #15608: [SPARK-17838][SparkR] Check named arguments for options ...
Github user felixcheung commented on the issue: https://github.com/apache/spark/pull/15608 merged to master. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #15608: [SPARK-17838][SparkR] Check named arguments for options ...
Github user HyukjinKwon commented on the issue: https://github.com/apache/spark/pull/15608 Yup, it seems not even `varargsToStrEnv`. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #15608: [SPARK-17838][SparkR] Check named arguments for options ...
Github user felixcheung commented on the issue: https://github.com/apache/spark/pull/15608 LGTM. I ran through a few other cases and I think the omitted names are handled properly with this. This should go to master then (handledCallJMethod is not in branch-2.0) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #15608: [SPARK-17838][SparkR] Check named arguments for options ...
Github user AmplabJenkins commented on the issue: https://github.com/apache/spark/pull/15608 Test PASSed. Refer to this link for build results (access rights to CI server needed): https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/67570/ Test PASSed. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #15608: [SPARK-17838][SparkR] Check named arguments for options ...
Github user AmplabJenkins commented on the issue: https://github.com/apache/spark/pull/15608 Merged build finished. Test PASSed. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #15608: [SPARK-17838][SparkR] Check named arguments for options ...
Github user SparkQA commented on the issue: https://github.com/apache/spark/pull/15608 **[Test build #67570 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/67570/consoleFull)** for PR 15608 at commit [`2336dd9`](https://github.com/apache/spark/commit/2336dd929f5500fa4c4a43e4137fe534d59f1d6f). * This patch passes all tests. * This patch merges cleanly. * This patch adds no public classes. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #15608: [SPARK-17838][SparkR] Check named arguments for options ...
Github user SparkQA commented on the issue: https://github.com/apache/spark/pull/15608 **[Test build #67570 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/67570/consoleFull)** for PR 15608 at commit [`2336dd9`](https://github.com/apache/spark/commit/2336dd929f5500fa4c4a43e4137fe534d59f1d6f). --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #15608: [SPARK-17838][SparkR] Check named arguments for options ...
Github user HyukjinKwon commented on the issue: https://github.com/apache/spark/pull/15608 @felixcheung I just made it as a single for loop (slightly different with the suggested one though..). Could you please check it again? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #15608: [SPARK-17838][SparkR] Check named arguments for options ...
Github user HyukjinKwon commented on the issue: https://github.com/apache/spark/pull/15608 I guess you meant when the options, for example, `DataFrameReader.extraOptions` turns into `Properties` when we call JDBC APIs. In this case, `Properties` always has a higher precedence and will overwrite `DataFrameReader.extraOptions`[1] and exclude Spark internal options[2]. [1]https://github.com/apache/spark/blob/master/sql/core/src/main/scala/org/apache/spark/sql/DataFrameReader.scala#L235 [2]https://github.com/apache/spark/blob/0c0ad436ad909364915b910867d08262c62bc95d/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCOptions.scala#L43 (BTW, it seems throwing `NullPointException` when we call `setProperty` and the key is `null`) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #15608: [SPARK-17838][SparkR] Check named arguments for options ...
Github user felixcheung commented on the issue:
https://github.com/apache/spark/pull/15608
re: JIRA - I don't mind one way or the other - both proposals sound good to
me.
> currently we don't make this failed when unused arbitrary options are
given (e.g. option("abc", "1")), so, it'd be okay to not make this failed. but
of course this is not a strong opinion.
a valid point. does it work properly when the Spark properties are set into
a Java Properties class?
https://docs.oracle.com/javase/7/docs/api/java/util/Properties.html#setProperty(java.lang.String,%20java.lang.String)
It seems if the key is `""` or `null` then it would overwrite?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #15608: [SPARK-17838][SparkR] Check named arguments for options ...
Github user HyukjinKwon commented on the issue: https://github.com/apache/spark/pull/15608 I am fine with leaving the JIRA open. I can definitely try to open followups. Otherwise, I also can convert the JIRA as a sub-task after introducing a parent JIRA. I will follow your lead. (If you close the JIRA then I will open another to make this as a sub-task. If you don't I will just try to make a followup in the future). > One case I'm not sure about, is what Reader/Writer does with unnamed parameter - should we optionally fail here on the R side, instead of just ignoring them (in R or JVM)? I was worried of this part too and It took me a while to build up my argument for this... My argument to convince myself was that currently we don't make this failed when unused arbitrary options are given (e.g. `option(`abc`, 1`)), so, it'd be okay to not make this failed. but of course this is not a strong opinion. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #15608: [SPARK-17838][SparkR] Check named arguments for options ...
Github user felixcheung commented on the issue: https://github.com/apache/spark/pull/15608 Thanks @HyukjinKwon This is definitely good checks to have. Calls to read.* and write.* are not easily checked for parameters (file paths are hard) so to me it is better to leave the checking to JVM and just handle the exception better. I wouldn't close this JIRA though but it is covering a broader case - those we could check parameters before passing to JVM? Would you like to keep this JIRA open after this PR, or open a separate JIRA for this specific class of handling? One case I'm not sure about, is what Reader/Writer does with unnamed parameter - should we optionally fail here on the R side, instead of just ignoring them (in R or JVM)? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #15608: [SPARK-17838][SparkR] Check named arguments for options ...
Github user AmplabJenkins commented on the issue: https://github.com/apache/spark/pull/15608 Test PASSed. Refer to this link for build results (access rights to CI server needed): https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/67440/ Test PASSed. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #15608: [SPARK-17838][SparkR] Check named arguments for options ...
Github user AmplabJenkins commented on the issue: https://github.com/apache/spark/pull/15608 Merged build finished. Test PASSed. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #15608: [SPARK-17838][SparkR] Check named arguments for options ...
Github user SparkQA commented on the issue: https://github.com/apache/spark/pull/15608 **[Test build #67440 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/67440/consoleFull)** for PR 15608 at commit [`e6afa4b`](https://github.com/apache/spark/commit/e6afa4b0b03cb0b428e79f030cac178ab45fa603). * This patch passes all tests. * This patch merges cleanly. * This patch adds no public classes. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #15608: [SPARK-17838][SparkR] Check named arguments for options ...
Github user SparkQA commented on the issue: https://github.com/apache/spark/pull/15608 **[Test build #67440 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/67440/consoleFull)** for PR 15608 at commit [`e6afa4b`](https://github.com/apache/spark/commit/e6afa4b0b03cb0b428e79f030cac178ab45fa603). --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #15608: [SPARK-17838][SparkR] Check named arguments for options ...
Github user HyukjinKwon commented on the issue: https://github.com/apache/spark/pull/15608 cc @felixcheung I recall we talked about this before. I first wanted to handle all the argument type checking but I just decided to do what I am pretty sure of (I remember we were concern of sweeping). Could you please take a look? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
