[SPARK-14839][SQL] Support for other types for `tableProperty` rule in SQL syntax #13517

HyukjinKwon · 2016-06-05T07:29:28Z

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 corrects tableProperty rule to support other types (string, boolean, double and integer) so that support the options for data sources in a consistent way. This will affect other rules such as DBPROPERTIES and TBLPROPERTIES (allowing other types as values).

Also, TODO add bucketing and partitioning. was removed because it was resolved in 24bea00

How was this patch tested?

Unit test in MetastoreDataSourcesSuite.scala.

HyukjinKwon · 2016-06-05T07:30:52Z

Please let me cc @davies. Thanks!

SparkQA · 2016-06-05T09:29:07Z

Test build #60004 has finished for PR 13517 at commit 905bdc5.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

rxin · 2016-06-06T03:42:33Z

python/pyspark/sql/readwriter.py

        :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


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.

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 option in Parquet data source, which I guess should be located in ParquetOptions (and this is undocumented as well). Can this be done here together maybe..?

SparkQA · 2016-06-06T11:03:36Z

Test build #60032 has finished for PR 13517 at commit 0c692e9.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

davies · 2016-06-06T21:46:23Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala

+    val props = properties.toMap
+    val badKeys = props.filter { case (_, v) => v == null }.keys
+    if (badKeys.nonEmpty) {
+      throw operationNotAllowed(


This error message does not match with the behavior.

Thanks! I just fixed.

SparkQA · 2016-06-07T01:40:13Z

Test build #60090 has finished for PR 13517 at commit eb0031a.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2016-06-07T01:42:32Z

retest this please

SparkQA · 2016-06-07T03:42:03Z

Test build #60092 has finished for PR 13517 at commit eb0031a.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2016-06-25T09:43:07Z

@davies and @rxin I addressed the comments. Thanks!

rxin · 2016-06-28T04:59:38Z

cc @hvanhovell for this one

hvanhovell · 2016-06-28T05:25:34Z

sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4

        (PARTITIONED BY partitionColumnNames=identifierList)?
        bucketSpec?                                                    #createTableUsing
    | createTableHeader tableProvider
-        (OPTIONS tablePropertyList)?


Why not generalize the tableProperty rule and use optionValue (rename it to something more consistent) as its value rule? Seems easier.

I hope I understood this correctly. I guess you meant the tableProperty rule, for example, as below:

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?

Ah, do you mean it is okay to support other types for other rules using tableProperty as well?

(Sorry for pinging @hvanhovell)

@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).

@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!

SparkQA · 2016-06-29T03:15:10Z

Test build #61429 has finished for PR 13517 at commit ea2af9b.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-07-03T06:00:33Z

Test build #61672 has finished for PR 13517 at commit 4b67bab.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

hvanhovell · 2016-07-03T14:03:32Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/MetastoreDataSourcesSuite.scala

    }
  }
+
+  test("SPARK-14839: Support for other types as option in OPTIONS clause") {


Could you move this test to DDLCommandSuite? Could you also add a test for TBLPROPERTIES/DBPROPERTIES?

hvanhovell · 2016-07-03T14:03:53Z

Looks pretty good. Left one test related comment.

SparkQA · 2016-07-04T02:35:38Z

Test build #61698 has finished for PR 13517 at commit 30dfea0.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-07-04T02:44:03Z

Test build #61699 has finished for PR 13517 at commit 1307f8c.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2016-07-07T03:49:32Z

(@hvanhovell I just addressed your comments!)

hvanhovell · 2016-07-07T03:56:15Z

@HyukjinKwon still on holiday...

LGTM - merging to master. Thanks!

HyukjinKwon · 2016-07-07T03:58:39Z

Oh....... sorry... and thanks..

hvanhovell · 2016-07-07T03:59:10Z

NP :)

HyukjinKwon added 5 commits June 5, 2016 15:26

Support for other types in option clause in SQL Syntax

ba014ee

Update commnets for a new function

1f94ca4

Add a todo

1833d4f

Add override

1e6bdd3

Change documentation for the correct explanation

905bdc5

rxin reviewed Jun 6, 2016
View reviewed changes

Remove samplingRatio change

0c692e9

davies reviewed Jun 6, 2016
View reviewed changes

Fix exception message

eb0031a

hvanhovell reviewed Jun 28, 2016
View reviewed changes

Fix style nits

ea2af9b

Generalize tablePropertyValue for tableProperty and options

4b67bab

HyukjinKwon changed the title ~~[SPARK-14839][SQL] Support for other types as option in OPTIONS clause~~ [SPARK-14839][SQL] Support for other types for tableProperty rule in SQL syntax Jul 3, 2016

hvanhovell reviewed Jul 3, 2016
View reviewed changes

HyukjinKwon added 4 commits July 4, 2016 09:52

Move existing test to DDLCommandSuite and add some more tests

0c5de5e

Fix indentation

b32979c

Consistent test variables

ed68fbc

Fix test names

30dfea0

Consistent table identifier

1307f8c

asfgit closed this in 34283de Jul 7, 2016

HyukjinKwon deleted the SPARK-14839 branch January 2, 2018 03:40

[SPARK-14839][SQL] Support for other types for tableProperty rule in SQL syntax #13517

[SPARK-14839][SQL] Support for other types for tableProperty rule in SQL syntax #13517

Uh oh!

Conversation

HyukjinKwon commented Jun 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

HyukjinKwon commented Jun 5, 2016

Uh oh!

SparkQA commented Jun 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Jun 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 7, 2016

Uh oh!

HyukjinKwon commented Jun 7, 2016

Uh oh!

SparkQA commented Jun 7, 2016

Uh oh!

HyukjinKwon commented Jun 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rxin commented Jun 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Jun 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Jun 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Jul 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 29, 2016

Uh oh!

SparkQA commented Jul 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hvanhovell commented Jul 3, 2016

Uh oh!

SparkQA commented Jul 4, 2016

Uh oh!

SparkQA commented Jul 4, 2016

Uh oh!

HyukjinKwon commented Jul 7, 2016

Uh oh!

hvanhovell commented Jul 7, 2016

Uh oh!

HyukjinKwon commented Jul 7, 2016

Uh oh!

hvanhovell commented Jul 7, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

[SPARK-14839][SQL] Support for other types for `tableProperty` rule in SQL syntax #13517

[SPARK-14839][SQL] Support for other types for `tableProperty` rule in SQL syntax #13517

HyukjinKwon commented Jun 5, 2016 •

edited

Loading

HyukjinKwon Jun 6, 2016 •

edited

Loading

HyukjinKwon commented Jun 25, 2016 •

edited

Loading

HyukjinKwon Jun 29, 2016 •

edited

Loading

HyukjinKwon Jun 29, 2016 •

edited

Loading

HyukjinKwon Jul 2, 2016 •

edited

Loading