[SPARK-44059][SQL] Add better error messages for SQL named argumnts by learningchess2003 · Pull Request #42177 · apache/spark

learningchess2003 · 2023-07-26T22:17:36Z

What changes were proposed in this pull request?

Correct error messages.

Why are the changes needed?

Need to have better quality messages.

Does this PR introduce any user-facing change?

Error messages are more specific.

How was this patch tested?

Tested in SQLQueryTestSuite: named-function-arguments.sql and NamedParameterFunctionSuite.

Authored-by: Richard Yu richard.yu@databricks.com
(cherry picked from commit 228b5db)

dtenedor · 2023-07-27T18:38:17Z

common/utils/src/main/resources/error/error-classes.json

      "BOTH_POSITIONAL_AND_NAMED" : {
        "message" : [
-          "A positional argument and named argument both referred to the same parameter."
+          "A positional argument and named argument both referred to the same parameter. Please remove the named argument referring to this parameter."


These error message changes are specific to SQL named arguments, can you please update the PR title and description to indicate that?

allisonwang-db · 2023-07-27T19:28:12Z

@learningchess2003 There is a LLM based tool to improve error messages: #41711 If possible could you test it out with your PR?

learningchess2003 · 2023-07-28T21:15:05Z

@allisonwang-db Tried the Chat GPT tool. The error messages were basically the same. So, I kept this one.

anchovYu · 2023-08-01T17:22:04Z

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

    val namedParametersSet = collection.mutable.Set[String]()

-    for (arg <- namedArgs) {
+    namedArgs.zipWithIndex.foreach { tuple =>


Suggested change

namedArgs.zipWithIndex.foreach { tuple =>

namedArgs.zipWithIndex.foreach { case (arg, index) =>

anchovYu · 2023-08-01T17:23:43Z

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


    // We rearrange named arguments to match their positional order.
-    val rearrangedNamedArgs: Seq[Expression] = namedParameters.map { param =>
+    val rearrangedNamedArgs: Seq[Expression] = namedParameters.zipWithIndex.map { tuple =>


anchovYu · 2023-08-01T17:26:49Z

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala

  }

-  def unexpectedPositionalArgument(functionName: String): Throwable = {
+  def unexpectedPositionalArgument(functionName: String, parameterName: String): Throwable = {


Can we rename parameterName to something like last or preceding named argument?

common/utils/src/main/resources/error/error-classes.json

Co-authored-by: Xinyi <xyyu15@gmail.com>

anchovYu · 2023-08-01T17:47:32Z

python/pyspark/worker.py

                return result

-            return lambda *a: map(lambda res: (res, arrow_return_type), map(verify_result, f(*a)))
+            return lambda *a: map(lambda res: (res, arrow_return_type), map(verify_result, f(*a, **kw)))


Revert this change?

anchovYu · 2023-08-01T17:47:54Z

docs/sql-error-conditions.md

 [SQLSTATE: 4274K](sql-error-conditions-sqlstates.html#class-42-syntax-error-or-access-rule-violation)

-Cannot invoke function `<functionName>` because the parameter named `<parameterName>` is required, but the function call did not supply a value. Please update the function call to supply an argument value (either positionally or by name) and retry the query again.
+Cannot invoke function `<functionName>` because the parameter named `<parameterName>` is required at index `<index>`, but the function call did not supply a value. Please update the function call to supply an argument value (either positionally or by name) and retry the query again.


This should be updated as well

learningchess2003 · 2023-08-01T17:57:34Z

@anchovYu Fixed!

anchovYu

LGTM!

learningchess2003 · 2023-08-02T20:38:04Z

@MaxGekk All tests are green! Can you merge it when you have time?

allisonwang-db

Can you add [SQL] in the PR title as well?

dtenedor · 2023-08-02T22:49:26Z

Hi @MaxGekk @HyukjinKwon the tests for this PR are passing, if everything looks good to you, would one of you mind to merge this PR for us? Thanks :) 🙏

HyukjinKwon · 2023-08-03T03:06:57Z

Merged to master and branch-3.5.

### What changes were proposed in this pull request? Correct error messages. ### Why are the changes needed? Need to have better quality messages. ### Does this PR introduce _any_ user-facing change? Error messages are more specific. ### How was this patch tested? Tested in SQLQueryTestSuite: named-function-arguments.sql and NamedParameterFunctionSuite. Authored-by: Richard Yu <richard.yudatabricks.com> (cherry picked from commit 228b5db) Closes #42177 from learningchess2003/error-messages. Lead-authored-by: Richard Yu <richard.yu@databricks.com> Co-authored-by: Richard Yu <134337791+learningchess2003@users.noreply.github.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org> (cherry picked from commit b390c9c) Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

error-messages

e1eb98c

github-actions bot added the SQL label Jul 26, 2023

dtenedor approved these changes Jul 27, 2023

View reviewed changes

learningchess2003 changed the title ~~[SPARK-44059] Add better error messages~~ [SPARK-44059] Add better error messages for SQL named argumnts Jul 27, 2023

learningchess2003 added 2 commits July 27, 2023 15:08

Add lines

3124806

Remove file

1e1c196

HyukjinKwon approved these changes Jul 28, 2023

View reviewed changes

Regenerate documentation

e6ba64d

github-actions bot added the DOCS label Jul 28, 2023

Redo stuff

e3aa231

allisonwang-db approved these changes Jul 28, 2023

View reviewed changes

anchovYu reviewed Aug 1, 2023

View reviewed changes

learningchess2003 and others added 2 commits August 1, 2023 10:40

Update common/utils/src/main/resources/error/error-classes.json

e5a616f

Co-authored-by: Xinyi <xyyu15@gmail.com>

Adding changes

dd0934c

github-actions bot added CORE PYTHON labels Aug 1, 2023

learningchess2003 requested a review from anchovYu August 1, 2023 17:45

anchovYu reviewed Aug 1, 2023

View reviewed changes

doc generation

35f50f3

github-actions bot removed CORE PYTHON labels Aug 1, 2023

srielau approved these changes Aug 1, 2023

View reviewed changes

anchovYu approved these changes Aug 1, 2023

View reviewed changes

MaxGekk approved these changes Aug 1, 2023

View reviewed changes

learningchess2003 added 2 commits August 2, 2023 09:10

Line

e74b305

Temporarily change mods

efcf120

github-actions bot added the INFRA label Aug 2, 2023

Attempt

b04547f

github-actions bot removed the INFRA label Aug 2, 2023

Try

6dccb53

github-actions bot added the INFRA label Aug 2, 2023

whatever

bd1bc81

github-actions bot removed the INFRA label Aug 2, 2023

allisonwang-db approved these changes Aug 2, 2023

View reviewed changes

learningchess2003 changed the title ~~[SPARK-44059] Add better error messages for SQL named argumnts~~ [SPARK-44059][SQL] Add better error messages for SQL named argumnts Aug 2, 2023

HyukjinKwon closed this in b390c9c Aug 3, 2023

	namedArgs.zipWithIndex.foreach { tuple =>
	namedArgs.zipWithIndex.foreach { case (arg, index) =>

Conversation

learningchess2003 commented Jul 26, 2023

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?

Uh oh!

dtenedor Jul 27, 2023

Choose a reason for hiding this comment

Uh oh!

allisonwang-db commented Jul 27, 2023

Uh oh!

learningchess2003 commented Jul 28, 2023

Uh oh!

anchovYu Aug 1, 2023

Choose a reason for hiding this comment

Uh oh!

anchovYu Aug 1, 2023

Choose a reason for hiding this comment

Uh oh!

anchovYu Aug 1, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

anchovYu Aug 1, 2023

Choose a reason for hiding this comment

Uh oh!

anchovYu Aug 1, 2023

Choose a reason for hiding this comment

Uh oh!

learningchess2003 commented Aug 1, 2023

Uh oh!

anchovYu left a comment

Choose a reason for hiding this comment

Uh oh!

learningchess2003 commented Aug 2, 2023

Uh oh!

allisonwang-db left a comment

Choose a reason for hiding this comment

Uh oh!

dtenedor commented Aug 2, 2023

Uh oh!

HyukjinKwon commented Aug 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants