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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
