uros-b commented on code in PR #58606:
URL: https://github.com/apache/spark/pull/58606#discussion_r3978186145
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveIdentifierClause.scala:
##########
@@ -95,12 +117,27 @@ class ResolveIdentifierClause(earlyBatches:
Seq[RuleExecutor[LogicalPlan]#Batch]
if (referredTempVars.isDefined) {
referredTempVars.get ++=
collectTemporaryVariablesInExpressionTree(e)
}
+ if (recordUnderIdentifier) {
+ recordTemporaryVariablesUnderIdentifier(e.identifierExpr)
+ }
e.exprBuilder.apply(
IdentifierResolution.evalIdentifierExpr(e.identifierExpr),
e.otherExprs)
}
}
+ /**
+ * Records the temporary variables read by an identifier expression in the
[[AnalysisContext]].
+ * Evaluating the identifier expression is the last time these references
are visible: the
+ * placeholder is replaced by the plan or expression built from the
evaluated name, which no
+ * longer mentions them. Temporary view creation persists the recorded names
so that the
+ * variables are still resolvable when the stored view text is analyzed
again.
+ */
+ private def recordTemporaryVariablesUnderIdentifier(identifierExpr:
Expression): Unit = {
Review Comment:
The comment states "Temporary view and SQL function creation persist the
recorded names," but only view creation consumes
referredTempVariableNamesUnderIdentifier: it is read solely by
CreateViewCommand.markAsAnalyzed and AlterViewAsCommand.markAsAnalyzed.
No SQL-function command reads it (the symbol is absent from master and the
diff touches no function file; CreateSQLFunctionCommand is unchanged). The
withAnalysisContext(function) reset only prevents a function body's identifier
vars from being mis-attributed to the enclosing view, it does not persist them
for the function.
Reword the comment to views only, or, if temporary SQL functions share the
same latent bug, split that off as a follow-up.
##########
sql/core/src/test/resources/sql-tests/inputs/identifier-clause.sql:
##########
@@ -468,3 +468,91 @@ SELECT EXTRACT(IDENTIFIER('YEAR') FROM DATE'2024-01-15');
SELECT TIMESTAMPADD(IDENTIFIER('YEAR'), 1, DATE'2024-01-15');
DROP SCHEMA identifier_clause_test_schema;
+
Review Comment:
Coverage is limited to a LocalTempView table reference (positive) and a
permanent-view rejection (negative). The subtlest new logic, the accumulator
reset on entering a nested view / SQL-function body, which guards against
attributing an inner object's variables to the object being created - has no
test.
Also untested: ALTER VIEW AS ... IDENTIFIER(var), GLOBAL TEMPORARY VIEW, and
the expression-position ExpressionWithUnresolvedIdentifier case. The
ALTER/global paths reuse the same createTemporaryViewRelation call already
exercised, but a nested-view guard test would lock in the reset behavior.
--
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]
