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: [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]
