-
Notifications
You must be signed in to change notification settings - Fork 28.7k
[SPARK-52146][SQL] Detect cyclic function references in SQL UDFs #51626
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-52146][SQL] Detect cyclic function references in SQL UDFs #51626
Conversation
val outer = expr.transform { | ||
case a: Attribute if a.resolved => OuterReference(a) | ||
case o: OuterReference => OuterReference(o) | ||
} | ||
Alias(Cast(outer, param.dataType), param.name)( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @cloud-fan this is needed to make the plan structure valid when using OneRowRelation.
30c74a8
to
eaee0a7
Compare
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala
Show resolved
Hide resolved
} | ||
// Check cyclic reference using qualified function names. | ||
val newPath = path :+ f.function.name | ||
if (f.function.name == name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shall we consider case sensitivity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and is this name
qualified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function names are always case insensitive, and the name here is qualified. I've added a few more tests to cover both scenarios.
…log/SessionCatalog.scala Co-authored-by: Wenchen Fan <cloud0fan@gmail.com>
need to fix |
@@ -367,6 +373,61 @@ case class CreateSQLFunctionCommand( | |||
} | |||
} | |||
|
|||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you fix the indentation, @allisonwang-db ?
CREATE OR REPLACE FUNCTION foo3_4a(x INT) RETURN FoO3_4b(x); | ||
CREATE OR REPLACE FUNCTION foo3_4a(x INT) RETURNS INT RETURN SELECT SUM(a) FROM foo3_4e(x); | ||
CREATE OR REPLACE FUNCTION foo3_4e(x INT) RETURNS TABLE (c INT) RETURN SELECT * FROM foo3_4f(x); | ||
CREATE OR REPLACE FUNCTION foo3_4e(x INT) RETURNS TABLE RETURN SELECT * FROM fOo3_4F(x); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for adding this test coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, LGTM (with one minor comment).
cc @peter-toth
the last commit just fixed indentation, thanks, merging to master/4.0! |
This change adds logic to detect cyclic function references when creating SQL UDFs to prevent infinite analysis. To improve SQL UDF usability No New tests No Closes #51626 from allisonwang-db/spark-52146-cyc-func-usage. Lead-authored-by: Allison Wang <allison.wang@databricks.com> Co-authored-by: Allison Wang <allisonwang@apache.org> Co-authored-by: Wenchen Fan <cloud0fan@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 3ff28ae) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
Late LGTM. |
What changes were proposed in this pull request?
This change adds logic to detect cyclic function references when creating SQL UDFs to prevent infinite analysis.
Why are the changes needed?
To improve SQL UDF usability
Does this PR introduce any user-facing change?
No
How was this patch tested?
New tests
Was this patch authored or co-authored using generative AI tooling?
No