cloud-fan commented on code in PR #58450:
URL: https://github.com/apache/spark/pull/58450#discussion_r3933755515
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala:
##########
@@ -2352,6 +2352,20 @@ class SessionCatalog(
}
}
+ /**
+ * Returns whether a temporary function is visible in the current resolution
context, applying the
+ * same stored-view filtering as actual resolution ([[handleViewContext]])
but WITHOUT its
+ * side effect of recording the name as a referred temp function. Inside a
stored view a temp
+ * function is visible only if the view captured it; outside a view this
matches
+ * [[isTemporaryFunction]]. Ownership probes use this so they agree with the
resolver on which
+ * routine owns a name inside a view.
+ */
+ def isTemporaryFunctionVisible(name: FunctionIdentifier): Boolean = {
+ isTemporaryFunction(name) &&
+ (AnalysisContext.get.catalogAndNamespace.isEmpty ||
Review Comment:
**Non-blocking (P2):** Could you add a stored-view regression for this
captured-name branch? Create the same-named temporary JSON routine after the
view so it exists in the session registry but was not captured, then verify the
owner probe ignores it and the builtin bare-star guard raises
`INVALID_USAGE_OF_STAR_OR_REGEX`. The current view test covers
persistent-catalog expansion and would stay green if this predicate regressed.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -4160,23 +4160,46 @@ class AstBuilder extends DataTypeAstBuilder
(JsonValueBehavior.Default, Some(expression(d.defaultExpr)))
}
+ // A clause-free JSON_ARRAY / JSON_QUERY that is a top-level JSON_ARRAY
element stays on the
+ // direct path. Those expressions emit JSON text implicitly, and the parent
JSON_ARRAY must see
+ // that lexical fact before analyzer rewrites can wrap the child in a Cast.
+ private def isTopLevelJsonArrayElement(ctx: RuleContext): Boolean = {
+ @scala.annotation.tailrec
+ def loop(parent: RuleContext): Boolean = parent match {
+ case null => false
+ case _: JsonArrayValueContext => true
+ case _: ExpressionContext | _: ValueExpressionDefaultContext |
+ _: ParenthesizedExpressionContext | _: CollateContext =>
+ loop(parent.getParent)
+ case p: PredicatedContext if p.predicate() == null =>
+ loop(parent.getParent)
+ case _ => false
+ }
+ loop(ctx.getParent)
+ }
+
/**
* Create a [[JsonValue]] expression for the SQL:2016 `JSON_VALUE` scalar
function. The `ON EMPTY`
* / `ON ERROR` clauses default to `NULL` when absent, per the standard.
*/
override def visitJsonValue(ctx: JsonValueContext): Expression =
withOrigin(ctx) {
val jsonExpr = expression(ctx.jsonExpr)
val path = string(visitStringLit(ctx.path))
- // Default RETURNING type is STRING. Normalize CHAR/VARCHAR to STRING for
the cast, as the value
- // is produced by a `Cast` to the declared type (a raw CHAR/VARCHAR target
has no encoder).
- val returning = Option(ctx.returning)
- .map(dt =>
CharVarcharUtils.replaceCharVarcharWithStringForCast(typedVisit[DataType](dt)))
- .getOrElse(StringType)
- val (onEmpty, emptyDefault) = Option(ctx.emptyBehavior)
- .map(buildJsonValueBehavior).getOrElse((JsonValueBehavior.Null, None))
- val (onError, errorDefault) = Option(ctx.errorBehavior)
- .map(buildJsonValueBehavior).getOrElse((JsonValueBehavior.Null, None))
- JsonValue(jsonExpr, path, returning, onEmpty, onError, emptyDefault,
errorDefault)
+ if (ctx.returning == null && ctx.emptyBehavior == null &&
ctx.errorBehavior == null) {
Review Comment:
**Blocking (P1):** **Blocking:** Clause presence now chooses direct builtin
construction, but canonical `Expression.sql` drops default-valued clauses. With
a temp `json_value` first on PATH, `JSON_VALUE(... RETURNING STRING)` is
builtin while its rendered clause-free SQL reparses to the temp routine,
changing the result and potentially its schema. Please keep canonical SQL on
the direct-builtin path and add a render/reparse regression under a shadowing
PATH.
**Recommended change:** Preserve direct-builtin ownership in canonical SQL
by rendering an explicit default clause for each affected JSON expression, with
focused shadowing round-trip coverage.
**Why this works:** Because AstBuilder treats any supported clause as direct
syntax, retaining one explicit default clause prevents the rendered form from
entering ordinary routine resolution when reparsed.
**Scope:** The JSON_VALUE, JSON_QUERY, JSON_EXISTS, and JSON_ARRAY sql
renderers plus focused parser/analyzer round-trip tests.
**Compatibility:** Runtime evaluation stays unchanged; canonical SQL becomes
more explicit and continues to encode the analyzed builtin owner.
**Risks:** Callers comparing exact Expression.sql strings will observe
explicit default clauses. Each constructor has different default clauses and
result metadata that must remain stable.
**Constraints:** Preserve nested child SQL and result types. Do not make
genuinely clause-free user input non-shadowable. Cover relevant non-default SQL
PATH order.
**Success:** Every directly constructed default-valued JSON expression
renders SQL that reparses to the same builtin owner, value, and schema with a
same-named routine first on PATH.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionResolution.scala:
##########
@@ -404,6 +388,93 @@ class FunctionResolution(
}
}
+ /**
+ * Returns whether an unqualified function name reaches `system.builtin`
before any temp or
+ * persistent function in the effective SQL PATH. When a temp or persistent
function shadows the
+ * builtin, special-syntax handling that only applies to Spark's builtins
must not fire, since the
+ * name no longer refers to the builtin -- e.g. rejecting a bare `*` in a
routed JSON constructor,
+ * or the `count(tbl.*)` guard. (Bare unqualified `count(*)` is normalized
to `count(1)` earlier
Review Comment:
**Nit (P3):** This is true only for parser-built SQL. `functions.count("*")`
constructs an `UnresolvedStar` without `AstBuilder`, and analyzer normalization
calls this owner probe. Please qualify the comment so it does not exclude the
reachable DataFrame path.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/resolver/FunctionResolverUtils.scala:
##########
@@ -141,7 +132,7 @@ trait FunctionResolverUtils {
private def assertSingleTableStarNotInCountFunction(
unresolvedFunction: UnresolvedFunction): Unit = {
if (!conf.allowStarWithSingleTableIdentifierInCount &&
- isCount(unresolvedFunction) &&
+
functionResolution.functionNameResolvesToBuiltin(unresolvedFunction.nameParts,
"count") &&
Review Comment:
**Non-blocking (P2):** Please compute the count owner once for this
unresolved function and reuse it for normalization and the table-star guard.
When a persistent candidate precedes `system.builtin`, each predicate call can
invoke `FunctionCatalog.functionExists`; the current false-normalization path
immediately repeats that external lookup for the same node. The classic
analyzer has the same duplicate pattern.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala:
##########
@@ -1025,9 +1025,31 @@ object FunctionRegistry {
expression[SchemaOfJson]("schema_of_json"),
expression[LengthOfJsonArray]("json_array_length"),
expression[JsonObjectKeys]("json_object_keys"),
- expression[JsonTypeof]("json_typeof")
+ expression[JsonTypeof]("json_typeof"),
+ // Built-in forms of the SQL:2016 JSON constructor and path functions,
resolved for plain calls
+ // that carry no SQL/JSON clauses (the dedicated grammar handles
clause-bearing syntax). The
+ // names must stay in sync with `routedJsonConstructorNames` below.
+ expressionBuilder("json_value", JsonValueExpressionBuilder),
+ expressionBuilder("json_query", JsonQueryExpressionBuilder),
+ expressionBuilder("json_exists", JsonExistsExpressionBuilder),
+ expressionBuilder("json_array", JsonArrayExpressionBuilder)
)
+ /**
+ * Names of the clause-free SQL/JSON constructor/path functions that
`AstBuilder` routes through
+ * function resolution. Single source of truth for the three places that
must stay in sync when a
Review Comment:
**Nit (P3):** This is a shared list for the star guard, not a single source
of truth for all three locations: builder registrations and parser routing
still need manual updates, as the final sentence notes. Please state those
synchronization points directly and retain “constructor/path function”
terminology for all four names.
##########
sql/core/src/test/scala/org/apache/spark/sql/JsonArraySuite.scala:
##########
@@ -218,6 +232,17 @@ class JsonArraySuite extends QueryTest with
SharedSparkSession {
Row("[[1,2],3]"))
}
+ test("a nested constructor wrapped in redundant parentheses is still spliced
raw") {
+ // Parentheses wrap the nested constructor in a value-preserving
ParenthesizedExpression. The
Review Comment:
**Nit (P3):** There is no Catalyst `ParenthesizedExpression` wrapper here.
Parentheses create an ANTLR `ParenthesizedExpressionContext`, and
`AstBuilder.visitParenthesizedExpression` returns the inner Catalyst expression
directly. Please describe the parser context traversal instead.
--
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]
