[SPARK-43922] Add named parameter support in parser for function calls #41429

learningchess2003 · 2023-06-01T22:39:49Z

What changes were proposed in this pull request?

We plan on adding two new tokens called namedArgumentExpression and functionArgument which would enable this feature. When parsing this logic, we also make changes to ASTBuilder such that it can detect if the argument passed is a named argument or a positional one.

Here is the link for the design document:
https://docs.google.com/document/d/1uOTX0MICxqu8fNanIsiyB8FV68CceGGpa8BJLP2u9o4/edit

Why are the changes needed?

This is part of a larger project to implement named parameter support for user defined functions, built-in functions, and table valued functions.

Does this PR introduce any user-facing change?

Yes, the user would be able to call functions with argument lists that contain named arguments.

How was this patch tested?

We add tests in the PlanParserSuite that will verify that the plan parsed is as intended.

anchovYu

Thanks for implementing it! Except the ones Daniel have posted, add one nit comment.

...alyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/NamedArgumentExpression.scala

anchovYu · 2023-06-12T17:26:16Z

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

+      if (e.namedArgumentExpression != null) {
+        val key = e.namedArgumentExpression.key.strictIdentifier
+        val value = e.namedArgumentExpression.value
+        NamedArgumentExpression(key.getText, expression(value))


…essions/NamedArgumentExpression.scala Co-authored-by: Xinyi <xyyu15@gmail.com>

dtenedor

The implementation generally LGTM, I tried to come up with testing ideas.

dtenedor · 2023-06-12T18:57:58Z

...alyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/NamedArgumentExpression.scala

+
+  override def dataType: DataType = value.dataType
+
+  override def toString: String = s"""$key => $value"""


Suggested change

override def toString: String = s"""$key => $value"""

override def toString: String = s"$key => $value"

learningchess2003 · 2023-06-12T21:56:20Z

@dtenedor Thanks for the comments! I've added the tests.

dtenedor · 2023-06-12T21:59:02Z

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

@@ -921,7 +930,7 @@ primaryExpression
    | LEFT_PAREN namedExpression (COMMA namedExpression)+ RIGHT_PAREN                          #rowConstructor
    | LEFT_PAREN query RIGHT_PAREN                                                             #subqueryExpression
    | IDENTIFIER_KW LEFT_PAREN expression RIGHT_PAREN                                          #identifierClause
-    | functionName LEFT_PAREN (setQuantifier? argument+=expression (COMMA argument+=expression)*)? RIGHT_PAREN
+    | functionName LEFT_PAREN (setQuantifier? argument+=functionArgument (COMMA argument+=functionArgument)*)? RIGHT_PAREN


can you maybe fix formatting of these three lines so they break before the 94th column that the above labeks (e.g. #identifierClause) start on? This could make the parser logic easier to read.

...alyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/NamedArgumentExpression.scala

dtenedor · 2023-06-12T22:04:28Z

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

@@ -1544,8 +1544,17 @@ class AstBuilder extends SqlBaseParserBaseVisitor[AnyRef] with SQLConfHelper wit
        if (name.length > 1) {
          throw QueryParsingErrors.invalidTableValuedFunctionNameError(name, ctx)
        }
+        val args = func.functionArgument.asScala.map { e =>
+          if (e.namedArgumentExpression != null) {


You can simplify using Option and map:

Option(e.namedArgumentExpression).map { n => NamedArgumentExpression(n.get.getText, expression(nvalue)) }.getOrElse { expression(e) }

Same on L2188 below.

dtenedor · 2023-06-12T22:07:02Z

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

+          if (e.namedArgumentExpression != null) {
+            val key = e.namedArgumentExpression.key.getText
+            val value = e.namedArgumentExpression.value
+            NamedArgumentExpression(key, expression(value))


Can you control this behavior with a SQLConf? You can create a new entry like ALLOW_NAMED_FUNCTION_ARGUMENTS and return an error here if the config is not set to true:

https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

…essions/NamedArgumentExpression.scala Co-authored-by: Daniel Tenedorio <daniel.tenedorio@databricks.com>

learningchess2003 · 2023-06-12T23:25:01Z

@dtenedor The configuration has been added. Other comments has been addressed.

dtenedor · 2023-06-12T23:29:21Z

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

+              NamedArgumentExpression(n.key.getText, expression(n.value))
+            } else {
+              throw new ParseException(
+                errorClass = "Named arguments not enabled.",


This is not quite how the error classes work :) instead you need to:

Add a new entry in error-classes.json [1]

Add a corresponding function in QueryCompilationErrors.scala [2]

Call that function here like throw QueryCompilationErrors.namedArgumentsNotEnabled(n.key.getText)

Exercise this behavior in a unit test to make sure we really get this error message if the configuration is not enabled.

[1] https://github.com/apache/spark/blob/master/core/src/main/resources/error/error-classes.json#L1326

[2] https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala

core/src/main/resources/error/error-classes.json

sql/core/src/test/scala/org/apache/spark/sql/errors/QueryParsingErrorsSuite.scala

gengliangwang · 2023-06-14T04:06:53Z

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

@@ -862,6 +862,15 @@ expression
    : booleanExpression
    ;

+namedArgumentExpression
+    : key=identifier FAT_ARROW value=expression


identifier can be quoted and unquoted
Shall we make it IDENTIFIER(unquoted) only?

@gengliangwang After looking at the design doc, would it be fine to keep it like this?

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

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/ExpressionParserSuite.scala

core/src/main/resources/error/error-classes.json

Co-authored-by: Maxim Gekk <max.gekk@gmail.com>

learningchess2003 · 2023-06-27T17:05:38Z

@MaxGekk Sorry about the confusion. I realized your suggestion for quoting the spark.sql.allowNamedFunctionArguments config was accurate. That was my mistake. I committed your suggestion. Let me know what you think!

cloud-fan · 2023-06-27T21:34:23Z

...alyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/NamedArgumentExpression.scala

+ */
+case class NamedArgumentExpression(key: String, value: Expression)
+  extends UnaryExpression with Unevaluable {
+  override def nullable: Boolean = value.nullable


nit: we can remove this line as UnaryExpression already provides an implementation of nullable

cloud-fan · 2023-06-27T21:35:54Z

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

@@ -1527,6 +1527,18 @@ class AstBuilder extends SqlBaseParserBaseVisitor[AnyRef] with SQLConfHelper wit
    }
  }

+  private def extractExpression(expr: FunctionArgumentContext, funcName: String) : Expression = {


how about extractFuncArgument?

cloud-fan · 2023-06-27T21:38:02Z

sql/core/src/test/scala/org/apache/spark/sql/errors/QueryExecutionErrorsSuite.scala

@@ -875,6 +875,18 @@ class QueryExecutionErrorsSuite
      sqlState = "XX000")
  }

+  test("INTERNAL_ERROR: Calling eval on Unevaluable NamedArgumentExpression") {


I don't think we need to test it. Unevaluable is not added by this PR and has been there for a long time.

learningchess2003 · 2023-06-27T21:49:10Z

@cloud-fan Thanks for the review! Addressed the comments.

MaxGekk

@learningchess2003 Are there any end-to-end tests? If there are some, could you point them out. If not, could you add something to sql/core/src/test/resources/sql-tests/inputs

learningchess2003 · 2023-06-28T22:16:14Z

@MaxGekk Sounds good. I've added the end-to-end tests. In the next PR after this, most of the error messages will be replaced with the intended results. Right now, nothing is really supported yet.

learningchess2003 · 2023-06-29T21:08:03Z

I messed up my Git structure with the fork, so migrating to another PR #41796. @MaxGekk just to let you know.

[SPARK-43922] Add named parameter support in parser for function calls

c7215fb

github-actions bot added the SQL label Jun 1, 2023

learningchess2003 and others added 4 commits June 6, 2023 11:27

Adding some compilation fixes

5e168de

Adding tests

e044d14

Adding table valued function tests

6e44c55

Merge branch 'master' into SPARK-43922

246d5c1

anchovYu reviewed Jun 12, 2023

View reviewed changes

...alyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/NamedArgumentExpression.scala Show resolved Hide resolved

anchovYu reviewed Jun 12, 2023

View reviewed changes

learningchess2003 and others added 6 commits June 12, 2023 10:39

Adding error analysis test

20b9b93

Addressing some comments

12c814b

Removing file

95b3f20

Update sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expr…

8312980

…essions/NamedArgumentExpression.scala Co-authored-by: Xinyi <xyyu15@gmail.com>

Adding some minor comments

30a72e9

Comment

515d153

dtenedor reviewed Jun 12, 2023

View reviewed changes

Adding suggested tests

7a8f4ad

dtenedor reviewed Jun 12, 2023

View reviewed changes

learningchess2003 and others added 3 commits June 12, 2023 16:14

Update sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expr…

23f9c98

…essions/NamedArgumentExpression.scala Co-authored-by: Daniel Tenedorio <daniel.tenedorio@databricks.com>

Update sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expr…

f53dab7

…essions/NamedArgumentExpression.scala Co-authored-by: Daniel Tenedorio <daniel.tenedorio@databricks.com>

Some changes

b26fd43

dtenedor reviewed Jun 12, 2023

View reviewed changes

Adding test and error

3ae0ee6

github-actions bot added the CORE label Jun 13, 2023

learningchess2003 added 2 commits June 13, 2023 09:19

Making conversion

4eff719

Updating test to reflect error spec

4038773

anchovYu reviewed Jun 13, 2023

View reviewed changes

core/src/main/resources/error/error-classes.json Outdated Show resolved Hide resolved

sql/core/src/test/scala/org/apache/spark/sql/errors/QueryParsingErrorsSuite.scala Show resolved Hide resolved

gengliangwang reviewed Jun 14, 2023

View reviewed changes

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

learningchess2003 added 2 commits June 22, 2023 12:43

Fixing scalastyle rebase error

9a5f74e

Fixing some problems

566c994

learningchess2003 requested review from MaxGekk and gengliangwang June 26, 2023 16:54

MaxGekk requested changes Jun 27, 2023

View reviewed changes

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/ExpressionParserSuite.scala Outdated Show resolved Hide resolved

core/src/main/resources/error/error-classes.json Outdated Show resolved Hide resolved

Fixing indentation

65d9d95

learningchess2003 requested a review from MaxGekk June 27, 2023 17:00

Update core/src/main/resources/error/error-classes.json

7653225

Co-authored-by: Maxim Gekk <max.gekk@gmail.com>

cloud-fan reviewed Jun 27, 2023

View reviewed changes

cloud-fan approved these changes Jun 27, 2023

View reviewed changes

Addressing comments

f071073

MaxGekk requested changes Jun 28, 2023

View reviewed changes

Add golden file for end-to-end tests

7d99b58

learningchess2003 requested a review from MaxGekk June 28, 2023 22:15

learningchess2003 added 3 commits June 28, 2023 15:18

Remove unnecessary line

345b96d

Line deletion

704dcad

Reverting error class change

01c3af9

github-actions bot removed the CORE label Jun 29, 2023

Try

c6ca3bd

github-actions bot added the CORE label Jun 29, 2023

learningchess2003 added 2 commits June 29, 2023 13:12

Moving stuff around

da49eed

Original

5735732

learningchess2003 closed this Jun 29, 2023

dtenedor mentioned this pull request Jun 29, 2023

[SPARK-43922][SQL] Add named parameter support in parser for function calls #41796

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-43922] Add named parameter support in parser for function calls #41429

[SPARK-43922] Add named parameter support in parser for function calls #41429

learningchess2003 commented Jun 1, 2023 •

edited

Loading

anchovYu left a comment •

edited

Loading

anchovYu Jun 12, 2023

dtenedor left a comment

dtenedor Jun 12, 2023

learningchess2003 commented Jun 12, 2023

dtenedor Jun 12, 2023

dtenedor Jun 12, 2023

dtenedor Jun 12, 2023

learningchess2003 commented Jun 12, 2023

dtenedor Jun 12, 2023

gengliangwang Jun 14, 2023

learningchess2003 Jun 14, 2023

learningchess2003 commented Jun 27, 2023

cloud-fan Jun 27, 2023

cloud-fan Jun 27, 2023 •

edited

Loading

cloud-fan Jun 27, 2023

learningchess2003 commented Jun 27, 2023

MaxGekk left a comment

learningchess2003 commented Jun 28, 2023

learningchess2003 commented Jun 29, 2023


		override def dataType: DataType = value.dataType

		override def toString: String = s"""$key => $value"""

	override def toString: String = s"""$key => $value"""
	override def toString: String = s"$key => $value"

[SPARK-43922] Add named parameter support in parser for function calls #41429

[SPARK-43922] Add named parameter support in parser for function calls #41429

Conversation

learningchess2003 commented Jun 1, 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?

anchovYu left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtenedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

learningchess2003 commented Jun 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

learningchess2003 commented Jun 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

learningchess2003 commented Jun 27, 2023

Choose a reason for hiding this comment

cloud-fan Jun 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

learningchess2003 commented Jun 27, 2023

MaxGekk left a comment

Choose a reason for hiding this comment

learningchess2003 commented Jun 28, 2023

learningchess2003 commented Jun 29, 2023

learningchess2003 commented Jun 1, 2023 •

edited

Loading

anchovYu left a comment •

edited

Loading

cloud-fan Jun 27, 2023 •

edited

Loading