You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/11/15 05:39:58 UTC
[GitHub] [spark] Yaohua628 opened a new pull request, #38663: [SPARK-41143][SQL] Add named argument function syntax support
Yaohua628 opened a new pull request, #38663:
URL: https://github.com/apache/spark/pull/38663
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'core/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
Support named arguments functions in Spark SQL:
General usage: `_FUNC_(arg0, arg1, arg2, arg5 => value5, arg8 => value8)`
- Arguments can be passed positionally or by name.
- Positional arguments cannot come after a named argument.
This PR adds syntax support to named argument functions: `arg0 => value0` and adds a trait.
### Why are the changes needed?
Better UX when using the Spark SQL function
### Does this PR introduce _any_ user-facing change?
No (not in this PR, this PR only adds syntax support)
### How was this patch tested?
New UTs
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Yaohua628 closed pull request #38663: [SPARK-41143][SQL] Add named argument syntax support for table-valued function
Posted by GitBox <gi...@apache.org>.
Yaohua628 closed pull request #38663: [SPARK-41143][SQL] Add named argument syntax support for table-valued function
URL: https://github.com/apache/spark/pull/38663
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #38663: [SPARK-41143][SQL] Add named argument function syntax support
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on PR #38663:
URL: https://github.com/apache/spark/pull/38663#issuecomment-1315236539
Can one of the admins verify this patch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dtenedor commented on a diff in pull request #38663: [SPARK-41143][SQL] Add named argument function syntax support
Posted by GitBox <gi...@apache.org>.
dtenedor commented on code in PR #38663:
URL: https://github.com/apache/spark/pull/38663#discussion_r1023072531
##########
sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBaseParser.g4:
##########
@@ -769,7 +769,7 @@ inlineTable
;
functionTable
- : funcName=functionName LEFT_PAREN (expression (COMMA expression)*)? RIGHT_PAREN tableAlias
+ : funcName=functionName LEFT_PAREN (functionArgument (COMMA functionArgument)*)? RIGHT_PAREN tableAlias
Review Comment:
here in the `.g4` file, we only change the syntax for table function calls, but the PR title and description mention general named argument support. Can we either update the PR description or else update the `functionCall` rule to replace `argument+=expression` with `argument+=functionArgument`? The latter would be nice since we can make all the parser changes in the same PR.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/literals.scala:
##########
@@ -324,6 +324,21 @@ object LiteralTreeBits {
val nullLiteralBits: BitSet = new ImmutableBitSet(TreePattern.maxId, LITERAL.id, NULL_LITERAL.id)
}
+case class NamedArgumentExpression(key: String, value: Expression) extends LeafExpression {
Review Comment:
this is in `literals.scala`, but this new expression is not really a literal :) can we maybe put this into its own file with a class comment?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/NamedArgumentFunction.scala:
##########
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.catalyst.plans.logical
+
+import java.util.Locale
+
+import scala.collection.mutable
+
+import org.apache.spark.sql.catalyst.expressions.{Expression, NamedArgumentExpression}
+import org.apache.spark.sql.errors.QueryCompilationErrors.{tableFunctionDuplicateNamedArguments, tableFunctionUnexpectedArgument}
+import org.apache.spark.sql.types._
+
+/**
+ * A trait to define a named argument function:
+ * Usage: _FUNC_(arg0, arg1, arg2, arg5 => value5, arg8 => value8)
+ *
+ * - Arguments can be passed positionally or by name
+ * - Positional arguments cannot come after a named argument
Review Comment:
from the example, named arguments can come after positional arguments. write that in the comment too? also maybe we can add other constraints:
* no function call may include two arguments with the same name
* case sensitivity follows the SQLConf.CASE_SENSITIVE boolean configuration
* the function signature must specify the argument names, and the provided argument names must match the names in the function signature
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/PlanParserSuite.scala:
##########
@@ -852,6 +852,31 @@ class PlanParserSuite extends AnalysisTest {
stop = 43))
}
+ test("table valued function with named arguments") {
+ // All named arguments
+ assertEqual(
+ "select * from my_tvf(arg1 => 'value1', arg2 => true)",
+ UnresolvedTableValuedFunction("my_tvf",
+ NamedArgumentExpression("arg1", Literal("value1")) ::
+ NamedArgumentExpression("arg2", Literal(true)) :: Nil, Seq.empty).select(star()))
+
+ // Unnamed and named arguments
+ assertEqual(
+ "select * from my_tvf(2, arg1 => 'value1', arg2 => true)",
+ UnresolvedTableValuedFunction("my_tvf",
+ Literal(2) ::
+ NamedArgumentExpression("arg1", Literal("value1")) ::
+ NamedArgumentExpression("arg2", Literal(true)) :: Nil, Seq.empty).select(star()))
+
+ // Mixed arguments
+ assertEqual(
+ "select * from my_tvf(arg1 => 'value1', 2, arg2 => true)",
Review Comment:
thanks for adding these parser tests! can we also add some query tests in e.g. `SQLQuerySuite` that show what happens if we try to analyze such function calls with named arguments? do we get an error message, or does the whole feature work end-to-end?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/NamedArgumentFunction.scala:
##########
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.catalyst.plans.logical
+
+import java.util.Locale
+
+import scala.collection.mutable
+
+import org.apache.spark.sql.catalyst.expressions.{Expression, NamedArgumentExpression}
+import org.apache.spark.sql.errors.QueryCompilationErrors.{tableFunctionDuplicateNamedArguments, tableFunctionUnexpectedArgument}
+import org.apache.spark.sql.types._
+
+/**
+ * A trait to define a named argument function:
+ * Usage: _FUNC_(arg0, arg1, arg2, arg5 => value5, arg8 => value8)
+ *
+ * - Arguments can be passed positionally or by name
+ * - Positional arguments cannot come after a named argument
+ */
+trait NamedArgumentFunction {
+ /**
+ * A trait [[Param]] that is used to define function parameter
+ * - name: case insensitive name
Review Comment:
there is a SQLConf.CASE_SENSITIVE configuration that toggles case-sensitivity during SQL analysis. We should probably follow that here
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -3380,4 +3380,20 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase {
"unsupported" -> unsupported.toString,
"class" -> unsupported.getClass.toString))
}
+
+ def tableFunctionDuplicateNamedArguments(name: String, pos: Int): Throwable = {
+ new AnalysisException(
Review Comment:
can we put this in `sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryParsingErrors.scala` and then we can include the context? then the error message will have the parse location of the named argument, which will be underlined in the text output and highlighted elsewhere?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dtenedor commented on a diff in pull request #38663: [SPARK-41143][SQL] Add named argument function syntax support
Posted by GitBox <gi...@apache.org>.
dtenedor commented on code in PR #38663:
URL: https://github.com/apache/spark/pull/38663#discussion_r1023072531
##########
sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBaseParser.g4:
##########
@@ -769,7 +769,7 @@ inlineTable
;
functionTable
- : funcName=functionName LEFT_PAREN (expression (COMMA expression)*)? RIGHT_PAREN tableAlias
+ : funcName=functionName LEFT_PAREN (functionArgument (COMMA functionArgument)*)? RIGHT_PAREN tableAlias
Review Comment:
here in the `.g4` file, we only change the syntax for table function calls, but the PR title and description mention general named argument support. Can we either:
1. update the PR description, or
2. update the `functionCall` rule to replace `argument+=expression` with `argument+=functionArgument`?
Up to you, the latter would be nice since we can make all the parser changes in the same PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Yaohua628 commented on a diff in pull request #38663: [SPARK-41143][SQL] Add named argument function syntax support
Posted by GitBox <gi...@apache.org>.
Yaohua628 commented on code in PR #38663:
URL: https://github.com/apache/spark/pull/38663#discussion_r1023431618
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/PlanParserSuite.scala:
##########
@@ -852,6 +852,31 @@ class PlanParserSuite extends AnalysisTest {
stop = 43))
}
+ test("table valued function with named arguments") {
+ // All named arguments
+ assertEqual(
+ "select * from my_tvf(arg1 => 'value1', arg2 => true)",
+ UnresolvedTableValuedFunction("my_tvf",
+ NamedArgumentExpression("arg1", Literal("value1")) ::
+ NamedArgumentExpression("arg2", Literal(true)) :: Nil, Seq.empty).select(star()))
+
+ // Unnamed and named arguments
+ assertEqual(
+ "select * from my_tvf(2, arg1 => 'value1', arg2 => true)",
+ UnresolvedTableValuedFunction("my_tvf",
+ Literal(2) ::
+ NamedArgumentExpression("arg1", Literal("value1")) ::
+ NamedArgumentExpression("arg2", Literal(true)) :: Nil, Seq.empty).select(star()))
+
+ // Mixed arguments
+ assertEqual(
+ "select * from my_tvf(arg1 => 'value1', 2, arg2 => true)",
Review Comment:
Got it! Good point, added a test, it will throw:
```
org.apache.spark.sql.AnalysisException: could not resolve `my_func` to a table-valued function; line 1 pos 14;
'Project [*]
+- 'UnresolvedTableValuedFunction [my_func], [jack, age => 18]
```
And the next step, we need to resolve this `UnresolvedTableValuedFunction` plan to actual function.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Yaohua628 commented on a diff in pull request #38663: [SPARK-41143][SQL] Add named argument function syntax support
Posted by GitBox <gi...@apache.org>.
Yaohua628 commented on code in PR #38663:
URL: https://github.com/apache/spark/pull/38663#discussion_r1023428966
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/NamedArgumentFunction.scala:
##########
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.catalyst.plans.logical
+
+import java.util.Locale
+
+import scala.collection.mutable
+
+import org.apache.spark.sql.catalyst.expressions.{Expression, NamedArgumentExpression}
+import org.apache.spark.sql.errors.QueryCompilationErrors.{tableFunctionDuplicateNamedArguments, tableFunctionUnexpectedArgument}
+import org.apache.spark.sql.types._
+
+/**
+ * A trait to define a named argument function:
+ * Usage: _FUNC_(arg0, arg1, arg2, arg5 => value5, arg8 => value8)
+ *
+ * - Arguments can be passed positionally or by name
+ * - Positional arguments cannot come after a named argument
+ */
+trait NamedArgumentFunction {
+ /**
+ * A trait [[Param]] that is used to define function parameter
+ * - name: case insensitive name
Review Comment:
Thanks for your feedback!
I think function parameters (identifier) have to be case insensitive: https://spark.apache.org/docs/latest/sql-ref-identifier.html
Also, from a UX perspective, a named argument function with case-sensitive parameters will be pretty hard to use? WDYT
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Yaohua628 commented on a diff in pull request #38663: [SPARK-41143][SQL] Add named argument function syntax support
Posted by GitBox <gi...@apache.org>.
Yaohua628 commented on code in PR #38663:
URL: https://github.com/apache/spark/pull/38663#discussion_r1023434857
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -3380,4 +3380,20 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase {
"unsupported" -> unsupported.toString,
"class" -> unsupported.getClass.toString))
}
+
+ def tableFunctionDuplicateNamedArguments(name: String, pos: Int): Throwable = {
+ new AnalysisException(
Review Comment:
Thanks for the feedback!
Honestly, I think the specific error is more like a compilation error instead of a parsing error?
We can parse it correctly, there is no syntax error, it is more like failing to resolve/compile the given `UnresolvedTableValuedFunction` into an actual resolved function.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org