cloud-fan commented on code in PR #58450:
URL: https://github.com/apache/spark/pull/58450#discussion_r3978947709
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -2105,54 +2105,55 @@ class Analyzer(
* This is used for special syntax transformations (e.g., COUNT(*) ->
COUNT(1)) that
* should only apply to builtin functions, not to user-defined functions.
*
- * When the effective SQL PATH puts `system.session` before
`system.builtin`, temp
- * functions shadow builtins, so an unqualified name that matches a temp
function
- * should NOT be treated as builtin.
+ * Mirrors function resolution precedence, including SQL PATH shadowing
for unqualified names
+ * and `spark.sql.legacy.persistentCatalogFirst` for two-part
`builtin.name` references.
*/
- private def matchesFunctionName(nameParts: Seq[String], expectedName:
String): Boolean = {
- if (!FunctionResolution.isUnqualifiedOrBuiltinFunctionName(nameParts,
expectedName)) {
- return false
- }
- if (nameParts.size == 1 &&
functionResolution.isSessionBeforeBuiltinInPath) {
- val v1Catalog = catalogManager.v1SessionCatalog
- !v1Catalog.isTemporaryFunction(FunctionIdentifier(nameParts.head))
- } else {
- true
- }
- }
+ private def matchesFunctionName(nameParts: Seq[String], expectedName:
String): Boolean =
+ functionResolution.functionNameResolvesToBuiltin(nameParts, expectedName)
/**
* Expands the matching attribute.*'s in `child`'s output.
*/
def expandStarExpression(expr: Expression, child: LogicalPlan): Expression
= {
expr.transformUp {
- case f0: UnresolvedFunction if !f0.isDistinct &&
- matchesFunctionName(f0.nameParts, "count") &&
- isCountStarExpansionAllowed(f0.arguments) =>
- // Transform COUNT(*) into COUNT(1).
- // We do not normalize the name to "count"; we keep the original
name parts
- // (e.g. builtin.count, system.builtin.count) so that resolution
still sees
- // the same qualification.
- f0.copy(arguments = Seq(Literal(1)))
- case f1: UnresolvedFunction if containsStar(f1.arguments) =>
- // SPECIAL CASE: We want to block count(tblName.*) because in spark,
count(tblName.*) will
- // be expanded while count(*) will be converted to count(1). They
will produce different
- // results and confuse users if there are any null values. For
count(t1.*, t2.*), it is
- // still allowed, since it's well-defined in spark.
- if (!conf.allowStarWithSingleTableIdentifierInCount &&
- matchesFunctionName(f1.nameParts, "count") &&
- f1.arguments.length == 1) {
- f1.arguments.foreach {
- case u: UnresolvedStar if u.isQualifiedByTable(child.output,
resolver) =>
- throw QueryCompilationErrors
-
.singleTableStarInCountNotAllowedError(u.target.get.mkString("."))
- case _ => // do nothing
+ case f: UnresolvedFunction if containsStar(f.arguments) =>
+ // A routed SQL/JSON function (json_array(*)) forbids a bare `*`;
reject it rather than
+ // expand below. A nested star (json_array(array(*))) is expanded
bottom-up before we get
+ // here, so only a bare `*` reaches this guard.
+ if
(functionResolution.resolvesToStarDisallowedJsonConstructor(f.nameParts)) {
Review Comment:
**Non-blocking (P2):** This guard still decides from the static built-in
name before resolving the registered builder.
`SparkSessionExtensions.injectFunction` installs a one-part function under the
same `system.builtin` identifier and replaces the existing entry, so an
injected two-argument `json_array` is rejected on `json_array(*)` before its
builder can receive the expanded columns. Please bind this restriction to the
selected stock SQL/JSON builder and add an injected-replacement regression in
both analyzer modes.
**Recommended change:** Move the JSON direct-star ownership decision to the
selected stock builder or an equivalent post-resolution implementation-identity
check, and cover an injected json_array replacement in both analyzer modes.
**Why this works:** Carry the direct-star fact through normal routine
resolution and reject it only when resolution selects the stock SQL/JSON
expression builder, instead of deciding from the static built-in-name set
before the registered builder is known.
**Scope:** SQL/JSON star preprocessing and function-resolution ownership in
Catalyst, plus focused SparkSessionExtensions regression coverage.
**Compatibility:** Keep stock SQL/JSON star rejection unchanged while
preserving ordinary star expansion for temporary, persistent, and
SparkSessionExtensions-provided functions selected by routine resolution.
**Risks:** Deferring the rejection must not allow the stock builder to
consume an expanded star. The two analyzer paths must not diverge in which
implementation identity they recognize as stock.
**Constraints:** The decision must be identical in the fixed-point and
single-pass analyzers. Qualification, SQL PATH precedence, stored-view
visibility, and persistentCatalogFirst behavior must remain unchanged. Only
Spark's stock SQL/JSON builders may receive the built-in-only direct-star
rejection.
**Success:** A SparkSessionExtensions replacement named json_array receives
the expanded columns from json_array(*) in both analyzer modes, while the
unmodified stock JSON_ARRAY(*) call still raises INVALID_USAGE_OF_STAR_OR_REGEX.
##########
sql/core/src/test/scala/org/apache/spark/sql/JsonArraySuite.scala:
##########
@@ -154,6 +157,18 @@ class JsonArraySuite extends QueryTest with
SharedSparkSession {
Row("[[1]]"))
}
+ test("a function-style string() cast detaches implicit FORMAT JSON like
CAST(... AS STRING)") {
+ // A user cast to STRING quotes the fragment (detaches implicit FORMAT
JSON). The function-style
+ // alias string(x) is exactly CAST(x AS STRING) and must behave
identically: `isImplicitlyJson`
Review Comment:
**Nit (P3):** At the parse-time routing decision, `string(...)` is still an
`UnresolvedFunction`; it becomes a `Cast` only later when the alias is
resolved, after the outer JSON_ARRAY has already received false FORMAT JSON
flags. The expected result is useful, but this comment attributes it to a
branch the case does not exercise. Please describe the unresolved-call routing
path instead.
##########
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. This is the shared list backing the star guard in
+ * [[FunctionResolution.resolvesToStarDisallowedJsonConstructor]], which
derives its set from here
+ * so a newly routed constructor/path function is covered automatically. It
is NOT a single source
+ * of truth for the whole feature: two sibling lists still need a matching
manual entry when a
Review Comment:
**Nit (P3):** This synchronization list misses
`ResolverGuard.isGenerallySupportedExpression`, another closed allowlist that
this PR updates for all four routed SQL/JSON expressions. A maintainer can
follow the two documented steps and still have the next clause-bearing
expression rejected by the single-pass resolver. Please add ResolverGuard as
the third sync site, or narrow the wording so the list is not presented as
exhaustive.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionResolution.scala:
##########
@@ -404,6 +388,97 @@ class FunctionResolution(
}
}
+ /**
+ * Returns whether an unqualified function name reaches `system.builtin`
before any temp or
Review Comment:
**Nit (P3):** This public helper returns true as soon as PATH reaches
`system.builtin`, even when that namespace has no function with this name; its
current caller is safe only because it first checks
`FunctionRegistry.functionSet`. Please document that the argument must already
be known to be a stock built-in name, so the advertised ownership predicate is
not reused without its required precondition.
##########
sql/core/src/test/scala/org/apache/spark/sql/JsonArraySuite.scala:
##########
@@ -466,18 +493,310 @@ class JsonArraySuite extends QueryTest with
SharedSparkSession {
}
}
+ test("plain call goes through routine resolution and can be shadowed via SET
PATH") {
+ // `withUserDefinedFunction` is unusable here: its cleanup asserts the
name no longer resolves,
+ // but `json_array` is now a registered built-in, so drop the temporary
routine explicitly.
+ withSQLConf(
+ SQLConf.PATH_ENABLED.key -> "true",
+ SQLConf.SESSION_FUNCTION_RESOLUTION_ORDER.key -> "second") {
+ try {
+ sql("CREATE TEMPORARY FUNCTION json_array(a INT, b STRING) RETURNS
STRING " +
+ "RETURN 'shadowed'")
+ sql("CREATE TEMPORARY FUNCTION json_value(a STRING, b STRING) RETURNS
STRING " +
+ "RETURN 'shadowed'")
+ sql("CREATE TEMPORARY FUNCTION json_query(a STRING, b STRING) RETURNS
STRING " +
+ "RETURN 'shadowed'")
+ sql("CREATE TEMPORARY FUNCTION json_exists(a STRING, b STRING) RETURNS
BOOLEAN " +
+ "RETURN false")
+ sql("SET PATH = system.session, system.builtin")
+ // A plain call is an ordinary function call, so the temporary routine
(ahead of
+ // system.builtin on the path) shadows the built-in constructor.
+ checkAnswer(sql("SELECT json_array(1, 'x')"), Row("shadowed"))
+ checkAnswer(sql("SELECT json_array(*) FROM VALUES (1, 'x') AS t(a,
b)"), Row("shadowed"))
+ // The clause-bearing form is not a function call, so it stays the
built-in constructor.
+ checkAnswer(sql("SELECT json_array('x' NULL ON NULL)"),
Row("""["x"]"""))
+ // Nested JSON-producing children stay on the direct-construction
path, so they are not
+ // shadowed. This preserves JSON_ARRAY's parse-time splice decisions.
+ checkAnswer(sql("SELECT json_array(json_array(1))"), Row("[[1]]"))
+ checkAnswer(
+ sql("""SELECT json_array(json_query('{"a":{"x":1}}', '$.a'))"""),
+ Row("""[{"x":1}]"""))
+ // Plain scalar and predicate children are still ordinary function
calls. Use an explicit
+ // outer NULL clause to keep the parent on the direct path while the
children are shadowed.
+ checkAnswer(
+ sql("""SELECT json_array(json_value('{"a":"x"}', '$.a') NULL ON
NULL)"""),
+ Row("""["shadowed"]"""))
+ checkAnswer(
+ sql("""SELECT json_array(json_exists('{"a":1}', '$.a') NULL ON
NULL)"""),
+ Row("[false]"))
+ } finally {
+ sql("SET PATH = DEFAULT_PATH")
+ sql("DROP TEMPORARY FUNCTION IF EXISTS json_array")
+ sql("DROP TEMPORARY FUNCTION IF EXISTS json_value")
+ sql("DROP TEMPORARY FUNCTION IF EXISTS json_query")
+ sql("DROP TEMPORARY FUNCTION IF EXISTS json_exists")
+ }
+ }
+ }
+
+ test("qualified plain JSON_ARRAY resolves to the built-in constructor") {
+ checkAnswer(sql("SELECT builtin.json_array(1, 'x')"), Row("""[1,"x"]"""))
+ checkAnswer(sql("SELECT system.builtin.json_array(1, 'x')"),
Row("""[1,"x"]"""))
+ }
+
+ test("a nested JSON constructor through a routed JSON_ARRAY call is quoted,
not spliced") {
Review Comment:
**Nit (P3):** This block also covers `JSON_QUERY`, which is a SQL/JSON query
function rather than a constructor. Please call the nested argument a
JSON-producing expression or value in the test title and opening comment so the
terminology covers every case in the block.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -2105,54 +2105,55 @@ class Analyzer(
* This is used for special syntax transformations (e.g., COUNT(*) ->
COUNT(1)) that
* should only apply to builtin functions, not to user-defined functions.
*
- * When the effective SQL PATH puts `system.session` before
`system.builtin`, temp
- * functions shadow builtins, so an unqualified name that matches a temp
function
- * should NOT be treated as builtin.
+ * Mirrors function resolution precedence, including SQL PATH shadowing
for unqualified names
+ * and `spark.sql.legacy.persistentCatalogFirst` for two-part
`builtin.name` references.
*/
- private def matchesFunctionName(nameParts: Seq[String], expectedName:
String): Boolean = {
- if (!FunctionResolution.isUnqualifiedOrBuiltinFunctionName(nameParts,
expectedName)) {
- return false
- }
- if (nameParts.size == 1 &&
functionResolution.isSessionBeforeBuiltinInPath) {
- val v1Catalog = catalogManager.v1SessionCatalog
- !v1Catalog.isTemporaryFunction(FunctionIdentifier(nameParts.head))
- } else {
- true
- }
- }
+ private def matchesFunctionName(nameParts: Seq[String], expectedName:
String): Boolean =
+ functionResolution.functionNameResolvesToBuiltin(nameParts, expectedName)
/**
* Expands the matching attribute.*'s in `child`'s output.
*/
def expandStarExpression(expr: Expression, child: LogicalPlan): Expression
= {
expr.transformUp {
- case f0: UnresolvedFunction if !f0.isDistinct &&
- matchesFunctionName(f0.nameParts, "count") &&
- isCountStarExpansionAllowed(f0.arguments) =>
- // Transform COUNT(*) into COUNT(1).
- // We do not normalize the name to "count"; we keep the original
name parts
- // (e.g. builtin.count, system.builtin.count) so that resolution
still sees
- // the same qualification.
- f0.copy(arguments = Seq(Literal(1)))
- case f1: UnresolvedFunction if containsStar(f1.arguments) =>
- // SPECIAL CASE: We want to block count(tblName.*) because in spark,
count(tblName.*) will
- // be expanded while count(*) will be converted to count(1). They
will produce different
- // results and confuse users if there are any null values. For
count(t1.*, t2.*), it is
- // still allowed, since it's well-defined in spark.
- if (!conf.allowStarWithSingleTableIdentifierInCount &&
- matchesFunctionName(f1.nameParts, "count") &&
- f1.arguments.length == 1) {
- f1.arguments.foreach {
- case u: UnresolvedStar if u.isQualifiedByTable(child.output,
resolver) =>
- throw QueryCompilationErrors
-
.singleTableStarInCountNotAllowedError(u.target.get.mkString("."))
- case _ => // do nothing
+ case f: UnresolvedFunction if containsStar(f.arguments) =>
+ // A routed SQL/JSON function (json_array(*)) forbids a bare `*`;
reject it rather than
Review Comment:
**Nit (P3):** `containsStar` here and `case _: Star` in the single-pass path
also select a direct qualified star such as `t.*`; they do not reject only the
literal bare `*`. Please describe this as a direct star argument, including
qualified stars, in both analyzer comments so the documented syntax domain
matches the guard.
--
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]
