[SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values #41191

dtenedor · 2023-05-16T22:29:04Z

What changes were proposed in this pull request?

This PR updates the SQL compiler to support general constnat expressions in the syntax for CREATE/REPLACE TABLE OPTIONS values, rather than restricting to a few types of literals only.

The analyzer now checks that the provided expressions are in fact foldable, and throws an error message otherwise.
This error message that users encounter in these cases improves from a general "syntax error at or near " to instead indicate that the syntax is valid, but only constant expressions are supported in these contexts.

Why are the changes needed?

This makes it easier to provide OPTIONS lists in SQL, supporting use cases like concatenating strings with ||.

Does this PR introduce any user-facing change?

Yes, the SQL syntax changes.

How was this patch tested?

This PR adds new unit test coverage.

commit

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

dtenedor · 2023-05-18T16:43:27Z

Note @MaxGekk @gengliangwang it says the GitHub actions CI failed, but actually this was just an unrelated PySpark flake and they actually passed :)

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

dtenedor

Thanks @gengliangwang for your review! Please take another look.

dtenedor

Thanks @gengliangwang for your review! Please take another look.

dtenedor · 2023-05-20T21:19:02Z

Update on this: I thought of a simpler way to break apart this work into smaller pieces so we can make gradual improvements. Let me make a commit and then I can ping this thread again.

move constant folding to analysis

dtenedor · 2023-05-22T19:37:48Z

This is done, I have deferred the constant folding logic to the analyzer. This is ready for another look.

dtenedor

Hi @gengliangwang, thanks again for your reviews, this is ready again :)

gengliangwang · 2023-06-07T07:12:02Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveTableSpec.scala

