You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by HyukjinKwon <gi...@git.apache.org> on 2016/06/05 07:29:30 UTC

[GitHub] spark pull request #13517: [SPARK-14839][SQL] Support for other types as opt...

GitHub user HyukjinKwon opened a pull request:

    https://github.com/apache/spark/pull/13517

    [SPARK-14839][SQL] Support for other types as option in OPTIONS clause

    ## What changes were proposed in this pull request?
    
    Currently, Scala API supports to take options with the types, `String`, `Long`, `Double` and `Boolean` and Python API also supports other types. 
    
    This PR adds the support for other types (string, boolean, double and integer) as options so that support the options for data sources in a consistent way.
    
    While looking up other options, I realised that `samplingRatio` documentation is missing. So, I added the documentation.
    
    Also, `TODO add bucketing and partitioning.` was removed because it was resolved in https://github.com/apache/spark/commit/24bea000476cdd0b43be5160a76bc5b170ef0b42
    
    ## How was this patch tested?
    
    Unit test in `MetastoreDataSourcesSuite.scala`.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/HyukjinKwon/spark SPARK-14839

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/spark/pull/13517.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #13517
    
----
commit ba014eee8f910262b6db2d9ec4fe84478890a03c
Author: hyukjinkwon <gu...@gmail.com>
Date:   2016-06-05T06:26:49Z

    Support for other types in option clause in SQL Syntax

commit 1f94ca4a47ae3ae99d41551466f42d4ce96c3f07
Author: hyukjinkwon <gu...@gmail.com>
Date:   2016-06-05T06:38:44Z

    Update commnets for a new function

commit 1833d4f66a8c80314f3075d0088cc1903f6ed16f
Author: hyukjinkwon <gu...@gmail.com>
Date:   2016-06-05T07:07:55Z

    Add a todo

commit 1e6bdd3a3b6e303006004bd893b6bbe20561016f
Author: hyukjinkwon <gu...@gmail.com>
Date:   2016-06-05T07:22:16Z

    Add override

commit 905bdc594ebbb658d9392585af3a0ab7206841e1
Author: hyukjinkwon <gu...@gmail.com>
Date:   2016-06-05T07:27:12Z

    Change documentation for the correct explanation

