[SPARK-48966][SQL] Improve error message with invalid unresolved column reference in UDTF call #47447

dtenedor · 2024-07-22T19:06:32Z

What changes were proposed in this pull request?

This bug covers improving an error message in the event of invalid UDTF calls. For example:

select * from udtf(
  observed => TABLE(select column from t),
  value_col => classic_dollars
)

Currently we get:

Unsupported subquery expression: Table arguments are used in a function where they are not supported:
'UnresolvedTableValuedFunction [udtf], [observed => table-argument#68918 [], value_col => 'classic_dollars, false
   +- Project ...
       +- SubqueryAlias ...
          +- Relation ...

But the real error is that the user passed column identifier classic_dollars rather than string "classic_dollars" into the string argument.

The core reason is that CheckAnalysis checks the analyzer output tree for unresolved TABLE arguments first before checking for remaining unresolved attribute references.

To fix it, we update CheckAnalysis to move the check for remaining TABLE arguments to later. Now that query returns something like:

{
  "errorClass" : "UNRESOLVED_COLUMN.WITHOUT_SUGGESTION",
  "sqlState" : "42703",
  "messageParameters" : {
    "objectName" : "`classic_dollars`"
  },
  "queryContext" : [ {
    "objectType" : "",
    "objectName" : "",
    "startIndex" : 93,
    "stopIndex" : 109,
    "fragment" : "classic_dollars"
  } ]
}

Why are the changes needed?

This improves error messages for SQL queries that are invalid, but that a user might reasonably create accidentally while figuring out how the syntax works for calling table-valued functions.

Does this PR introduce any user-facing change?

No

How was this patch tested?

This PR adds and updates golden file test coverage.

Was this patch authored or co-authored using generative AI tooling?

Just some casual, everyday GitHub CoPilot usage.

dtenedor · 2024-07-22T19:06:56Z

cc @allisonwang-db @ueshin

allisonwang-db · 2024-07-23T20:18:27Z

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