+            val dt = value.dataType
+            value match {
+              case Literal(null, _) =>
+                "null"


should it be null or "null"?

It should be "null", this part computes the string value for the table option.

But string can be null too. If it is "null", then the literal should be Literal("null", StringType)

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala

dtenedor

Hi @gengliangwang I responded to your comments. I think this is ready to merge now, if you agree.

dtenedor · 2023-06-07T16:24:44Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveTableSpec.scala

+            val dt = value.dataType
+            value match {
+              case Literal(null, _) =>
+                "null"


It should be "null", this part computes the string value for the table option.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala

fetch from master

dtenedor · 2023-06-07T17:49:14Z

The main commit responding to the last round of code review comments is here: 65f9a92.

dtenedor · 2023-06-08T23:01:53Z

(Note, this is passing all CI again.)

gengliangwang · 2023-06-09T07:27:53Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveTableSpec.scala

+            val dt = value.dataType
+            value match {
+              case Literal(null, _) =>
+                "null"


Suggested change

"null"

null

Sounds good, done.

gengliangwang

LGTM except for the last comment. Sorry for the slow response, I was focusing on another project.

dtenedor · 2023-06-09T23:43:38Z

LGTM except for the last comment. Sorry for the slow response, I was focusing on another project.

No worries, thanks for the careful reviews :)

gengliangwang · 2023-06-12T04:30:44Z

This has been iterated for many rounds. I will make a follow-up PR to further make the code simpler.
Merging to master.

…related plans ### What changes were proposed in this pull request? Follow-up of #41191 to clean up the code in UnresolvedTableSpec and related plans: * Rename `OptionsListExpressions` as `OptionList` * Rename `trait TableSpec` as `TableSpecBase` * Rename `ResolvedTableSpec` as `TableSpec`, make sure all the physical plans are using `TableSpec` instead of `TableSpecBase`. * Move option list expressions to UnresolvedTableSpec, so that all the specs are in one class. * Make UnaryExpression an `UnaryExpression`, so that transforming with `mapExpressions` will transform it and the option list expressions in its child * Restore the signatures of class `CreateTable`, `CreateTableAsSelect`, `ReplaceTable` and `ReplaceTableAsSelect` ### Why are the changes needed? Make the code implementation simpler ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Existing tests Closes #41549 from gengliangwang/optionsFollowUp. Authored-by: Gengliang Wang <gengliang@apache.org> Signed-off-by: Gengliang Wang <gengliang@apache.org>

…related plans ### What changes were proposed in this pull request? Follow-up of apache/spark#41191 to clean up the code in UnresolvedTableSpec and related plans: * Rename `OptionsListExpressions` as `OptionList` * Rename `trait TableSpec` as `TableSpecBase` * Rename `ResolvedTableSpec` as `TableSpec`, make sure all the physical plans are using `TableSpec` instead of `TableSpecBase`. * Move option list expressions to UnresolvedTableSpec, so that all the specs are in one class. * Make UnaryExpression an `UnaryExpression`, so that transforming with `mapExpressions` will transform it and the option list expressions in its child * Restore the signatures of class `CreateTable`, `CreateTableAsSelect`, `ReplaceTable` and `ReplaceTableAsSelect` ### Why are the changes needed? Make the code implementation simpler ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Existing tests Closes #41549 from gengliangwang/optionsFollowUp. Authored-by: Gengliang Wang <gengliang@apache.org> Signed-off-by: Gengliang Wang <gengliang@apache.org>

…LACE TABLE OPTIONS values ### What changes were proposed in this pull request? This PR updates the SQL compiler to support general constnat expressions in the syntax for CREATE/REPLACE TABLE OPTIONS values, rather than restricting to a few types of literals only. * The analyzer now checks that the provided expressions are in fact `foldable`, and throws an error message otherwise. * This error message that users encounter in these cases improves from a general "syntax error at or near <location>" to instead indicate that the syntax is valid, but only constant expressions are supported in these contexts. ### Why are the changes needed? This makes it easier to provide OPTIONS lists in SQL, supporting use cases like concatenating strings with `||`. ### Does this PR introduce _any_ user-facing change? Yes, the SQL syntax changes. ### How was this patch tested? This PR adds new unit test coverage. Closes apache#41191 from dtenedor/expression-properties. Authored-by: Daniel Tenedorio <daniel.tenedorio@databricks.com> Signed-off-by: Gengliang Wang <gengliang@apache.org>

…related plans ### What changes were proposed in this pull request? Follow-up of apache#41191 to clean up the code in UnresolvedTableSpec and related plans: * Rename `OptionsListExpressions` as `OptionList` * Rename `trait TableSpec` as `TableSpecBase` * Rename `ResolvedTableSpec` as `TableSpec`, make sure all the physical plans are using `TableSpec` instead of `TableSpecBase`. * Move option list expressions to UnresolvedTableSpec, so that all the specs are in one class. * Make UnaryExpression an `UnaryExpression`, so that transforming with `mapExpressions` will transform it and the option list expressions in its child * Restore the signatures of class `CreateTable`, `CreateTableAsSelect`, `ReplaceTable` and `ReplaceTableAsSelect` ### Why are the changes needed? Make the code implementation simpler ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Existing tests Closes apache#41549 from gengliangwang/optionsFollowUp. Authored-by: Gengliang Wang <gengliang@apache.org> Signed-off-by: Gengliang Wang <gengliang@apache.org>

commit

1b58181

commit

github-actions bot added the SQL label May 16, 2023

dtenedor added 5 commits May 16, 2023 15:49

fix build

0b9410d

fix tests

d9e1f68

fix tests

bdf66bd

fix tests

e62852b

fix tests

35f1363

MaxGekk reviewed May 17, 2023

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala Outdated Show resolved Hide resolved

dtenedor requested a review from MaxGekk May 17, 2023 16:47

dtenedor added 2 commits May 17, 2023 12:29

respond to code review comments

ca1f7fb

respond to code review comments

c3ee5d4

dtenedor requested a review from gengliangwang May 17, 2023 19:32

MaxGekk reviewed May 17, 2023

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala Outdated Show resolved Hide resolved

respond to code review comments

32dbbda

dtenedor requested a review from MaxGekk May 17, 2023 19:56

dtenedor changed the title ~~[SPARK-43529][SQL] Support general expressions as OPTIONS values in the parser~~ [SPARK-43529][SQL] Support general constant expressions as OPTIONS values in the parser May 17, 2023

dtenedor added 3 commits May 17, 2023 12:59

add another test

ff1cda7

add more test cases

4b8bb24

respond to code review comments

d81432e

gengliangwang reviewed May 19, 2023

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala Outdated Show resolved Hide resolved

respond to code review comments

81dcf4e

dtenedor commented May 20, 2023

View reviewed changes

dtenedor changed the title ~~[SPARK-43529][SQL] Support general constant expressions as OPTIONS values in the parser~~ [SPARK-43529][SQL] Support general literal expressions as OPTIONS values in the parser May 20, 2023

dtenedor requested a review from gengliangwang May 20, 2023 00:44

dtenedor added 2 commits May 20, 2023 20:19

move constant folding to analysis

d951f9d

move constant folding to analysis

defer constant folding to analysis

5992c8a

dtenedor commented Jun 6, 2023

View reviewed changes

This comment was marked as duplicate.

Sign in to view

gengliangwang reviewed Jun 7, 2023

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala Outdated Show resolved Hide resolved

gengliangwang reviewed Jun 7, 2023

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala Outdated Show resolved Hide resolved

respond to code review comments

65f9a92

dtenedor commented Jun 7, 2023

View reviewed changes

dtenedor added 4 commits June 7, 2023 09:48

respond to code review comments

ddbcc9d

Merge branch 'master' into expression-properties

612720f

fetch from master

respond to code review comments

f0896b7

respond to code review comments

25c7307

dtenedor requested a review from gengliangwang June 7, 2023 17:49

dtenedor added 2 commits June 7, 2023 10:53

respond to code review comments

2d95db0

respond to code review comments

8c2468d

gengliangwang reviewed Jun 9, 2023

View reviewed changes

gengliangwang approved these changes Jun 9, 2023

View reviewed changes

dtenedor added 2 commits June 9, 2023 16:21

respond to code review comments

597b122

respond to code review comments

799d178

gengliangwang closed this in 7250941 Jun 12, 2023

gengliangwang mentioned this pull request Jun 12, 2023

[SPARK-43529][SQL][FOLLOWUP] Code cleanup in UnresolvedTableSpec and related plans #41549

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values #41191

[SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values #41191

dtenedor commented May 16, 2023 •

edited

Loading

dtenedor commented May 18, 2023

dtenedor left a comment

dtenedor left a comment

dtenedor commented May 20, 2023

dtenedor commented May 22, 2023

dtenedor left a comment

This comment was marked as duplicate.

gengliangwang Jun 7, 2023

dtenedor Jun 7, 2023

gengliangwang Jun 9, 2023

dtenedor left a comment

dtenedor Jun 7, 2023

dtenedor commented Jun 7, 2023

dtenedor commented Jun 8, 2023

gengliangwang Jun 9, 2023

dtenedor Jun 9, 2023

gengliangwang left a comment

dtenedor commented Jun 9, 2023

gengliangwang commented Jun 12, 2023

[SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values #41191

[SPARK-43529][SQL] Support general constant expressions as CREATE/REPLACE TABLE OPTIONS values #41191

Conversation

dtenedor commented May 16, 2023 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

dtenedor commented May 18, 2023

dtenedor left a comment

Choose a reason for hiding this comment

dtenedor left a comment

Choose a reason for hiding this comment

dtenedor commented May 20, 2023

dtenedor commented May 22, 2023

dtenedor left a comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

gengliangwang Jun 7, 2023

Choose a reason for hiding this comment

dtenedor Jun 7, 2023

Choose a reason for hiding this comment

gengliangwang Jun 9, 2023

Choose a reason for hiding this comment

dtenedor left a comment

Choose a reason for hiding this comment

dtenedor Jun 7, 2023

Choose a reason for hiding this comment

dtenedor commented Jun 7, 2023

dtenedor commented Jun 8, 2023

gengliangwang Jun 9, 2023

Choose a reason for hiding this comment

dtenedor Jun 9, 2023

Choose a reason for hiding this comment

gengliangwang left a comment

Choose a reason for hiding this comment

dtenedor commented Jun 9, 2023

gengliangwang commented Jun 12, 2023

dtenedor commented May 16, 2023 •

edited

Loading