cloud-fan commented on code in PR #59828:
URL: https://github.com/apache/spark/pull/59828#discussion_r4418287793
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/jsonExpressions.scala:
##########
@@ -1314,6 +1314,57 @@ trait ImplicitlyFormattedAsJson extends Expression {
def emitsImplicitJsonText: Boolean = true
}
+object ImplicitlyFormattedAsJson {
+ /**
+ * Whether `e` lexically emits JSON text -- i.e. carries an implicit
SQL/JSON `FORMAT JSON` and
+ * should be spliced raw rather than quoted (a nested JSON constructor, or
`JSON_QUERY` under
+ * `KEEP QUOTES`; see [[ImplicitlyFormattedAsJson]]). Sees through a
value-preserving `Collate`
+ * (a postfix `... COLLATE c` only annotates the collation of the JSON text,
e.g.
+ * `JSON_ARRAY(JSON_ARRAY(1) COLLATE UTF8_LCASE)`), but deliberately NOT
through a `Cast`:
+ * `AstBuilder`/`JsonArray.sql` use a `CAST(... AS STRING)` wrapper
specifically to detach a value
+ * from implicit `FORMAT JSON`, so unwrapping it would defeat that
neutralization. The routed/
+ * qualified builders use [[splicesThroughCollationCast]], which
additionally sees through an
+ * analyzer collation `Cast`.
+ */
+ @scala.annotation.tailrec
+ def isImplicitlyJson(e: Expression): Boolean = e match {
+ case c: Collate => isImplicitlyJson(c.child)
+ case i: ImplicitlyFormattedAsJson => i.emitsImplicitJsonText
+ case _ => false
+ }
+
+ /**
+ * The routed/qualified splice decision made by `JsonArrayExpressionBuilder`
/
+ * `JsonObjectExpressionBuilder` on a resolved argument. Like
[[isImplicitlyJson]], but also sees
+ * through an analyzer-inserted collation `Cast`: under an object/view
default collation,
+ * `ApplyDefaultCollation` wraps a nested producer in a `Cast` to the
default `StringType` before
+ * the builder runs, which would otherwise hide the producer and quote it
(unlike the direct
+ * grammar path, which freezes the decision at parse time). Only a genuine
collation change is
+ * unwrapped (the cast recolors existing JSON text); a user `CAST(... AS
STRING)` or the
+ * `string(...)` alias still detaches, so the explicit-cast quoting is
preserved.
+ */
+ @scala.annotation.tailrec
+ def splicesThroughCollationCast(e: Expression): Boolean = e match {
+ case c: Cast if isAnalyzerCollationCast(c) =>
splicesThroughCollationCast(c.child)
+ case _ => isImplicitlyJson(e)
Review Comment:
**Non-blocking (P2):** This unwraps an analyzer collation Cast only when it
is the outermost node, then defers to `isImplicitlyJson`, which sees through
`Collate` but not `Cast`. Under a default collation,
`castDefaultStringExpressions` descends through a user `COLLATE` and casts the
producer below it, so a qualified or routed nested producer arrives as
`Collate(Cast(JsonArray, STRING COLLATE <default>), <c>)` and is quoted. For
example, `CREATE TABLE t DEFAULT COLLATION UTF8_LCASE AS SELECT
json_array(json_array(0), builtin.json_array(1) COLLATE UNICODE) AS b` emits
the second element as a quoted string, while the same SELECT without a default
collation returns `[[0],[1]]`. `builtin.json_array(json_array(1) COLLATE
UNICODE)` and `builtin.json_object('a', json_object('b', 1) COLLATE UNICODE)`
behave the same way through the builders. Matching any interleaving of
`Collate` and an analyzer collation `Cast`, while still stopping at user casts,
would close this for every consumer of t
he predicate.
See **Shared repair plan 1** in the review body.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/jsonExpressions.scala:
##########
@@ -1314,6 +1314,57 @@ trait ImplicitlyFormattedAsJson extends Expression {
def emitsImplicitJsonText: Boolean = true
}
+object ImplicitlyFormattedAsJson {
+ /**
+ * Whether `e` lexically emits JSON text -- i.e. carries an implicit
SQL/JSON `FORMAT JSON` and
+ * should be spliced raw rather than quoted (a nested JSON constructor, or
`JSON_QUERY` under
+ * `KEEP QUOTES`; see [[ImplicitlyFormattedAsJson]]). Sees through a
value-preserving `Collate`
+ * (a postfix `... COLLATE c` only annotates the collation of the JSON text,
e.g.
+ * `JSON_ARRAY(JSON_ARRAY(1) COLLATE UTF8_LCASE)`), but deliberately NOT
through a `Cast`:
+ * `AstBuilder`/`JsonArray.sql` use a `CAST(... AS STRING)` wrapper
specifically to detach a value
+ * from implicit `FORMAT JSON`, so unwrapping it would defeat that
neutralization. The routed/
+ * qualified builders use [[splicesThroughCollationCast]], which
additionally sees through an
+ * analyzer collation `Cast`.
+ */
+ @scala.annotation.tailrec
+ def isImplicitlyJson(e: Expression): Boolean = e match {
+ case c: Collate => isImplicitlyJson(c.child)
+ case i: ImplicitlyFormattedAsJson => i.emitsImplicitJsonText
+ case _ => false
+ }
+
+ /**
+ * The routed/qualified splice decision made by `JsonArrayExpressionBuilder`
/
+ * `JsonObjectExpressionBuilder` on a resolved argument. Like
[[isImplicitlyJson]], but also sees
+ * through an analyzer-inserted collation `Cast`: under an object/view
default collation,
+ * `ApplyDefaultCollation` wraps a nested producer in a `Cast` to the
default `StringType` before
+ * the builder runs, which would otherwise hide the producer and quote it
(unlike the direct
+ * grammar path, which freezes the decision at parse time). Only a genuine
collation change is
Review Comment:
**Nit (P3):** This contrasts the predicate with "the direct grammar path,
which freezes the decision at parse time", but stock-owned grammar wrappers now
run this same predicate in `finalizeFlags`. JsonArray's own docs make the same
outdated claim. The class scaladoc says `formatJson` "is set once, at parse
time" (1378), the field comment says both flags are "never re-derived from the
child expression" (1415), and the eval comment calls `needsValidation` "the
frozen parse-time" flag (1641). For `JSON_ARRAY(builtin.json_array(1) NULL ON
NULL)`, the element is spliced only because analysis re-derives the flag from
the resolved child. The updated JsonObjectExpr comment already describes the
actual lifecycle: flags are finalized before the node is built and frozen
afterward.
**Verification:**
- **Inspection:** The JsonArray and splice-predicate comments describe
parser flags plus analysis-time recovery from resolved children, frozen once
the node is built, consistent with the JSON_OBJECT comment.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/jsonExpressions.scala:
##########
@@ -1720,6 +1760,100 @@ object JsonArray {
}
}
+/**
+ * A parsed `JSON_ARRAY(...)` / `JSON_OBJECT(...)` whose splice flags are
finalized after child
+ * resolution: parser flags keep explicit `FORMAT JSON` and lexical nested
constructors, and the
+ * resolved children add qualified/routed nested constructors the parser
cannot see.
+ *
+ * A clause-free call sets `routeThroughResolution` so a same-named routine
(or an `injectFunction`
+ * replacement) can shadow it; it then resolves as a plain
`prettyName(arguments...)` call. Other
+ * calls are owned by the stock built-in.
+ */
+trait UnresolvedJsonConstructor
+ extends Unevaluable
+ with DefaultStringProducingExpression
Review Comment:
**Blocking (P1):** This wrapper claims `DefaultStringProducingExpression`
with `dataType = returning`, which is the plain STRING type for every routed
wrapper. Under a table or view default collation, the collation runner inside
ResolveAliases (and ResolveReferences for view reads) wraps it in `Cast(_,
STRING COLLATE <default>)` before ResolveJsonConstructors decides ownership.
When a shadow or `injectFunction` replacement owns the name, the wrapper is
replaced in place by an `UnresolvedFunction`, and the Cast stays around the
owner's result. For example, with the INT-returning `json_array` replacement
from SparkSessionExtensionSuite, `CREATE TABLE t DEFAULT COLLATION UTF8_LCASE
AS SELECT json_array(json_array(1)) AS c` stores c as STRING COLLATE UTF8_LCASE
with value '1' instead of INT 1, while `SELECT json_array(1)` keeps INT. The
same applies to `json_object('a', json_object('b', 1))`, the VALUE/colon
spellings, temporary/PATH/V2 owners, and every read of a view with a default
collation. A STRING-returning owner is silently recolored. For the comma-form
`json_object` with a nested producer, the merge target already routed this call
without the Cast, so this is a regression there. Single-pass decides ownership
first, so the two analyzers disagree on these plans.
**Recommended change:** Stop routed SQL/JSON constructor wrappers
(clause-free calls that contain a nested constructor) from presenting
themselves as default-string-producing expressions, so collation rules treat
them like the unresolved function call they stand for. Stock-owned wrappers
keep reporting their RETURNING type. Add JSON_ARRAY and JSON_OBJECT tests in
which a temporary function or an injected replacement owns the name, and
analyze nested calls under a table default collation and a view default
collation.
**Why this works:** The default-collation pass casts only nodes that declare
themselves default-string producers. Without that claim, the pass descends into
a routed wrapper's children instead of casting the wrapper itself. When a
routine owns the name, the call then resolves exactly like the flat call and
keeps the routine's own type and collation. When the stock built-in owns it,
the wrapper materializes to a JSON_ARRAY/JSON_OBJECT expression, which is
itself a default-string producer, and the collation pass that runs in every
analyzer iteration recolors it as before.
**Scope:** Give routed constructor wrappers a representation that does not
claim a default string type, and pin down shadowed calls under a default
collation with tests.
**Compatibility:** Default-collation recoloring of stock JSON_ARRAY and
JSON_OBJECT results, and the exemption for an explicitly collated RETURNING
type, must remain unchanged.
**Risks:** A routed wrapper that waits for its children, for example inside
a lambda, is recolored only after it materializes rather than while it is
unresolved. Because the collation pass runs in every iteration the resolved
plan should not change, and restoring the trait on routed wrappers rolls the
change back.
**Constraints:** Stock-owned wrappers must keep reporting their RETURNING
type, so an explicit RETURNING STRING COLLATE c is still not recolored to the
object or view default.
**Success:** Under a table or view default collation, a nested clause-free
JSON_ARRAY or JSON_OBJECT call owned by a temporary, PATH, persistent or V2
routine, or by an injected replacement, returns that owner's own result type
and collation, the same as the equivalent flat call. Without a shadow, the same
nested calls under a default collation still splice and still produce a result
in the object or view default collation. The fixed-point and single-pass
analyzers produce the same plan for these queries.
**Verification:**
- **Regression:** Under DEFAULT COLLATION UTF8_LCASE, both for a CTAS and
for a view, json_array(json_array(1)) and json_object('a', json_object('b', 1))
owned by an injected replacement or a temporary function that returns a
non-string keep that owner's result type and value, and an owner that returns
STRING keeps its own collation.
- **Behavior:** Without a shadow, the same nested calls under the same
default collations still return spliced JSON whose type carries the default
collation.
- **Compatibility:** The view-based cases analyze identically in the
fixed-point and single-pass analyzers when both run side by side.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -709,6 +709,7 @@ class Analyzer(
ResolveAliases ::
ResolveSubquery ::
ResolveSubqueryColumnAliases ::
+ ResolveJsonConstructors ::
Review Comment:
**Blocking (P1):** Every clause-bearing, explicit-FORMAT-JSON, or nested
JSON_ARRAY/JSON_OBJECT now reaches the analyzer as a wrapper that only this
rule materializes. The rule runs after ResolveFunctions (702) and
ResolveAliases (709), and only once all of the wrapper's children are resolved.
In the merge target these were `JsonArray`/`JsonObjectExpr` nodes that
ResolveTimeZone completed in iteration 1 regardless of their children, so one
bottom-up ResolveFunctions pass finished any nesting. There are two
consequences, both for queries with no qualified or shadowed constructor.
First, an ordinary function between levels, as in
`JSON_ARRAY(upper(JSON_ARRAY(upper(JSON_ARRAY(1 RETURNING STRING)) RETURNING
STRING)) RETURNING STRING)`, peels one level per iteration, so about 100
alternations exceed the default `spark.sql.analyzer.maxIterations` and fail
analysis where master succeeds. Second, a single wrapper whose child resolves
late in an iteration costs an extra full Resolution ite
ration; this happens with time-zone-aware children such as `date_format` or a
`CAST`, with children that need type coercion, and with lambda bodies. For
example, `JSON_OBJECT('d' VALUE date_format(ts, 'yyyy') NULL ON NULL)` gets its
alias one iteration later than in master. The bounded-passes test covers only
direct constructor-in-constructor nesting, which this rule's bottom-up
traversal does handle in one pass. The single-pass resolver is unaffected.
**Recommended change:** Route or materialize SQL/JSON constructor wrappers
within the same bottom-up pass that resolves function calls, before alias
assignment, rather than in a separate rule that runs after them. Add JSON_ARRAY
and JSON_OBJECT analyzer tests that alternate ordinary functions with
constructor levels under a low analyzer iteration limit.
**Why this works:** When the function-resolution pass reaches a wrapper
whose name a routine owns, it turns the wrapper into an ordinary call right
away, without waiting for its children, as the current rule does, so the call
resolves exactly like the flat call. A stock-owned wrapper still rejects a
direct star, and once its children are resolved the pass materializes the stock
built-in with the session time zone, so the constructor is already resolved
when the pass reaches the call that encloses it. An alternating chain of
functions and constructors then resolves in a single pass. A wrapper whose
children became resolved in the previous iteration materializes before aliases
are assigned, just as the merge target's parse-time node did.
**Scope:** Move wrapper routing and materialization into the
function-resolution traversal, and cover alternating nesting.
**Risks:** Function resolution takes on wrapper handling, so a mistake there
would affect every function call. The new handling must be pruned by its tree
pattern and must not intercept ordinary function calls. Restoring the separate
rule rolls the change back.
**Constraints:** Ownership must still be decided once per wrapper, and the
direct-star rejection and star-owner selection must stay as they are. Routing a
routine-owned wrapper must not wait for its children, so a routine still
receives the call while its arguments resolve, as it does for the flat call.
**Success:** Alternating nesting of an ordinary function and a
clause-bearing or nested JSON constructor, deeper than the analyzer iteration
limit, analyzes successfully, and its iteration count does not grow with depth.
A clause-bearing JSON_ARRAY or JSON_OBJECT whose child becomes resolved late in
an iteration needs no more analyzer iterations than the merge target needed.
**Verification:**
- **Regression:** With spark.sql.analyzer.maxIterations deliberately set
low, a chain of upper(JSON_ARRAY(... RETURNING STRING)) levels and a
JSON_OBJECT VALUE ... NULL ON NULL chain, each nested deeper than that limit,
analyze and return the expected JSON in fixed-point mode.
- **Behavior:** Existing splice, shadowing, star, lambda, subquery,
grouping, and default-collation behavior of both constructors is unchanged in
both analyzers.
- **Regression:** A clause-bearing JSON_OBJECT or JSON_ARRAY whose child is
resolved only late in an iteration, such as JSON_OBJECT('d' VALUE
date_format(ts, 'yyyy') NULL ON NULL) or a value under CAST(... AS STRING),
needs no more Resolution-batch iterations than the merge target, checked
through the per-rule invocation counts in QueryPlanningTracker.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/resolver/ResolverGuard.scala:
##########
@@ -629,7 +629,8 @@ class ResolverGuard(
// JSON
case _: GetJsonObject | _: JsonTuple | _: JsonToStructs | _:
StructsToJson |
_: SchemaOfJson | _: JsonObjectKeys | _: LengthOfJsonArray | _:
JsonArray |
- _: JsonExists | _: JsonQuery | _: JsonValue | _: JsonObjectExpr =>
+ _: JsonExists | _: JsonQuery | _: JsonValue | _: JsonObjectExpr |
+ _: UnresolvedJsonConstructor =>
Review Comment:
**Nit (P3):** A flat `json_array(1)` goes through `checkUnresolvedFunction`,
which rejects it as a non-builtin function when a temporary function owns the
name, so it stays on the fixed-point analyzer. A routed wrapper such as
`json_array(json_array(1))` or `json_object('a' VALUE json_object('b' VALUE
1))` is admitted here with only its children checked, and `ExpressionResolver`
then resolves it against the temporary owner. In tentative single-pass mode and
in the dual-run comparison (enabled in tests), temp-function calls therefore
reach the single-pass resolver, which the guard exists to prevent. This can
surface spurious single-pass failures or plan mismatches for non-SQL temp
functions. Strict single-pass and default production analysis are unaffected.
For wrappers with `routeThroughResolution`, delegating to
`checkUnresolvedFunction(j.toUnresolvedFunction)` would give them the same
decision as the flat call, and stock-owned wrappers can stay in this arm.
**Verification:**
- **Regression:** The guard rejects a nested constructor call shadowed by a
temporary function as a non-builtin function, matching the flat call.
- **Behavior:** The guard still admits unshadowed nested, clause-bearing and
explicit FORMAT JSON forms of both constructors.
##########
sql/core/src/test/scala/org/apache/spark/sql/JsonArraySuite.scala:
##########
@@ -646,23 +676,176 @@ class JsonArraySuite extends QueryTest with
SharedSparkSession {
checkAnswer(sql("SELECT system.builtin.json_array(1, 'x')"),
Row("""[1,"x"]"""))
}
- test("a nested JSON-producing argument through a routed JSON_ARRAY call is
quoted, not spliced") {
- // A routed call carries no lexical FORMAT JSON, so a nested
JSON-producing argument (a JSON
- // constructor like json_array or a path function like json_query) is
quoted as a plain value,
- // unlike the JSON_ARRAY(...) grammar which splices it (see the unqualified
- // `json_array(json_array(1))` -> `[[1]]` cases above). The outer call
routes whenever it sees
- // no lexical implicit-JSON element: `JsonArray.isImplicitlyJson`
recognizes only a
- // grammar-built nested JSON expression, so a qualified outer call and an
unqualified call over
- // a qualified nested JSON expression (whose child is still unresolved)
both route and quote.
- // Only a fully unqualified nested JSON expression stays on the direct
grammar path; splicing
- // through a routed call is left as a follow-up.
- checkAnswer(sql("SELECT builtin.json_array(json_array(1))"),
Row("""["[1]"]"""))
- checkAnswer(sql("SELECT system.builtin.json_array(json_array(1))"),
Row("""["[1]"]"""))
- checkAnswer(sql("SELECT builtin.json_array(json_array(1), 2)"),
Row("""["[1]",2]"""))
- checkAnswer(sql("SELECT json_array(builtin.json_array(1))"),
Row("""["[1]"]"""))
+ test("SPARK-59243: a nested JSON constructor through a routed JSON_ARRAY
call is spliced") {
+ // A routed or qualified built-in JSON_ARRAY splices a lexically-nested
JSON constructor
+ // argument, matching the JSON_ARRAY(...) grammar
(`json_array(json_array(1))` -> `[[1]]`).
+ // The built-in builder derives the FORMAT JSON flag from the resolved
argument via
+ // `ImplicitlyFormattedAsJson.splicesThroughCollationCast`, so a qualified
outer call and an
+ // unqualified call over a qualified nested constructor both splice,
matching the fully
+ // unqualified grammar form.
+ checkAnswer(sql("SELECT builtin.json_array(json_array(1))"), Row("[[1]]"))
+ checkAnswer(sql("SELECT system.builtin.json_array(json_array(1))"),
Row("[[1]]"))
+ checkAnswer(sql("SELECT builtin.json_array(json_array(1), 2)"),
Row("[[1],2]"))
+ checkAnswer(sql("SELECT json_array(builtin.json_array(1))"), Row("[[1]]"))
+ checkAnswer(sql("SELECT json_array(json_array(0),
builtin.json_array(1))"), Row("[[0],[1]]"))
checkAnswer(
sql("""SELECT builtin.json_array(json_query('{"a":{"x":1}}', '$.a'))"""),
- Row("""["{\"x\":1}"]"""))
+ Row("""[{"x":1}]"""))
+ // A user CAST still detaches FORMAT JSON, so the element stays quoted
(the routed splice check
+ // sees through an analyzer collation Cast, but not a user `CAST(... AS
STRING)`).
+ checkAnswer(sql("SELECT builtin.json_array(CAST(json_array(1) AS
STRING))"), Row("""["[1]"]"""))
+ }
+
+ test("SPARK-59243: clause-bearing JSON_ARRAY splices qualified nested
constructors") {
+ // Clause-bearing direct forms do not route through function resolution,
but still need to
+ // recover the qualified nested constructor after children resolve.
+ checkAnswer(sql("SELECT json_array(builtin.json_array(1) NULL ON NULL)"),
Row("[[1]]"))
+ checkAnswer(sql("SELECT json_array(builtin.json_array(1) RETURNING
STRING)"), Row("[[1]]"))
+ checkAnswer(
+ sql("SELECT json_array(builtin.json_array(1) FORMAT JSON NULL ON NULL)"),
+ Row("[[1]]"))
+ checkAnswer(
+ sql("SELECT json_array(CAST(builtin.json_array(1) AS STRING) NULL ON
NULL)"),
+ Row("""["[1]"]"""))
+
+ Seq(false, true).foreach { singlePass =>
+ withSQLConf(
+ SQLConf.ANALYZER_SINGLE_PASS_RESOLVER_ENABLED.key ->
singlePass.toString) {
+ val analyzed =
+ sql("SELECT json_array(builtin.json_array(1) FORMAT JSON NULL ON
NULL)")
+ .queryExecution.analyzed
+ assert(analyzed.resolved, s"singlePass=$singlePass")
+ val arrays = analyzed.expressions.flatMap(_.collect { case j:
JsonArray => j })
+ val outer = arrays.find(_.nullBehavior ==
JsonConstructorNullBehavior.Null).get
+ assert(outer.formatJson == Seq(true), s"singlePass=$singlePass")
+ assert(outer.needsValidation == Seq(false), s"singlePass=$singlePass")
+ }
+ }
+ }
+
+ test("SPARK-59243: routed JSON_ARRAY splice round-trips through canonical
SQL") {
+ // The analyzed expression's canonical SQL must reparse to the same
spliced result.
+ Seq(
+ "SELECT builtin.json_array(json_array(1))",
+ "SELECT json_array(builtin.json_array(1))").foreach { query =>
+ val analyzed = sql(query).queryExecution.analyzed
+ val jsonArray = analyzed.expressions.flatMap(_.collect { case j:
JsonArray => j }).head
+ checkAnswer(sql(s"SELECT ${jsonArray.sql}"), Row("[[1]]"))
+ }
+ }
+
+ test("SPARK-59243: spliced nested JSON_ARRAY canonical SQL stays built-in
under a shadow") {
+ // Canonical SQL of a resolved built-in must reparse to the built-in even
under a shadow: the
+ // nested splice now routes, so each clause-free level carries an
ownership clause.
+ val inner = JsonArray(
+ Seq(Literal(1)), Seq(false), Seq(false),
JsonConstructorNullBehavior.Absent, StringType)
+ val canonical = JsonArray(
+ Seq(inner), Seq(true), Seq(false), JsonConstructorNullBehavior.Absent,
StringType).sql
+ withSQLConf(
+ SQLConf.PATH_ENABLED.key -> "true",
+ SQLConf.SESSION_FUNCTION_RESOLUTION_ORDER.key -> "second") {
+ try {
+ sql("CREATE TEMPORARY FUNCTION json_array(a STRING) RETURNS STRING
RETURN 'shadowed'")
+ sql("SET PATH = system.session, system.builtin")
+ checkAnswer(sql(s"SELECT $canonical"), Row("[[1]]"))
+ } finally {
+ sql("SET PATH = DEFAULT_PATH")
+ sql("DROP TEMPORARY FUNCTION IF EXISTS json_array")
+ }
+ }
+ }
+
+ test("SPARK-59243: an explicit collation-changing string() cast detaches
splicing") {
+ // `string(... COLLATE c)` is an explicit cast (via the cast alias) that
detaches FORMAT JSON,
+ // so the nested constructor is quoted, not spliced. The
analyzer-collation-cast unwrap in
+ // `splicesThroughCollationCast` must not treat it as an
`ApplyDefaultCollation` cast.
+ checkAnswer(
+ sql("SELECT builtin.json_array(string(json_array(1) COLLATE
UTF8_LCASE))"),
+ Row("""["[1]"]"""))
+ // Also when the cast itself changes the collation: it is still a user
cast, not a
+ // default-collation recoloring, on both the routed and the direct grammar
paths.
+ val nested = "string(json_array(1 RETURNING STRING COLLATE UTF8_LCASE))"
+ checkAnswer(sql(s"SELECT builtin.json_array($nested)"), Row("""["[1]"]"""))
+ checkAnswer(sql(s"SELECT json_array($nested NULL ON NULL)"),
Row("""["[1]"]"""))
+ }
+
+ test("SPARK-59243: a nested clause-free JSON_ARRAY call is shadowed like a
flat call") {
+ withSQLConf(
+ SQLConf.PATH_ENABLED.key -> "true",
+ SQLConf.SESSION_FUNCTION_RESOLUTION_ORDER.key -> "second") {
+ try {
+ sql("CREATE TEMPORARY FUNCTION json_array(a STRING) RETURNS STRING
RETURN 'shadowed'")
+ sql("SET PATH = system.session, system.builtin")
+ // The inner json_array(1) is a grammar element (built-in) producing
"[1]"; the outer
+ // clause-free call routes, so the temp shadows it.
+ checkAnswer(sql("SELECT json_array(json_array(1))"), Row("shadowed"))
+ // Strict single-pass routes the outer clause-free call to the shadow
owner too, but the
+ // owner is a SQL function, which the single-pass analyzer does not
yet resolve: it reaches
+ // that owner (proving the routing) and then aborts with an
+ // ExplicitlyUnsupportedResolverFeature on the SQL function body. The
guard-gated modes fall
+ // back to fixed-point, which produces the shadow result asserted
above.
+ withSQLConf(SQLConf.ANALYZER_SINGLE_PASS_RESOLVER_ENABLED.key ->
"true") {
+ intercept[ExplicitlyUnsupportedResolverFeature] {
+ sql("SELECT json_array(json_array(1))").queryExecution.analyzed
+ }
+ }
+ // Qualifying every level selects Spark's constructor and splices.
+ checkAnswer(sql("SELECT builtin.json_array(builtin.json_array(1))"),
Row("[[1]]"))
+ } finally {
+ sql("SET PATH = DEFAULT_PATH")
+ sql("DROP TEMPORARY FUNCTION IF EXISTS json_array")
+ }
+ }
+ }
+
+ test("SPARK-59243: nested constructors splice in subqueries, grouping, and
INSERT VALUES") {
+ // The finalization rule runs over every operator, not just a top-level
Project.
+ checkAnswer(sql("SELECT (SELECT builtin.json_array(json_array(1)))"),
Row("[[1]]"))
+ checkAnswer(
+ sql("SELECT 1 WHERE '[[1]]' IN (SELECT
json_array(builtin.json_array(1)))"),
+ Row(1))
+ checkAnswer(
+ sql("SELECT json_array(json_array(count(*))) FROM VALUES (1), (2) AS
t(a)"),
+ Row("[[2]]"))
+ // HAVING and ORDER BY are checked separately: combining them with GROUP
BY hits an unrelated
+ // dual-run plan mismatch for any select-list expression.
+ val grouped = "FROM VALUES (1), (2), (2) AS t(a) GROUP BY a"
+ checkAnswer(
+ sql(s"SELECT builtin.json_array(json_array(a)) AS j $grouped " +
+ "HAVING builtin.json_array(json_array(count(*))) = '[[2]]'"),
+ Row("[[2]]"))
+ checkAnswer(
+ sql(s"SELECT builtin.json_array(json_array(a)) AS j $grouped " +
+ "ORDER BY json_array(builtin.json_array(a))"),
+ Seq(Row("[[1]]"), Row("[[2]]")))
+ withTable("t") {
+ sql("CREATE TABLE t (j STRING) USING parquet")
+ sql("INSERT INTO t VALUES (builtin.json_array(json_array(1))),
(json_object('a', " +
+ "builtin.json_object('b', 1)))")
+ checkAnswer(sql("SELECT j FROM t"), Seq(Row("[[1]]"),
Row("""{"a":{"b":1}}""")))
+ }
+ }
+
+ test("SPARK-59243: a persistent function cannot reference a temp shadow via
a nested call") {
+ withSQLConf(
+ SQLConf.PATH_ENABLED.key -> "true",
+ SQLConf.SESSION_FUNCTION_RESOLUTION_ORDER.key -> "second") {
+ try {
+ sql("CREATE TEMPORARY FUNCTION json_array(a STRING) RETURNS STRING
RETURN 'shadowed'")
+ sql("SET PATH = system.session, system.builtin")
+ // The routed nested call is a reference to the temp shadow, like the
flat call.
+ Seq("json_array(1)", "json_array(json_array(1))").foreach { body =>
+ val e = intercept[AnalysisException] {
+ sql(s"CREATE FUNCTION perm_json_fn() RETURNS STRING RETURN $body")
Review Comment:
**Nit (P3):** The new `CreateSQLFunctionCommand` case excludes stock-owned
wrappers only through its `routeThroughResolution` check. This test and its
JsonObjectSuite counterpart cover only routed bodies that must fail. In master,
stock-owned forms parsed to resolved `JsonArray`/`JsonObjectExpr` and were
never inspected. If that check were dropped, `CREATE FUNCTION perm() RETURNS
STRING RETURN json_array(1 NULL ON NULL)` would start failing with
INVALID_TEMP_OBJ_REFERENCE whenever a temporary `json_array` exists, and no
test would notice. Accepted cases such as `json_array(1 NULL ON NULL)`,
`json_array(json_array(1) NULL ON NULL)`, and `json_object('id' VALUE '[1]'
FORMAT JSON)` next to the rejected ones would pin it down.
See **Shared repair plan 2** in the review body.
##########
sql/core/src/test/scala/org/apache/spark/sql/JsonArraySuite.scala:
##########
@@ -549,11 +579,11 @@ class JsonArraySuite extends QueryTest with
SharedSparkSession {
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]]"))
+ // A nested clause-free call routes too, so it is shadowed like a flat
call (see the
+ // SPARK-59243 nested-shadowing test). Qualifying every level keeps
the built-in splice.
+ checkAnswer(sql("SELECT builtin.json_array(builtin.json_array(1))"),
Row("[[1]]"))
checkAnswer(
- sql("""SELECT json_array(json_query('{"a":{"x":1}}', '$.a'))"""),
+ sql("""SELECT builtin.json_array(builtin.json_query('{"a":{"x":1}}',
'$.a'))"""),
Review Comment:
**Nit (P3):** Before this change, the test asserted that
`json_array(json_query(...))` still splices the built-in `json_query` while
temporary `json_array` and `json_query` routines are on the PATH. The
replacement, `builtin.json_array(builtin.json_query(...))`, tests qualified
routing rather than direct-argument ownership. That was the only assertion that
a JSON_QUERY written as a direct constructor argument stays the stock built-in
under a `json_query` shadow (the `isDirectJsonConstructorArgument` check in
`visitJsonQuery`). If that rule regressed, the inner call would route to the
temporary `json_query`, and the non-shadow tests would still pass because the
builders recover the splice from a resolved stock JsonQuery. An assertion that
keeps the outer call stock-owned, for example `json_array(json_query(...) NULL
ON NULL)` as the neighboring json_value/json_exists cases do, would restore the
signal.
See **Shared repair plan 2** in the review body.
##########
sql/core/src/test/scala/org/apache/spark/sql/JsonArraySuite.scala:
##########
@@ -954,6 +1226,23 @@ class JsonArraySuite extends QueryTest with
SharedSparkSession {
}
}
+ test("SPARK-59243: routed/qualified nested JSON_ARRAY splices under a
default collation") {
+ // Both columns resolve through the built-in builder (not the direct
grammar path): col a via
+ // the qualified outer call, col b via the qualified nested call.
`ApplyDefaultCollation` wraps
+ // the nested producer in a collation Cast; the builder must still see
through it (an analyzer
+ // Cast, not a user one) and splice, so the result matches the UTF8_BINARY
case rather than
+ // quoting.
+ withSQLConf(SQLConf.OBJECT_LEVEL_COLLATIONS_ENABLED.key -> "true") {
+ withTable("t") {
+ sql(
+ """CREATE TABLE t DEFAULT COLLATION UTF8_LCASE AS
Review Comment:
**Nit (P3):** This test and its JsonObjectSuite counterpart check
default-collation splicing only through CTAS followed by `spark.table`.
ResolverGuard has no `CreateTableAsSelect` arm, so dual-run never runs these
queries in single-pass, and reading the table returns persisted values without
re-analysis. If the Cast that `DefaultCollationTypeCoercion` inserts in
single-pass stopped matching `isAnalyzerCollationCast`, single-pass would quote
`builtin.json_array(json_array(1))` in a default-collation view while
fixed-point still splices, and these tests would still pass. A `CREATE VIEW ...
DEFAULT COLLATION UTF8_LCASE` variant read under dual-run, like the existing
'view default collation preserves an explicit collated RETURNING' test, would
cover that path.
See **Shared repair plan 1** in the review body.
##########
sql/core/src/test/scala/org/apache/spark/sql/JsonArraySuite.scala:
##########
@@ -646,23 +676,176 @@ class JsonArraySuite extends QueryTest with
SharedSparkSession {
checkAnswer(sql("SELECT system.builtin.json_array(1, 'x')"),
Row("""[1,"x"]"""))
}
- test("a nested JSON-producing argument through a routed JSON_ARRAY call is
quoted, not spliced") {
- // A routed call carries no lexical FORMAT JSON, so a nested
JSON-producing argument (a JSON
- // constructor like json_array or a path function like json_query) is
quoted as a plain value,
- // unlike the JSON_ARRAY(...) grammar which splices it (see the unqualified
- // `json_array(json_array(1))` -> `[[1]]` cases above). The outer call
routes whenever it sees
- // no lexical implicit-JSON element: `JsonArray.isImplicitlyJson`
recognizes only a
- // grammar-built nested JSON expression, so a qualified outer call and an
unqualified call over
- // a qualified nested JSON expression (whose child is still unresolved)
both route and quote.
- // Only a fully unqualified nested JSON expression stays on the direct
grammar path; splicing
- // through a routed call is left as a follow-up.
- checkAnswer(sql("SELECT builtin.json_array(json_array(1))"),
Row("""["[1]"]"""))
- checkAnswer(sql("SELECT system.builtin.json_array(json_array(1))"),
Row("""["[1]"]"""))
- checkAnswer(sql("SELECT builtin.json_array(json_array(1), 2)"),
Row("""["[1]",2]"""))
- checkAnswer(sql("SELECT json_array(builtin.json_array(1))"),
Row("""["[1]"]"""))
+ test("SPARK-59243: a nested JSON constructor through a routed JSON_ARRAY
call is spliced") {
+ // A routed or qualified built-in JSON_ARRAY splices a lexically-nested
JSON constructor
+ // argument, matching the JSON_ARRAY(...) grammar
(`json_array(json_array(1))` -> `[[1]]`).
+ // The built-in builder derives the FORMAT JSON flag from the resolved
argument via
+ // `ImplicitlyFormattedAsJson.splicesThroughCollationCast`, so a qualified
outer call and an
+ // unqualified call over a qualified nested constructor both splice,
matching the fully
+ // unqualified grammar form.
+ checkAnswer(sql("SELECT builtin.json_array(json_array(1))"), Row("[[1]]"))
+ checkAnswer(sql("SELECT system.builtin.json_array(json_array(1))"),
Row("[[1]]"))
+ checkAnswer(sql("SELECT builtin.json_array(json_array(1), 2)"),
Row("[[1],2]"))
+ checkAnswer(sql("SELECT json_array(builtin.json_array(1))"), Row("[[1]]"))
+ checkAnswer(sql("SELECT json_array(json_array(0),
builtin.json_array(1))"), Row("[[0],[1]]"))
checkAnswer(
sql("""SELECT builtin.json_array(json_query('{"a":{"x":1}}', '$.a'))"""),
- Row("""["{\"x\":1}"]"""))
+ Row("""[{"x":1}]"""))
+ // A user CAST still detaches FORMAT JSON, so the element stays quoted
(the routed splice check
+ // sees through an analyzer collation Cast, but not a user `CAST(... AS
STRING)`).
+ checkAnswer(sql("SELECT builtin.json_array(CAST(json_array(1) AS
STRING))"), Row("""["[1]"]"""))
Review Comment:
**Nit (P3):** `isAnalyzerCollationCast` relies on
`!c.containsTag(Cast.USER_SPECIFIED_CAST)` to keep a user cast that changes the
collation from being unwrapped and spliced. This test and the other user-cast
tests all cast to the plain STRING type with no default collation, so `to !=
from` rejects the cast before the tag is read. The `string(...)` tests are
rejected earlier by the Collate-child and explicitly-collated-child conditions.
If the tag check were removed, `builtin.json_array(CAST(json_array(1) AS STRING
COLLATE UTF8_LCASE))` would splice to `[[1]]` instead of quoting, as would
`CAST(json_array(1) AS STRING)` inside a DEFAULT COLLATION UTF8_LCASE CTAS, and
nothing would fail. A case where the tag alone decides the outcome would cover
this guard.
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]