+
+        // Check for unresolved TABLE arguments after the main check above to allow other analysis
+        // errors to apply first, providing better error messages.
+        getAllExpressions(operator).foreach(_.foreachUp {


Can we check this together with getAllExpressions(operator).foreach(_.foreachUp { in line 290 so we don't need to iterate the plan again?

No, because we need to check all the expressions for "unresolved attribute" or other errors first, before we do this check for FunctionTableSubqueryArgumentExpression instances.

That code on L290 starts with this:

getAllExpressions(operator).foreach(_.foreachUp { case a: Attribute if !a.resolved => failUnresolvedAttribute(operator, a, "UNRESOLVED_COLUMN") ...

We need to make sure we perform that "unresolved attribute" check for all expressions in the plan before we proceed further to check for FunctionTableSubqueryArgumentExpression.

Ah I get it now. This is a node that should have been rewritten during analysis. cc @cloud-fan do you know if there are better places to add this?

allisonwang-db · 2024-07-23T20:18:51Z

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

      case _: FunctionTableSubqueryArgumentExpression =>
-        expr.failAnalysis(
-          errorClass = "UNSUPPORTED_SUBQUERY_EXPRESSION_CATEGORY.UNSUPPORTED_TABLE_ARGUMENT",
-          messageParameters = Map("treeNode" -> planToString(plan)))
+        // Do nothing here, since we will check for this pattern later.


Shall we just remove this case?

I originally tried this, but found that we encountered a MatchError here in the presence of the FunctionTableSubqueryArgumentExpression. Therefore I had to add this "do nothing" case back here, to allow the new code on L395-403 to check for this pattern separately later.

dtenedor

Hi @allisonwang-db thanks for your review! Responded to your comments, please take another look.

dtenedor · 2024-08-13T18:55:39Z

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

      case _: FunctionTableSubqueryArgumentExpression =>
-        expr.failAnalysis(
-          errorClass = "UNSUPPORTED_SUBQUERY_EXPRESSION_CATEGORY.UNSUPPORTED_TABLE_ARGUMENT",
-          messageParameters = Map("treeNode" -> planToString(plan)))
+        // Do nothing here, since we will check for this pattern later.


I originally tried this, but found that we encountered a MatchError here in the presence of the FunctionTableSubqueryArgumentExpression. Therefore I had to add this "do nothing" case back here, to allow the new code on L395-403 to check for this pattern separately later.

dtenedor · 2024-08-13T19:07:02Z

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

+
+        // Check for unresolved TABLE arguments after the main check above to allow other analysis
+        // errors to apply first, providing better error messages.
+        getAllExpressions(operator).foreach(_.foreachUp {


No, because we need to check all the expressions for "unresolved attribute" or other errors first, before we do this check for FunctionTableSubqueryArgumentExpression instances.

That code on L290 starts with this:

getAllExpressions(operator).foreach(_.foreachUp { case a: Attribute if !a.resolved => failUnresolvedAttribute(operator, a, "UNRESOLVED_COLUMN") ...

We need to make sure we perform that "unresolved attribute" check for all expressions in the plan before we proceed further to check for FunctionTableSubqueryArgumentExpression.

HyukjinKwon · 2024-08-15T23:22:10Z

Merged to master.

…mn reference in UDTF call ### What changes were proposed in this pull request? This bug covers improving an error message in the event of invalid UDTF calls. For example: ``` select * from udtf( observed => TABLE(select column from t), value_col => classic_dollars ) ``` Currently we get: ``` Unsupported subquery expression: Table arguments are used in a function where they are not supported: 'UnresolvedTableValuedFunction [udtf], [observed => table-argument#68918 [], value_col => 'classic_dollars, false +- Project ... +- SubqueryAlias ... +- Relation ... ``` But the real error is that the user passed column identifier classic_dollars rather than string "classic_dollars" into the string argument. The core reason is that `CheckAnalysis` checks the analyzer output tree for unresolved TABLE arguments first before checking for remaining unresolved attribute references. To fix it, we update `CheckAnalysis` to move the check for remaining `TABLE` arguments to later. Now that query returns something like: ``` { "errorClass" : "UNRESOLVED_COLUMN.WITHOUT_SUGGESTION", "sqlState" : "42703", "messageParameters" : { "objectName" : "`classic_dollars`" }, "queryContext" : [ { "objectType" : "", "objectName" : "", "startIndex" : 93, "stopIndex" : 109, "fragment" : "classic_dollars" } ] } ``` ### Why are the changes needed? This improves error messages for SQL queries that are invalid, but that a user might reasonably create accidentally while figuring out how the syntax works for calling table-valued functions. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? This PR adds and updates golden file test coverage. ### Was this patch authored or co-authored using generative AI tooling? Just some casual, everyday GitHub CoPilot usage. Closes apache#47447 from dtenedor/improve-error-udtf. Authored-by: Daniel Tenedorio <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]>

commit

75057f7

github-actions bot added the SQL label Jul 22, 2024

fix test

6a3ae60

github-actions bot added the PYTHON label Jul 22, 2024

fix test

7963116

HyukjinKwon approved these changes Jul 23, 2024

View reviewed changes

allisonwang-db reviewed Jul 23, 2024

View reviewed changes

dtenedor commented Aug 13, 2024

View reviewed changes

dtenedor requested a review from allisonwang-db August 13, 2024 19:07

HyukjinKwon closed this in 3e867a6 Aug 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-48966][SQL] Improve error message with invalid unresolved column reference in UDTF call #47447

[SPARK-48966][SQL] Improve error message with invalid unresolved column reference in UDTF call #47447

dtenedor commented Jul 22, 2024 •

edited

Loading

dtenedor commented Jul 22, 2024

allisonwang-db Jul 23, 2024

dtenedor Aug 13, 2024

allisonwang-db Aug 14, 2024

allisonwang-db Jul 23, 2024

dtenedor Aug 13, 2024

dtenedor left a comment

dtenedor Aug 13, 2024

dtenedor Aug 13, 2024

HyukjinKwon commented Aug 15, 2024

[SPARK-48966][SQL] Improve error message with invalid unresolved column reference in UDTF call #47447

[SPARK-48966][SQL] Improve error message with invalid unresolved column reference in UDTF call #47447

Conversation

dtenedor commented Jul 22, 2024 • 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?

Was this patch authored or co-authored using generative AI tooling?

dtenedor commented Jul 22, 2024

allisonwang-db Jul 23, 2024

Choose a reason for hiding this comment

dtenedor Aug 13, 2024

Choose a reason for hiding this comment

allisonwang-db Aug 14, 2024

Choose a reason for hiding this comment

allisonwang-db Jul 23, 2024

Choose a reason for hiding this comment

dtenedor Aug 13, 2024

Choose a reason for hiding this comment

dtenedor left a comment

Choose a reason for hiding this comment

dtenedor Aug 13, 2024

Choose a reason for hiding this comment

dtenedor Aug 13, 2024

Choose a reason for hiding this comment

HyukjinKwon commented Aug 15, 2024

dtenedor commented Jul 22, 2024 •

edited

Loading