You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "anshulbaliga7 (via GitHub)" <gi...@apache.org> on 2026/10/03 09:13:32 UTC
[PR] [SPARK-59970][SQL] Reject a temporary variable read through an IDENTIFIER clause in a persistent SQL function [spark]
anshulbaliga7 opened a new pull request, #59226:
URL: https://github.com/apache/spark/pull/59226
### What changes were proposed in this pull request?
`CreateSQLFunctionCommand` now captures the temporary variables that the function body reads only inside IDENTIFIER clauses and treats them like any other temporary variable reference: a persistent function is rejected with `INVALID_TEMP_OBJ_REFERENCE`, and a temporary function records them in its `referredTempVariableNames` property.
The names are recorded by `ResolveIdentifierClause` in `AnalysisContext.referredTempVariableNamesUnderIdentifier` (SPARK-59323), but the command analyzed the body with `Analyzer.execute`, which opens a fresh `AnalysisContext` and discards the accumulator on exit. A new `Analyzer.executeWithReferredTempVariablesUnderIdentifier` runs the same fixed-point analysis in a context it owns and returns the recorded names alongside the analyzed plan. It is the sibling of `executeAndCheckReferredTempVariablesUnderIdentifier`, minus `checkAnalysis`, because the function command inspects the analyzed body (return type inference, aggregate/window checks) before checking it. Both analysis sites (scalar and table function) use it, and the names are unioned into `verifyTemporaryObjectsNotExists` and `generateFunctionProperties`.
Bodies without IDENTIFIER clauses are unaffected.
### Why are the changes needed?
A persistent SQL function could capture a session-scoped dependency and then fail in another session:
```sql
DECLARE OR REPLACE VARIABLE col STRING DEFAULT 'a';
CREATE FUNCTION f(x INT) RETURNS INT RETURN (SELECT IDENTIFIER(col) FROM VALUES (1) AS T(a));
-- succeeds; in a new session:
SELECT f(1);
-- [NOT_A_CONSTANT_STRING.NOT_CONSTANT] The expression col used for the routine or clause
-- IDENTIFIER must be a constant STRING which is NOT NULL.
```
A direct reference (`RETURN x + length(col)`) was already rejected, because `ViewHelper.collectTemporaryVariables` sees the `VariableReference` in the analyzed plan. Under an IDENTIFIER clause the reference is gone by then: the placeholder is replaced by the expression or plan built from the evaluated name.
SPARK-59323 (#58606) fixed the same gap for CREATE VIEW, ALTER VIEW, CACHE TABLE AS SELECT and metric views; its review noted the SQL function path as pre-existing and to be tracked separately.
### Does this PR introduce _any_ user-facing change?
Yes. `CREATE FUNCTION` (persistent) whose body reads a temporary variable only through an IDENTIFIER clause now fails with `INVALID_TEMP_OBJ_REFERENCE` at creation, matching a direct variable reference, instead of succeeding and failing later when the function is called from a session where the variable does not exist. Temporary functions are unchanged in behavior; their stored `referredTempVariableNames` now also lists such variables.
### How was this patch tested?
New cases in `sql-udf.sql` (regenerated through `SQLQueryTestSuite`), in the existing "permanent functions with temp objects" section:
- a direct temporary variable reference, as the baseline rejection;
- a variable read through IDENTIFIER in a scalar function body and in a table function body, both rejected with `INVALID_TEMP_OBJ_REFERENCE` naming the variable (both were accepted before this change);
- a temporary function reading the variable through IDENTIFIER is created and evaluates.
```
SPARK_GENERATE_GOLDEN_FILES=1 build/sbt "sql/testOnly org.apache.spark.sql.SQLQueryTestSuite -- -z sql-udf.sql"
build/sbt "sql/testOnly org.apache.spark.sql.SQLQueryTestSuite -- -z sql-udf.sql -z identifier-clause.sql" "sql/testOnly org.apache.spark.sql.execution.SQLFunctionSuite"
```
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude claude-opus-4-8
--
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
Re: [PR] [SPARK-59970][SQL] Reject a temporary variable read through an IDENTIFIER clause in a persistent SQL function [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #59226:
URL: https://github.com/apache/spark/pull/59226#discussion_r4265282501
##########
docs/sql-migration-guide.md:
##########
@@ -47,9 +47,9 @@ license: |
- Since Spark 4.4, `spark.sql.sources.v2.bucketing.partition.filter.enabled` defaults to `true`. In a storage-partitioned join, the partition key groups pushed down to both sides may now be narrowed to those that can produce output for the join type, instead of always taking the union of the two sides' groups: an inner or semi join may keep only the groups present on both sides, a join that keeps or tests every left row (left outer, left anti, left single, existence) keeps the left side's groups, a right outer join keeps the right side's, and a full outer join is unaffected; a CROSS join carrying an equality condition is treated like an inner join. The narrowing is skipped when either side's partitioning may contain unknown partition keys, as after a shuffle on one side, so such a join keeps the full union. A group that is dropped is not scanned at all, so a query may read fewer files. To restore the previous behavior, set `spark.sql.sources.v2.bucketing.partition.filter.enabled` to
`false`.
- Since Spark 4.4, `spark.sql.sources.v2.bucketing.partitionKeyOrdering.enabled` defaults to `true`. A V2 scan that reports a keyed partitioning but no explicit ordering now also reports itself sorted by its partition key expressions, because every row of such a partition evaluates them to the same value. Partition transforms such as `days(ts)` or `bucket(8, id)` are left out of that ordering. This lets Spark drop a `Sort` it would otherwise place above the scan. An aggregate that groups by a leading run of the other key expressions, in key order, may also be planned as a sort aggregate rather than a hash aggregate. This is because `spark.sql.execution.replaceHashWithSortAgg` keys off the reported ordering. To restore the previous behavior, set `spark.sql.sources.v2.bucketing.partitionKeyOrdering.enabled` to `false`.
- Since Spark 4.4, `spark.sql.sources.v2.bucketing.preserveKeyOrderingOnCoalesce.enabled` defaults to `true`. When `GroupPartitionsExec` merges several input partitions that share one partition key value into a single output partition, it now keeps sort orders over the partition key expressions in the ordering it reports, since those expressions are constant within the merged partition. Sort orders over partition transforms such as `days(ts)` or `bucket(8, id)` are left out. Orders over other columns are still dropped, as the concatenation invalidates them, and a join that reduced the partition keys onto a common key space (see `spark.sql.sources.v2.bucketing.allowCompatibleTransforms.enabled`) reports no order, since the merged partitions then share only the reduced key. This can remove a `Sort` downstream of the merge. To restore the previous behavior, set `spark.sql.sources.v2.bucketing.preserveKeyOrderingOnCoalesce.enabled` to `false`.
+- Since Spark 4.4, `CREATE FUNCTION` for a persistent SQL function fails with `INVALID_TEMP_OBJ_REFERENCE` when the function body reads a session variable through an `IDENTIFIER` clause, matching the existing rejection of a direct variable reference. Such a function previously succeeded at creation and failed when called from a session where the variable did not exist. To restore the previous behavior, set `spark.sql.legacy.allowSessionVariableInPersistedView` to `true`.
## Upgrading from Spark SQL 4.2 to 4.3
-
Review Comment:
**Nit (P3):** This removes the blank line after `## Upgrading from Spark SQL 4.2 to 4.3`, a section this PR otherwise doesn't touch. It looks like a leftover from moving the note to the 4.3-to-4.4 section. Every other `## Upgrading ...` heading in the guide is followed by a blank line. Could you restore it so the guide diff only adds the 4.4 note?
**Verification:**
- **Inspection:** The PR diff for the SQL migration guide adds only the 4.4 note and no longer changes the 4.2-to-4.3 section.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/SQLFunctionSuite.scala:
##########
@@ -35,6 +40,79 @@ class SQLFunctionSuite extends SharedSparkSession {
Seq((0, 1), (1, 2)).toDF("a", "b").createOrReplaceTempView("t")
}
+ test("SPARK-59970: a SQL function does not inherit the IDENTIFIER-clause variables of the " +
+ "functions it calls") {
+ withUserDefinedFunction("ident_callee" -> false, "ident_caller" -> false) {
+ sql("DECLARE OR REPLACE VARIABLE ident_col STRING DEFAULT 'a'")
+ try {
+ // The callee may only be created with the legacy flag; its dependency on `ident_col` is
+ // its own and is not persisted.
+ withSQLConf(SQLConf.VARIABLES_UNDER_IDENTIFIER_IN_VIEW.key -> "true") {
+ sql("CREATE FUNCTION ident_callee(x INT) RETURNS INT " +
+ "RETURN (SELECT IDENTIFIER(ident_col) FROM VALUES (1) AS T(a))")
+ }
+ checkAnswer(sql("SELECT ident_callee(0)"), Row(1))
+ // Expanding the callee must not attribute `ident_col` to the caller: creating the caller
+ // succeeds without the flag. (The variable must still exist, since the callee's body is
+ // inlined and its IDENTIFIER clause is evaluated during this analysis.)
+ sql("CREATE FUNCTION ident_caller(x INT) RETURNS INT RETURN ident_callee(x) + 1")
+ // A direct IDENTIFIER read in the caller's own body is still rejected.
+ checkError(
+ exception = intercept[AnalysisException] {
+ sql("CREATE FUNCTION ident_caller2(x INT) RETURNS INT " +
+ "RETURN ident_callee(x) + (SELECT IDENTIFIER(ident_col) FROM VALUES (1) AS T(a))")
+ },
+ condition = "INVALID_TEMP_OBJ_REFERENCE",
+ parameters = Map(
+ "obj" -> "FUNCTION",
+ "objName" -> "`default`.`ident_caller2`",
+ "tempObj" -> "VARIABLE",
+ "tempObjName" -> "`ident_col`"))
+ checkAnswer(sql("SELECT ident_caller(0)"), Row(2))
+ } finally {
+ sql("DROP TEMPORARY VARIABLE IF EXISTS ident_col")
+ }
+ }
+ }
+
+ test("SPARK-59970: IDENTIFIER-clause variables are stored for a temporary function only") {
+ val variableNamesKey = "referredTempVariableNames"
+ withUserDefinedFunction("ident_temp_fn" -> true, "ident_perm_fn" -> false) {
+ sql("DECLARE OR REPLACE VARIABLE ident_col STRING DEFAULT 'a'")
+ try {
+ // A temporary function records the variable, like a direct reference, so the stored body
Review Comment:
**Nit (P3):** This says the temporary function records the variable "so the stored body still resolves the IDENTIFIER clause when it is re-analyzed", but nothing reads `referredTempVariableNames` back. `SQLFunction.referredTempNamesToProps` only writes it, and `AnalysisContext.withAnalysisContext(function)` copies the caller's context rather than restoring anything from the function's properties. At top level the body resolves `ident_col` because no view restricts the lookup, and inside a temporary view the view's own stored list applies, so the result is the same with or without the property. The `AnalysisContext` comment added in this PR says as much ("functions persist no IDENTIFIER-variable metadata of their own that is consulted on resolution").
Could you reword it as parity metadata? For example: "A temporary function records the variable in its properties, as it does a direct reference. The property is informational and is not consulted when the body is re-analyzed." The assertions themselves look right.
**Verification:**
- **Inspection:** The comment above the temporary-function stored-properties assertion describes the recording as parity metadata and makes no claim that the property affects resolution.
- **Behavior:** The stored-properties test still asserts that the temporary function records the IDENTIFIER-clause variable and that the persistent function created under the legacy flag stores none, and it passes.
--
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
Re: [PR] [SPARK-59970][SQL] Reject a temporary variable read through an IDENTIFIER clause in a persistent SQL function [spark]
Posted by "anshulbaliga7 (via GitHub)" <gi...@apache.org>.
anshulbaliga7 commented on PR #59226:
URL: https://github.com/apache/spark/pull/59226#issuecomment-6108078111
Thanks @stevomitric, I have addressed all four in the second commit.
The nested case was a real bug, not intended so `withAnalysisContext(function)` shares the IDENTIFIER accumulator with the caller (right for views as per SPARK-59323), so f2's variable was attributed to g. A new `AnalysisContext` flag turns that sharing off only during function creation and views are unchanged. The new `SQLFunctionSuite` test creates f2 under the legacy flag and confirms g is created with the flag off, while a direct IDENTIFIER read in g's own body is still rejected.
The legacy flag is now honored for functions the same way as for views, and its doc mentions functions. Also added a migration-guide entry under 4.2 to 4.3, and golden cases for a relation-name IDENTIFIER, a session.-qualified name, two variables in one body, the legacy flag, and the nested call.
Can you PTAL again? Thanks!
--
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
Re: [PR] [SPARK-59970][SQL] Reject a temporary variable read through an IDENTIFIER clause in a persistent SQL function [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #59226:
URL: https://github.com/apache/spark/pull/59226#discussion_r4241715770
##########
docs/sql-migration-guide.md:
##########
@@ -46,6 +46,7 @@ license: |
## Upgrading from Spark SQL 4.2 to 4.3
+- Since Spark 4.3, `CREATE FUNCTION` for a persistent SQL function fails with `INVALID_TEMP_OBJ_REFERENCE` when the function body reads a session variable through an `IDENTIFIER` clause, matching the existing rejection of a direct variable reference. Such a function previously succeeded at creation and failed when called from a session where the variable did not exist. To restore the previous behavior, set `spark.sql.legacy.allowSessionVariableInPersistedView` to `true`.
Review Comment:
**Nit (P3):** This change can't ship in 4.3. `branch-4.3` is already at 4.3.1-SNAPSHOT with `v4.3.0-rc1` tagged, and it doesn't have the SPARK-59323 accumulator this PR builds on, while `branch-4.x` (4.4.0-SNAPSHOT) does. As written, users upgrading from 4.3 to 4.4 won't find this breaking `CREATE FUNCTION` change in their section, and the guide says 4.3 already rejects these functions. Could you move it to "Upgrading from Spark SQL 4.3 to 4.4" and say "Since Spark 4.4"?
**Verification:**
- **Inspection:** The migration guide lists the CREATE FUNCTION rejection only in the 4.3-to-4.4 section, attributed to Spark 4.4.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/command/CreateSQLFunctionCommand.scala:
##########
@@ -250,15 +255,22 @@ case class CreateSQLFunctionCommand(
)
}
- (plan, analyzed, Right(returnParam))
+ (plan, analyzed, Right(returnParam), referredTempVarsUnderIdentifier)
}
// A permanent function is not allowed to reference temporary objects.
- // This should be called after `qe.assertAnalyzed()` (i.e., `plan` can be resolved)
- verifyTemporaryObjectsNotExists(catalog, isTemp, name, unresolvedPlan, analyzedPlan)
+ // This should be called after `qe.assertAnalyzed()` (i.e., `plan` can be resolved).
+ // Like persisted views, honor `spark.sql.legacy.allowSessionVariableInPersistedView` for the
+ // variables read only through IDENTIFIER clauses: with it set they are not rejected (and,
+ // as for views, not persisted either).
+ val identifierVarsToReject =
+ if (conf.getConf(SQLConf.VARIABLES_UNDER_IDENTIFIER_IN_VIEW)) Nil else identifierVars
+ verifyTemporaryObjectsNotExists(
+ catalog, isTemp, name, unresolvedPlan, analyzedPlan, identifierVarsToReject)
// Generate function properties.
- val properties = generateFunctionProperties(sparkSession, unresolvedPlan, analyzedPlan)
+ val properties =
+ generateFunctionProperties(sparkSession, unresolvedPlan, analyzedPlan, identifierVars)
Review Comment:
**Nit (P3):** With `spark.sql.legacy.allowSessionVariableInPersistedView=true`, only the rejection list is cleared (`identifierVarsToReject` above). This call still passes the full `identifierVars`, and `generateFunctionProperties` unions them into `tempVars` outside the `isTemp` branch that guards `tempViews`/`tempFunctions`. So a persistent function such as `foo3_5f` is stored in the catalog with `referredTempVariableNames=[["foo3_5_col"]]`. That contradicts the comment above ("as for views, not persisted either"), and persisted views never store temp variable names. Nothing reads the property today, so resolution is unaffected, but the persisted metadata is wrong. Gating `tempVars` on `isTemp`, the same way `tempViews`/`tempFunctions` already are, would fix this and keep the temporary-function recording.
See **Shared repair plan 1** in the review body.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/SQLFunctionSuite.scala:
##########
@@ -35,6 +35,41 @@ class SQLFunctionSuite extends SharedSparkSession {
Seq((0, 1), (1, 2)).toDF("a", "b").createOrReplaceTempView("t")
}
+ test("SPARK-59970: a SQL function does not inherit the IDENTIFIER-clause variables of the " +
+ "functions it calls") {
+ withUserDefinedFunction("ident_callee" -> false, "ident_caller" -> false) {
+ sql("DECLARE OR REPLACE VARIABLE ident_col STRING DEFAULT 'a'")
+ try {
+ // The callee may only be created with the legacy flag; its dependency on `ident_col` is
+ // its own and is not persisted.
Review Comment:
**Nit (P3):** This comment, and the matching one in `sql-udf.sql`, says the dependency is not persisted, but neither test reads the created function's stored properties; both only check creation and query results. The current code does persist `ident_col` under the legacy flag, and the tests still pass. Nothing checks that a temporary function records the variable in `referredTempVariableNames` either, as the PR description says it does. Asserting on the stored function properties, for a temporary function and for a persistent one created under the flag, would pin both.
See **Shared repair plan 1** in the review body.
--
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
Re: [PR] [SPARK-59970][SQL] Reject a temporary variable read through an IDENTIFIER clause in a persistent SQL function [spark]
Posted by "uros-b (via GitHub) via reviews" <re...@spark.apache.org>.
uros-b commented on PR #59226:
URL: https://github.com/apache/spark/pull/59226#issuecomment-6648934108
Thank you @anshulbaliga7 and @stevomitric @HyukjinKwon @cloud-fan!
--
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
Re: [PR] [SPARK-59970][SQL] Reject a temporary variable read through an IDENTIFIER clause in a persistent SQL function [spark]
Posted by "cloud-fan (via GitHub) via reviews" <re...@spark.apache.org>.
cloud-fan closed pull request #59226: [SPARK-59970][SQL] Reject a temporary variable read through an IDENTIFIER clause in a persistent SQL function
URL: https://github.com/apache/spark/pull/59226
--
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
Re: [PR] [SPARK-59970][SQL] Reject a temporary variable read through an IDENTIFIER clause in a persistent SQL function [spark]
Posted by "anshulbaliga7 (via GitHub)" <gi...@apache.org>.
anshulbaliga7 commented on PR #59226:
URL: https://github.com/apache/spark/pull/59226#issuecomment-5971829573
CI is green. @cloud-fan @uros-b @stevomitric could you take a look? This closes the SQL-function gap tracked separately from #58606.
--
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
Re: [PR] [SPARK-59970][SQL] Reject a temporary variable read through an IDENTIFIER clause in a persistent SQL function [spark]
Posted by "anshulbaliga7 (via GitHub)" <gi...@apache.org>.
anshulbaliga7 commented on PR #59226:
URL: https://github.com/apache/spark/pull/59226#issuecomment-6152317366
Thanks @HyukjinKwon for the review, have fixed all three. Stored temporary variable names are now gated on `isTemp` like the view and function names, so a persistent function created under the legacy flag stores none. Added a new test that reads the stored properties for both a temporary and a persistent function. Moved the migration note to 4.3 to 4.4, and you're right that branch-4.3 lacks the SPARK-59323 mechanism.
--
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
Re: [PR] [SPARK-59970][SQL] Reject a temporary variable read through an IDENTIFIER clause in a persistent SQL function [spark]
Posted by "anshulbaliga7 (via GitHub)" <gi...@apache.org>.
anshulbaliga7 commented on PR #59226:
URL: https://github.com/apache/spark/pull/59226#issuecomment-6516337065
Thanks @cloud-fan, fixed the description wording and merged master once more for the migration guide.
Can you PTAL again? Thanks!
--
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
Re: [PR] [SPARK-59970][SQL] Reject a temporary variable read through an IDENTIFIER clause in a persistent SQL function [spark]
Posted by "anshulbaliga7 (via GitHub) via reviews" <re...@spark.apache.org>.
anshulbaliga7 commented on PR #59226:
URL: https://github.com/apache/spark/pull/59226#issuecomment-6562731613
Merged master again for the migration guide (8th time). @cloud-fan @dongjoon-hyun this has two approvals and will keep conflicting while 4.4 notes land, could we get this in when convenient? Thanks!
--
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
Re: [PR] [SPARK-59970][SQL] Reject a temporary variable read through an IDENTIFIER clause in a persistent SQL function [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #59226:
URL: https://github.com/apache/spark/pull/59226#issuecomment-6157626729
Thanks for the update. I re-checked all three: temporary variable names are now collected only for temporary functions, so a persistent function created under the legacy flag stores none; the new SQLFunctionSuite test reads the stored properties of both a temporary and a persistent function; and the migration note is now in the 4.3-to-4.4 section as "Since Spark 4.4".
<!-- SPARK_DEV_REVIEW_REPLY {"feedback_id":"conversation:6152317366","thread_id":"conversation:6152317366","verdict_sha256":"33d5dc960f68924e27fdbbc4b563ea7b929ee371fdab4a6a1bfa6d2449e380fc"} -->
--
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
Re: [PR] [SPARK-59970][SQL] Reject a temporary variable read through an IDENTIFIER clause in a persistent SQL function [spark]
Posted by "anshulbaliga7 (via GitHub)" <gi...@apache.org>.
anshulbaliga7 commented on PR #59226:
URL: https://github.com/apache/spark/pull/59226#issuecomment-6174868753
Thanks @HyukjinKwon, both are done and I have reworded the comment as parity metadata and restored the blank line, so the guide diff is now only the 4.4 note.
Can you PTAL again? Thanks!
--
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
Re: [PR] [SPARK-59970][SQL] Reject a temporary variable read through an IDENTIFIER clause in a persistent SQL function [spark]
Posted by "anshulbaliga7 (via GitHub)" <gi...@apache.org>.
anshulbaliga7 commented on PR #59226:
URL: https://github.com/apache/spark/pull/59226#issuecomment-6198611010
Thanks @cloud-fan for the review, I have added SQL function creation to both consumer lists and it is comment-only.
Can you PTAL again? Thanks!
--
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
Re: [PR] [SPARK-59970][SQL] Reject a temporary variable read through an IDENTIFIER clause in a persistent SQL function [spark]
Posted by "anshulbaliga7 (via GitHub)" <gi...@apache.org>.
anshulbaliga7 commented on PR #59226:
URL: https://github.com/apache/spark/pull/59226#issuecomment-6498901796
@uros-b @cloud-fan CI is green now. Have addressed the above comments, can you PTAL once? Thanks!
--
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
Re: [PR] [SPARK-59970][SQL] Reject a temporary variable read through an IDENTIFIER clause in a persistent SQL function [spark]
Posted by "stevomitric (via GitHub)" <gi...@apache.org>.
stevomitric commented on code in PR #59226:
URL: https://github.com/apache/spark/pull/59226#discussion_r4230974314
##########
sql/core/src/test/resources/sql-tests/analyzer-results/sql-udf.sql.out:
##########
@@ -3959,6 +3959,94 @@ org.apache.spark.sql.AnalysisException
}
+-- !query
+DECLARE OR REPLACE VARIABLE foo3_5_col STRING DEFAULT 'a'
+-- !query analysis
+CreateVariable default(cast(a as string), sql=''a''), true
++- ResolvedIdentifier org.apache.spark.sql.catalyst.analysis.FakeSystemCatalog$@xxxxxxxx, session.foo3_5_col
+
+
+-- !query
+CREATE FUNCTION foo3_5d(x INT) RETURNS INT RETURN x + length(foo3_5_col)
+-- !query analysis
+org.apache.spark.sql.AnalysisException
+{
+ "errorClass" : "INVALID_TEMP_OBJ_REFERENCE",
Review Comment:
will probably need migration guides since some CREATE FUNCTION statements that used to succeed now fail.
##########
sql/core/src/test/resources/sql-tests/inputs/sql-udf.sql:
##########
@@ -817,6 +817,22 @@ CREATE FUNCTION foo3_5d(x INT) RETURNS TABLE (a INT) RETURN SELECT foo3_5a(x);
-- Expect error: permanent function cannot reference temporary view
CREATE FUNCTION foo3_5d(x INT) RETURNS INT RETURN (SELECT SUM(a) FROM t);
CREATE FUNCTION foo3_5d(x INT) RETURNS TABLE (a INT) RETURN SELECT a FROM t;
+-- Expect error: permanent function cannot reference a temporary variable
Review Comment:
It might be useful also to include (maybe not just golden file) tests that cover:
- IDENTIFIER as a table name (FROM IDENTIFIER(tbl_var))
- qualified session.var names
- more than one variable
- the nested-function case
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/command/CreateSQLFunctionCommand.scala:
##########
@@ -385,7 +396,8 @@ case class CreateSQLFunctionCommand(
throw UserDefinedFunctionErrors.invalidTempFuncReference(
routineName = name.asMultipart, tempFuncName = funcName)
}
- val tempVars = ViewHelper.collectTemporaryVariables(analyzed)
+ val tempVars =
+ (ViewHelper.collectTemporaryVariables(analyzed) ++ referredTempVarsUnderIdentifier).distinct
Review Comment:
is `spark.sql.legacy.allowSessionVariableInPersistedView` honored for functions?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -575,6 +575,24 @@ class Analyzer(
}
}
+ /**
+ * Like [[execute]], but also returns the temporary variables recorded via IDENTIFIER clauses
+ * during this analysis (`AnalysisContext.referredTempVariableNamesUnderIdentifier`).
+ *
+ * SQL function creation analyzes the body, inspects it, and only then runs `checkAnalysis`, so
+ * it cannot go through [[executeAndCheckReferredTempVariablesUnderIdentifier]]. It still needs
+ * the recorded variables, which [[execute]] discards with its analysis context: a persistent
+ * function must reject them and a temporary one stores them in its metadata.
+ */
+ def executeWithReferredTempVariablesUnderIdentifier(
Review Comment:
if we have an older persistent f2 with IDENTIFIER(col) in its body, with col declared, CREATE FUNCTION g(x) RETURN f2(x) now fails, naming col. is this intended?
--
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
Re: [PR] [SPARK-59970][SQL] Reject a temporary variable read through an IDENTIFIER clause in a persistent SQL function [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #59226:
URL: https://github.com/apache/spark/pull/59226#discussion_r4281392358
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -182,6 +182,13 @@ case class AnalysisContext(
// than one variable is read via an IDENTIFIER clause.
referredTempVariableNamesUnderIdentifier: mutable.Set[Seq[String]] =
mutable.LinkedHashSet.empty,
+ // Whether a SQL function expanded during this analysis shares the accumulator above with the
Review Comment:
**Nit (P3):** This makes CREATE FUNCTION a consumer of `referredTempVariableNamesUnderIdentifier`. A persistent function rejects the recorded names, or discards them under `spark.sql.legacy.allowSessionVariableInPersistedView`, and a temporary one stores them in `referredTempVariableNames`. The two comments that enumerate the set's consumers still list only the view paths, though:
- The accumulator doc directly above names only a temporary view, temporary ALTER VIEW, or CACHE TABLE AS SELECT as storing the names, and a persisted CREATE/ALTER VIEW as rejecting them.
- `ResolveIdentifierClause.scala:116-118` says "The recorded set is consumed by temporary-view create/ALTER and CACHE TABLE AS SELECT (stored as metadata) and by persisted CREATE/ALTER VIEW (rejected -- unless the legacy flag makes the persisted paths discard it instead)".
This new field doc hints at function creation, but the `ResolveIdentifierClause` list is what someone would read before changing what gets recorded, e.g. the command-name exclusion that comment justifies, or how the legacy flag discards the set. Could you add SQL function creation to both lists? Persistent functions reject the set (or discard it under the legacy flag), and temporary functions store it as informational metadata. This is comment-only; nothing changes at runtime.
**Verification:**
- **Inspection:** The comments describing who consumes the IDENTIFIER-variable accumulator, in the analysis context and in the IDENTIFIER-clause recording rule, name SQL function creation alongside the view and CACHE TABLE AS SELECT consumers, and the diff touches only comments.
--
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
Re: [PR] [SPARK-59970][SQL] Reject a temporary variable read through an IDENTIFIER clause in a persistent SQL function [spark]
Posted by "anshulbaliga7 (via GitHub)" <gi...@apache.org>.
anshulbaliga7 commented on PR #59226:
URL: https://github.com/apache/spark/pull/59226#issuecomment-6372651977
@uros-b @cloud-fan gentle ping. There are no conflicts, and the fork run for the current head is green: https://github.com/anshulbaliga7/spark/actions/runs/39973801850. I think the apache-side build check shows "queued" only because the status-sync workflow has been failing today for all PRs?
--
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
Re: [PR] [SPARK-59970][SQL] Reject a temporary variable read through an IDENTIFIER clause in a persistent SQL function [spark]
Posted by "anshulbaliga7 (via GitHub) via reviews" <re...@spark.apache.org>.
anshulbaliga7 commented on PR #59226:
URL: https://github.com/apache/spark/pull/59226#issuecomment-6624340021
Gentle ping @cloud-fan , can you PTAL once? Thanks!
--
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