----


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types for `tablePro...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    **[Test build #61698 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/61698/consoleFull)** for PR 13517 at commit [`30dfea0`](https://github.com/apache/spark/commit/30dfea05bb0ce864a7ccb5fe6a2d091c7fe3c988).


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request #13517: [SPARK-14839][SQL] Support for other types for `t...

Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:

    https://github.com/apache/spark/pull/13517


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request #13517: [SPARK-14839][SQL] Support for other types as opt...

Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on a diff in the pull request:

    https://github.com/apache/spark/pull/13517#discussion_r68698738
  
    --- Diff: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 ---
    @@ -252,6 +252,21 @@ tablePropertyKey
         | STRING
         ;
     
    +optionParameterList
    +    : '(' optionParameter (',' optionParameter)* ')'
    +    ;
    +
    +optionParameter
    +    : key=tablePropertyKey (EQ? value=optionValue)?
    --- End diff --
    
    We could remove `EQ?` here. This is actually not supported by data source tables. 


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types as option in ...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    **[Test build #60032 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/60032/consoleFull)** for PR 13517 at commit [`0c692e9`](https://github.com/apache/spark/commit/0c692e9032c57af002878c0a9be5f8cc85b369d2).
     * 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request #13517: [SPARK-14839][SQL] Support for other types as opt...

Posted by davies <gi...@git.apache.org>.
Github user davies commented on a diff in the pull request:

    https://github.com/apache/spark/pull/13517#discussion_r65975446
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala ---
    @@ -435,6 +434,37 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder {
       }
     
       /**
    +   * Parse a key-value map from a [[OptionParameterListContext]], assuming all values are
    +   * specified. This allows string, boolean, decimal and integer literals which are converted
    +   * to strings.
    +   */
    +  override def visitOptionParameterList(ctx: OptionParameterListContext): Map[String, String] = {
    +    // TODO: Currently it does not treat null. Hive does not allow null for metadata and
    +    // throws an exception.
    +    val properties = ctx.optionParameter.asScala.map { property =>
    +      val key = visitTablePropertyKey(property.key)
    +      val value = if (property.value.STRING != null) {
    +        string(property.value.STRING)
    +      } else if (property.value.booleanValue != null) {
    +        property.value.getText.toLowerCase
    +      } else {
    +        property.value.getText
    +      }
    +      key -> value
    +    }
    +
    +    // Check for duplicate property names.
    +    checkDuplicateKeys(properties, ctx)
    +    val props = properties.toMap
    +    val badKeys = props.filter { case (_, v) => v == null }.keys
    +    if (badKeys.nonEmpty) {
    +      throw operationNotAllowed(
    +        s"Values should not be specified for key(s): ${badKeys.mkString("[", ",", "]")}", ctx)
    --- End diff --
    
    This error message does not match with the behavior.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types for `tablePro...

Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    (@hvanhovell I just addressed your comments!)


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types as option in ...

Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    cc @davies and @rxin I addressed the comments. Thanks!


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request #13517: [SPARK-14839][SQL] Support for other types as opt...

Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:

    https://github.com/apache/spark/pull/13517#discussion_r69377675
  
    --- Diff: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 ---
    @@ -45,11 +45,11 @@ statement
         | ALTER DATABASE identifier SET DBPROPERTIES tablePropertyList     #setDatabaseProperties
         | DROP DATABASE (IF EXISTS)? identifier (RESTRICT | CASCADE)?      #dropDatabase
         | createTableHeader ('(' colTypeList ')')? tableProvider
    -        (OPTIONS tablePropertyList)?
    +        (OPTIONS optionParameterList)?
             (PARTITIONED BY partitionColumnNames=identifierList)?
             bucketSpec?                                                    #createTableUsing
         | createTableHeader tableProvider
    -        (OPTIONS tablePropertyList)?
    --- End diff --
    
    (Sorry for pining @hvanhovell)


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types as option in ...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    **[Test build #61429 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/61429/consoleFull)** for PR 13517 at commit [`ea2af9b`](https://github.com/apache/spark/commit/ea2af9b44e7fe177b73cb105eea4757788b984bc).
     * 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request #13517: [SPARK-14839][SQL] Support for other types as opt...

Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:

    https://github.com/apache/spark/pull/13517#discussion_r69385174
  
    --- Diff: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 ---
    @@ -45,11 +45,11 @@ statement
         | ALTER DATABASE identifier SET DBPROPERTIES tablePropertyList     #setDatabaseProperties
         | DROP DATABASE (IF EXISTS)? identifier (RESTRICT | CASCADE)?      #dropDatabase
         | createTableHeader ('(' colTypeList ')')? tableProvider
    -        (OPTIONS tablePropertyList)?
    +        (OPTIONS optionParameterList)?
             (PARTITIONED BY partitionColumnNames=identifierList)?
             bucketSpec?                                                    #createTableUsing
         | createTableHeader tableProvider
    -        (OPTIONS tablePropertyList)?
    --- End diff --
    
    @hvanhovell I see. Thanks, I didn't expect you are on holidays..
    I will push some commits and wait. Please feel free to review when you have some time!


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types as option in ...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types as option in ...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    **[Test build #60092 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/60092/consoleFull)** for PR 13517 at commit [`eb0031a`](https://github.com/apache/spark/commit/eb0031af138ed40a8ede9329e10d5fe0ea7e7e8c).


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request #13517: [SPARK-14839][SQL] Support for other types as opt...

Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on a diff in the pull request:

    https://github.com/apache/spark/pull/13517#discussion_r68699131
  
    --- Diff: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 ---
    @@ -45,11 +45,11 @@ statement
         | ALTER DATABASE identifier SET DBPROPERTIES tablePropertyList     #setDatabaseProperties
         | DROP DATABASE (IF EXISTS)? identifier (RESTRICT | CASCADE)?      #dropDatabase
         | createTableHeader ('(' colTypeList ')')? tableProvider
    -        (OPTIONS tablePropertyList)?
    +        (OPTIONS optionParameterList)?
             (PARTITIONED BY partitionColumnNames=identifierList)?
             bucketSpec?                                                    #createTableUsing
         | createTableHeader tableProvider
    -        (OPTIONS tablePropertyList)?
    --- End diff --
    
    Why not generalize the `tableProperty` rule and use `optionValue` (rename it to something more consistent) as its value rule? Seems easier.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types as option in ...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    **[Test build #61672 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/61672/consoleFull)** for PR 13517 at commit [`4b67bab`](https://github.com/apache/spark/commit/4b67bab4b8fc663284ac29b1e2b83ad75eb2ba74).


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types as option in ...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types for `tablePro...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types as option in ...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/61429/
    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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types as option in ...

Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    retest this please


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types as option in ...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/60032/
    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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request #13517: [SPARK-14839][SQL] Support for other types for `t...

Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:

    https://github.com/apache/spark/pull/13517#discussion_r69399556
  
    --- Diff: sql/hive/src/test/scala/org/apache/spark/sql/hive/MetastoreDataSourcesSuite.scala ---
    @@ -1117,4 +1117,26 @@ class MetastoreDataSourcesSuite extends QueryTest with SQLTestUtils with TestHiv
           }
         }
       }
    +
    +  test("SPARK-14839: Support for other types as option in OPTIONS clause") {
    --- End diff --
    
    Sure!


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types as option in ...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    **[Test build #61429 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/61429/consoleFull)** for PR 13517 at commit [`ea2af9b`](https://github.com/apache/spark/commit/ea2af9b44e7fe177b73cb105eea4757788b984bc).


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request #13517: [SPARK-14839][SQL] Support for other types as opt...

Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on a diff in the pull request:

    https://github.com/apache/spark/pull/13517#discussion_r68699390
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala ---
    @@ -435,6 +434,37 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder {
       }
     
       /**
    +   * Parse a key-value map from a [[OptionParameterListContext]], assuming all values are
    +   * specified. This allows string, boolean, decimal and integer literals which are converted
    +   * to strings.
    +   */
    +  override def visitOptionParameterList(ctx: OptionParameterListContext): Map[String, String] = {
    +    // TODO: Currently it does not treat null. Hive does not allow null for metadata and
    +    // throws an exception.
    +    val properties = ctx.optionParameter.asScala.map { property =>
    +      val key = visitTablePropertyKey(property.key)
    +      val value = if (property.value.STRING != null) {
    +        string(property.value.STRING)
    +      } else if (property.value.booleanValue != null) {
    +        property.value.getText.toLowerCase
    +      } else {
    +        property.value.getText
    +      }
    +      key -> value
    +    }
    +
    +    // Check for duplicate property names.
    +    checkDuplicateKeys(properties, ctx)
    +    val props = properties.toMap
    +    val badKeys = props.filter { case (_, v) => v == null }.keys
    --- End diff --
    
    NIT (not your code): `val badKeys = props.collect { case (key, null) => key }`


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types as option in ...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    **[Test build #60032 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/60032/consoleFull)** for PR 13517 at commit [`0c692e9`](https://github.com/apache/spark/commit/0c692e9032c57af002878c0a9be5f8cc85b369d2).


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request #13517: [SPARK-14839][SQL] Support for other types as opt...

Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/13517#discussion_r65834648
  
    --- Diff: python/pyspark/sql/readwriter.py ---
    @@ -180,6 +180,9 @@ def json(self, path, schema=None, primitivesAsString=None, prefersDecimal=None,
             :param path: string represents path to the JSON dataset,
                          or RDD of Strings storing JSON objects.
             :param schema: an optional :class:`StructType` for the input schema.
    +        :param samplingRatio: sets the ratio for sampling and reading the input data to infer
    --- End diff --
    
    it was actually intentional that samplingRatio was undocumented, because regardless the value, Spark still needs to read all the data so this might as well be 1 all the time.



---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types for `tablePro...

Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    NP :)


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types as option in ...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    **[Test build #60090 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/60090/consoleFull)** for PR 13517 at commit [`eb0031a`](https://github.com/apache/spark/commit/eb0031af138ed40a8ede9329e10d5fe0ea7e7e8c).


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types as option in ...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    **[Test build #60092 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/60092/consoleFull)** for PR 13517 at commit [`eb0031a`](https://github.com/apache/spark/commit/eb0031af138ed40a8ede9329e10d5fe0ea7e7e8c).
     * 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types as option in ...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    **[Test build #60090 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/60090/consoleFull)** for PR 13517 at commit [`eb0031a`](https://github.com/apache/spark/commit/eb0031af138ed40a8ede9329e10d5fe0ea7e7e8c).
     * This patch **fails Spark unit 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request #13517: [SPARK-14839][SQL] Support for other types as opt...

Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:

    https://github.com/apache/spark/pull/13517#discussion_r68869270
  
    --- Diff: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 ---
    @@ -45,11 +45,11 @@ statement
         | ALTER DATABASE identifier SET DBPROPERTIES tablePropertyList     #setDatabaseProperties
         | DROP DATABASE (IF EXISTS)? identifier (RESTRICT | CASCADE)?      #dropDatabase
         | createTableHeader ('(' colTypeList ')')? tableProvider
    -        (OPTIONS tablePropertyList)?
    +        (OPTIONS optionParameterList)?
             (PARTITIONED BY partitionColumnNames=identifierList)?
             bucketSpec?                                                    #createTableUsing
         | createTableHeader tableProvider
    -        (OPTIONS tablePropertyList)?
    --- End diff --
    
    I hope I understood this correctly. I guess you meant the `tableProperty` rule, for example,  as below:
    
    ```antlr
    tableProperty
        : key=tablePropertyKey (EQ? value= optionValue)?
        ;
    ```
    
    If so, I am worried if this affects other rules such as `DBPROPERTIES` and `TBLPROPERTIES` (allowing other types as values). I made this separate because it seems allowing other types in `OPTIONS` clause complies standard SQL.
    
    If not, could you give me an advice in a bit more detail please?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types for `tablePro...

Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    Looks pretty good. Left one test related comment.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types for `tablePro...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types for `tablePro...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    **[Test build #61698 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/61698/consoleFull)** for PR 13517 at commit [`30dfea0`](https://github.com/apache/spark/commit/30dfea05bb0ce864a7ccb5fe6a2d091c7fe3c988).
     * 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types for `tablePro...

Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    Oh....... sorry... and thanks..


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types as option in ...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/60092/
    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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types for `tablePro...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    **[Test build #61699 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/61699/consoleFull)** for PR 13517 at commit [`1307f8c`](https://github.com/apache/spark/commit/1307f8cbdd4b26885a81ad6e5770c2bb82a0159e).
     * 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types as option in ...

Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    **[Test build #60004 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/60004/consoleFull)** for PR 13517 at commit [`905bdc5`](https://github.com/apache/spark/commit/905bdc594ebbb658d9392585af3a0ab7206841e1).
     * 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types as option in ...

Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/60004/
    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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types as option in ...

Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types for `tablePro...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    **[Test build #61672 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/61672/consoleFull)** for PR 13517 at commit [`4b67bab`](https://github.com/apache/spark/commit/4b67bab4b8fc663284ac29b1e2b83ad75eb2ba74).
     * 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types as option in ...

Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    Please let me cc @davies. Thanks!


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types for `tablePro...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/61699/
    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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types as option in ...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    Merged build finished. Test FAILed.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types as option in ...

Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    cc @hvanhovell for this one


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types as option in ...

Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    **[Test build #60004 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/60004/consoleFull)** for PR 13517 at commit [`905bdc5`](https://github.com/apache/spark/commit/905bdc594ebbb658d9392585af3a0ab7206841e1).


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types for `tablePro...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/61698/
    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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request #13517: [SPARK-14839][SQL] Support for other types as opt...

Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on a diff in the pull request:

    https://github.com/apache/spark/pull/13517#discussion_r69380919
  
    --- Diff: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 ---
    @@ -45,11 +45,11 @@ statement
         | ALTER DATABASE identifier SET DBPROPERTIES tablePropertyList     #setDatabaseProperties
         | DROP DATABASE (IF EXISTS)? identifier (RESTRICT | CASCADE)?      #dropDatabase
         | createTableHeader ('(' colTypeList ')')? tableProvider
    -        (OPTIONS tablePropertyList)?
    +        (OPTIONS optionParameterList)?
             (PARTITIONED BY partitionColumnNames=identifierList)?
             bucketSpec?                                                    #createTableUsing
         | createTableHeader tableProvider
    -        (OPTIONS tablePropertyList)?
    --- End diff --
    
    @HyukjinKwon I am on holiday - so I am bit slow with my responses.
    
    Yo have understood me correctly. What I am suggesting will affect the DBPROPERTIES and TBLPROPERTIES; it will also allow for boolean and numeric options. I don't think this is a bad thing, it is better to have a lenient parser and to constrain behavior in the `AstBuilder` (this allows us to throw much better error messages).


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types as option in ...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/60090/
    Test FAILed.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types as option in ...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types for `tablePro...

Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    @HyukjinKwon still on holiday...
    
    LGTM - merging to master. Thanks!


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request #13517: [SPARK-14839][SQL] Support for other types as opt...

Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:

    https://github.com/apache/spark/pull/13517#discussion_r68871740
  
    --- Diff: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 ---
    @@ -45,11 +45,11 @@ statement
         | ALTER DATABASE identifier SET DBPROPERTIES tablePropertyList     #setDatabaseProperties
         | DROP DATABASE (IF EXISTS)? identifier (RESTRICT | CASCADE)?      #dropDatabase
         | createTableHeader ('(' colTypeList ')')? tableProvider
    -        (OPTIONS tablePropertyList)?
    +        (OPTIONS optionParameterList)?
             (PARTITIONED BY partitionColumnNames=identifierList)?
             bucketSpec?                                                    #createTableUsing
         | createTableHeader tableProvider
    -        (OPTIONS tablePropertyList)?
    --- End diff --
    
    Ah, do you mean it is okay to support other types for other related rules?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types for `tablePro...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/61672/
    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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types for `tablePro...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    **[Test build #61699 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/61699/consoleFull)** for PR 13517 at commit [`1307f8c`](https://github.com/apache/spark/commit/1307f8cbdd4b26885a81ad6e5770c2bb82a0159e).


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #13517: [SPARK-14839][SQL] Support for other types for `tablePro...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/13517
  
    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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request #13517: [SPARK-14839][SQL] Support for other types for `t...

Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on a diff in the pull request:

    https://github.com/apache/spark/pull/13517#discussion_r69392182
  
    --- Diff: sql/hive/src/test/scala/org/apache/spark/sql/hive/MetastoreDataSourcesSuite.scala ---
    @@ -1117,4 +1117,26 @@ class MetastoreDataSourcesSuite extends QueryTest with SQLTestUtils with TestHiv
           }
         }
       }
    +
    +  test("SPARK-14839: Support for other types as option in OPTIONS clause") {
    --- End diff --
    
    Could you move this test to `DDLCommandSuite`? Could you also add a test for `TBLPROPERTIES`/`DBPROPERTIES`?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request #13517: [SPARK-14839][SQL] Support for other types as opt...

Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:

    https://github.com/apache/spark/pull/13517#discussion_r65991481
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala ---
    @@ -435,6 +434,37 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder {
       }
     
       /**
    +   * Parse a key-value map from a [[OptionParameterListContext]], assuming all values are
    +   * specified. This allows string, boolean, decimal and integer literals which are converted
    +   * to strings.
    +   */
    +  override def visitOptionParameterList(ctx: OptionParameterListContext): Map[String, String] = {
    +    // TODO: Currently it does not treat null. Hive does not allow null for metadata and
    +    // throws an exception.
    +    val properties = ctx.optionParameter.asScala.map { property =>
    +      val key = visitTablePropertyKey(property.key)
    +      val value = if (property.value.STRING != null) {
    +        string(property.value.STRING)
    +      } else if (property.value.booleanValue != null) {
    +        property.value.getText.toLowerCase
    +      } else {
    +        property.value.getText
    +      }
    +      key -> value
    +    }
    +
    +    // Check for duplicate property names.
    +    checkDuplicateKeys(properties, ctx)
    +    val props = properties.toMap
    +    val badKeys = props.filter { case (_, v) => v == null }.keys
    +    if (badKeys.nonEmpty) {
    +      throw operationNotAllowed(
    +        s"Values should not be specified for key(s): ${badKeys.mkString("[", ",", "]")}", ctx)
    --- End diff --
    
    Thanks! I just fixed.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request #13517: [SPARK-14839][SQL] Support for other types as opt...

Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:

    https://github.com/apache/spark/pull/13517#discussion_r65835938
  
    --- Diff: python/pyspark/sql/readwriter.py ---
    @@ -180,6 +180,9 @@ def json(self, path, schema=None, primitivesAsString=None, prefersDecimal=None,
             :param path: string represents path to the JSON dataset,
                          or RDD of Strings storing JSON objects.
             :param schema: an optional :class:`StructType` for the input schema.
    +        :param samplingRatio: sets the ratio for sampling and reading the input data to infer
    --- End diff --
    
    Ah, I see. It does not affect the actual I/O but just drops some and then try to infer the schema. 
    I will remove the change.
    
    BTW, actually, I have found another one [`mergeSchema`](https://github.com/apache/spark/blob/431542765785304edb76a19885fbc5f9b8ae7d64/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetFileFormat.scala#L148-L152) option in Parquet data source, which I guess should be located in `ParquetOptions`. Can this be done here together maybe..?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org