cloud-fan commented on code in PR #59828:
URL: https://github.com/apache/spark/pull/59828#discussion_r4421746435
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -2594,18 +2621,39 @@ class Analyzer(
case q: LogicalPlan =>
q.transformExpressionsUpWithPruning(
- _.containsAnyPattern(UNRESOLVED_FUNCTION, GENERATOR),
+ _.containsAnyPattern(UNRESOLVED_FUNCTION, GENERATOR,
UNRESOLVED_JSON_CONSTRUCTOR),
ruleId) {
- case u @ UnresolvedFunction(nameParts, arguments, _, _, _, _, _, _)
- if functionResolution.hasLambdaAndResolvedArguments(arguments)
=> withPosition(u) {
- functionResolution.resolveFunction(u) match {
- case func: HigherOrderFunction => func
- case other => other.failAnalysis(
- errorClass =
"INVALID_LAMBDA_FUNCTION_CALL.NON_HIGHER_ORDER_FUNCTION",
- messageParameters = Map(
- "class" -> other.getClass.getCanonicalName))
+ case u @ UnresolvedFunction(_, arguments, _, _, _, _, _, _)
+ if functionResolution.hasLambdaAndResolvedArguments(arguments) =>
+ resolveHigherOrderFunction(u)
+
+ // Decide a SQL/JSON constructor's owner in this same bottom-up
pass, so an enclosing call
+ // and its alias see the result without an extra iteration per
nesting level.
+ case j: UnresolvedJsonConstructor
+ if functionResolution.routesJsonConstructorToFunction(j) =>
Review Comment:
**Non-blocking (P2):** The ownership probe in this guard runs before the
`!j.childrenResolved` wait below, so a routed wrapper that the built-in owns
repeats the SQL PATH walk each time `ResolveFunctions` revisits it while its
children are unresolved (for example inside a lambda, or with outer or lateral
references), and once more when it finalizes. A wrapper that a routine owns
walks the PATH here and again in `resolveFunction`. The equivalent flat call is
skipped until its children resolve, and `LookupFunctions` resolves the
single-part built-in name in memory, so it walks the PATH once.
With `SET PATH = cat.fns, system.builtin`, `transform(arr, x ->
json_array(json_array(x)))` makes two uncached `functionExists` calls on
`cat.fns`, where `transform(arr, x -> json_array(x))` makes one `loadFunction`
call. This only costs analysis time, and under the default PATH each walk stops
at `system.builtin` in memory.
**Recommended change:** When the ownership check finds that the built-in
owns a routed nested JSON_ARRAY or JSON_OBJECT call, replace the routed wrapper
with its stock-owned counterpart, carrying the same values, splice flags, and
clause-free defaults. Later iterations then finalize it without checking the
SQL PATH again. Routine-owned wrappers still become an ordinary call right away.
**Why this works:** Ownership is decided once per routed call, as in the
direct-star path and the single-pass resolver. The stock-owned counterpart then
waits for its children without touching the catalog.
**Scope:** One-time ownership decision for routed JSON constructor wrappers
in fixed-point function resolution.
**Compatibility:** Routine and injected-replacement ownership of nested
clause-free JSON constructor calls.
**Risks:** While it waits for its children, the stock-owned counterpart
reports its RETURNING type to default-collation rules, as clause-bearing stock
wrappers already do. That is correct only because the built-in has already been
chosen as owner. Reverting the change restores per-iteration checking.
**Constraints:** A routine-owned or injected-replacement-owned wrapper must
still become an ordinary call without waiting for its children. A routed
wrapper must not be presented as a default-string producer before its owner is
decided.
**Exclusions:**
- The second PATH walk when a routine owns the call. Removing it would need
resolution to reuse the owner the probe found, which would change the flat
call's fallback to the built-in when a shadow disappears between the probe and
resolution.
**Success:** A routed nested call owned by the built-in checks the SQL PATH
once per analysis, however many iterations its children take to resolve.
Routine-owned and injected-replacement-owned nested calls resolve exactly as
before.
**Verification:**
- **Regression:** With a SQL PATH that lists a lookup-counting function
catalog ahead of system.builtin, a nested clause-free JSON_ARRAY that the
built-in owns, inside a lambda body, makes no more catalog function lookups
(existence checks and loads combined) than the equivalent flat call.
- **Compatibility:** Existing splice, shadowing, direct-star, lambda, and
default-collation results for routed nested calls are unchanged in both
analyzers.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/jsonExpressions.scala:
##########
@@ -1314,6 +1315,60 @@ 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`, in any interleaving with
`Collate`: under an
+ * object/view default collation, `ApplyDefaultCollation` wraps a nested
producer (also one under
+ * a user `COLLATE`) in a `Cast` to the default `StringType` before the
builder runs, which would
+ * otherwise hide the producer and quote it. The stock-owned grammar
wrappers apply the same
+ * predicate when they finalize their flags. 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: Collate => splicesThroughCollationCast(c.child)
+ case c: Cast if isAnalyzerCollationCast(c) =>
splicesThroughCollationCast(c.child)
+ case i: ImplicitlyFormattedAsJson => i.emitsImplicitJsonText
+ case _ => false
+ }
+
+ private def isAnalyzerCollationCast(c: Cast): Boolean = {
+ // `ApplyDefaultCollation` / `DefaultCollationTypeCoercion` wrap only a
companion-`StringType`
+ // `DefaultStringProducingExpression` (they skip explicitly collated
ones), so only that shape
+ // is an analyzer recoloring. User detachments stay quoted:
`string(json_array(1) COLLATE c)`
+ // (child is a `Collate`), `string(json_array(1 RETURNING STRING COLLATE
c))` (explicitly
+ // collated child), and `CAST(... AS STRING)` (`USER_SPECIFIED_CAST`).
Requiring a real
+ // collation change leaves the no-op `string(...)` cast alone.
+ if (!c.child.isInstanceOf[DefaultStringProducingExpression]) return false
+ (c.dataType, c.child.dataType) match {
+ case (to: StringType, from: StringType) =>
+ from.eq(StringType) && to != from &&
!c.containsTag(Cast.USER_SPECIFIED_CAST)
Review Comment:
**Blocking (P1):** This predicate tells a user `CAST(... AS STRING)` from an
analyzer collation cast only by `USER_SPECIFIED_CAST`, but the fixed-point
`ApplyDefaultCollation` drops that tag in a reachable shape. Under a
non-UTF8_BINARY table, view, or SQL-function default collation, the
already-cast arm of `castDefaultStringExpressions` rebuilds a retargeted user
cast with `cast.copy(...)` whenever a default-string producer below it gets its
own collation cast, and `withNewChildren` installs the untagged copy. Any
splice decision made after that pass, in the builders or in `finalizeFlags`,
then splices the value the user cast in order to quote it.
For example, `CREATE TABLE t DEFAULT COLLATION UTF8_LCASE AS SELECT
builtin.json_array(CAST(json_array(json_array(1)) AS STRING), (SELECT 1)) AS j`
stores `[[[1]],1]` instead of `["[[1]]",1]`. The scalar-subquery sibling defers
the outer call past the first `ApplyDefaultCollation` pass; without it the same
expression is decided earlier and stays quoted. The unqualified
`json_array(...)` call, the clause-bearing forms, and `JSON_OBJECT` behave the
same way. The single-pass analyzer keeps the tag (`CoercesExpressionTypes`
copies tags onto the coercion result) and quotes, so the two analyzers also
disagree for views.
**Recommended change:** Make the default-collation rule re-apply the
user-specified cast marker when it rebuilds an already-retargeted user cast
around recolored children, as the collation type-coercion rule does when it
retargets a cast. Copy only that marker, not every tag, because that rule notes
that a stale collation-context tag must not be carried over. Add JSON_ARRAY and
JSON_OBJECT default-collation tests in which the enclosing constructor's splice
decision is deferred by a sibling argument.
**Why this works:** With the marker preserved, a user CAST, TRY_CAST, or ::
around a nested JSON producer keeps reading as a user cast, so the splice
predicate quotes it no matter which analyzer iteration makes the decision.
**Scope:** Marker-preserving cast rebuild in the default-collation rule,
plus deferred-decision regression coverage for the JSON constructors.
**Compatibility:** Default-collation recoloring of JSON constructor results,
and splicing through casts the analyzer inserts for the default collation.
**Risks:** Auto-generated column names read the same marker, so a deferred
un-aliased column of this shape is renamed to include the user's CAST, which is
the name the same column already gets when nothing is deferred and in the
single-pass analyzer. This applies to any such column under a non-UTF8_BINARY
object or view default collation, not only to JSON constructors. A view created
on Spark 4.2.0 or later with such a column stored the old name, and reading a
view matches its stored column names against the query output, so it can fail
with an incompatible view schema error until it is recreated or the column is
aliased. CHAR/VARCHAR truncation is unaffected because only casts already
retargeted to the default STRING collation reach this rebuild. Reverting the
change restores the current behavior.
**Constraints:** Casts that the default-collation rules insert must stay
unmarked, so qualified and routed nested producers keep splicing under an
object or view default collation. Re-apply only the user-specified cast marker;
do not copy the collation-context tag.
**Exclusions:**
- Resolution rules outside the analysis package and rules injected through
session extensions, which the targeted search for marker-dropping cast rebuilds
did not cover.
**Success:** Under a table, view, or SQL-function default collation, a user
cast to STRING around a nested JSON producer stays quoted in JSON_ARRAY and
JSON_OBJECT whether the enclosing splice decision is made in the same analyzer
iteration or a later one. Qualified and routed nested producers without a user
cast still splice under a default collation. A deferred un-aliased column whose
user cast was rebuilt gets the same auto-generated name as when nothing defers
it, in both analyzers.
**Verification:**
- **Regression:** Under a table default collation and a view default
collation, an unqualified, a qualified, and a clause-bearing JSON_ARRAY whose
user-cast nested-constructor argument has a scalar-subquery sibling return the
quoted value in both analyzers, matching the same query without the sibling.
The same holds for JSON_OBJECT.
- **Compatibility:** Existing default-collation results stay the same for
qualified, routed, COLLATE-annotated, string(), and plain user-cast cases of
both constructors.
- **Compatibility:** Under a view default collation, a deferred un-aliased
column whose user cast wraps a default-string producer with such a descendant
gets the same auto-generated name in the fixed-point analyzer as without the
deferring sibling, and as in the single-pass analyzer.
- **Behavior:** A user cast that the default-collation rule rebuilds around
recolored children still carries the user-specified marker, and an
analyzer-inserted cast it rebuilds stays unmarked.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/jsonExpressions.scala:
##########
@@ -2411,14 +2582,14 @@ case class JsonObjectKeyGuard(child: Expression)
extends UnaryExpression with No
* JSON_OBJECT('id': 7, 'v': NULL ABSENT ON NULL) -> '{"id":7}'
* JSON_OBJECT() -> '{}'
*
- * A clause-free call that is not itself lexically nested in an enclosing
constructor's *value*
- * position routes through function resolution and is rebuilt by
`JsonObjectExpressionBuilder`, so a
- * same-named routine can shadow the built-in `json_object`: always for the
comma form (a nested
- * producer value routes too, carrying its raw-splice eligibility via
`JsonImplicitFormatCarrier`),
- * but for the `VALUE` / `:` forms only when no member carries a raw value
(`rawJson.contains(true)`
- * -- an explicit `FORMAT JSON` or a nested producer in *value* position --
keeps it direct).
- * Otherwise it is built directly from the grammar (clause-bearing, or
value-nested); a nested
- * *key*-position call stays routable (see `AstBuilder.visitJsonObject`).
+ * A clause-free call without explicit `FORMAT JSON`, and not itself lexically
nested in an
+ * enclosing constructor's *value* position, routes through function
resolution so a same-named
+ * routine can shadow the built-in `json_object`. A parser-visible nested
producer value keeps its
+ * raw-splice eligibility in an [[UnresolvedJsonObject]]; a shadow receives
plain key/value
Review Comment:
**Nit (P3):** On the routed, shadowable path the parser builds
`UnresolvedRoutedJsonObject(members, rawJson)` in `AstBuilder.visitJsonObject`,
not `UnresolvedJsonObject`, which is the stock-owned wrapper that never routes.
As written, the sentence points a reader at the wrong class while describing
the call a shadow can own.
See **Shared repair plan 1** in the review body.
##########
sql/core/src/test/scala/org/apache/spark/sql/JsonArraySuite.scala:
##########
@@ -646,23 +681,197 @@ 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]"]"""))
+ // A user cast that changes the collation is told apart from an analyzer
collation Cast only by
+ // its USER_SPECIFIED_CAST tag, and must still detach.
+ checkAnswer(
+ sql("SELECT builtin.json_array(CAST(json_array(1) AS STRING COLLATE
UTF8_LCASE))"),
+ Row("""["[1]"]"""))
+ withSQLConf(SQLConf.OBJECT_LEVEL_COLLATIONS_ENABLED.key -> "true") {
+ withTable("t") {
+ sql("CREATE TABLE t DEFAULT COLLATION UTF8_LCASE AS " +
+ "SELECT builtin.json_array(CAST(json_array(1) AS STRING)) AS a")
+ checkAnswer(spark.table("t"), 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.
Review Comment:
**Nit (P3):** There is no longer a separate finalization rule:
`ResolveJsonConstructors` was removed, and `ResolveFunctions` (and
`ExpressionResolver` in the single-pass analyzer) now route or finalize the
wrappers. A reader looking for the rule this comment names will not find it.
See **Shared repair plan 2** in the review body.
##########
sql/core/src/test/scala/org/apache/spark/sql/JsonArraySuite.scala:
##########
@@ -954,6 +1283,77 @@ 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
Review Comment:
**Nit (P3):** The CTAS below now selects four columns. Column `c`,
`json_array(json_array(0), builtin.json_array(1) COLLATE UNICODE)`, parses to
`UnresolvedRoutedJsonArray` and is finalized through
`toBuiltin`/`finalizeFlags`, not through `JsonArrayExpressionBuilder`, so the
comment misstates which splice-predicate consumers this test covers.
See **Shared repair plan 2** in the review body.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/jsonExpressions.scala:
##########
@@ -1720,6 +1765,144 @@ 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.
+ *
+ * [[StockJsonConstructor]]s are owned by the stock built-in.
[[RoutedJsonConstructor]]s are
+ * clause-free calls a same-named routine (or an `injectFunction` replacement)
can shadow; they then
+ * resolve as a plain `prettyName(arguments...)` call.
+ */
+sealed trait UnresolvedJsonConstructor extends Unevaluable with
ImplicitlyFormattedAsJson {
+ def routeThroughResolution: Boolean
+ /** The flattened argument list a routed call passes to a same-named
routine. */
+ def arguments: Seq[Expression] = children
+ /** The stock built-in over the current children, with splice flags
finalized. */
+ def toBuiltin: TimeZoneAwareExpression
+ /** The ordinary call a routed constructor resolves as when a routine owns
the name. */
+ def toUnresolvedFunction: UnresolvedFunction =
CurrentOrigin.withOrigin(origin) {
+ UnresolvedFunction(prettyName, arguments, isDistinct = false)
+ }
+
+ override def dataType: DataType = throw new UnresolvedException("dataType")
+ override def nullable: Boolean = throw new UnresolvedException("nullable")
+ override lazy val resolved: Boolean = false
+ final override val nodePatterns: Seq[TreePattern] =
Seq(UNRESOLVED_JSON_CONSTRUCTOR)
+}
+
+/** An [[UnresolvedJsonConstructor]] the stock built-in owns (clause-bearing
or `FORMAT JSON`). */
+sealed trait StockJsonConstructor
+ extends UnresolvedJsonConstructor
+ with DefaultStringProducingExpression {
+ def returning: DataType
+ final override def routeThroughResolution: Boolean = false
+ // Report the actual RETURNING type (not the default from
`DefaultStringProducingExpression`) so
+ // `ApplyDefaultCollation` sees an explicit `RETURNING STRING COLLATE c` on
this still-unresolved
+ // node and does not recolor it to the object/view default.
+ override def dataType: DataType = returning
+
+ // A parser flag stays set; a resolved child that splices through a
collation cast adds one.
+ // An added splice is a trusted constructor, so it never needs validation.
+ protected def finalizeFlags(
+ parserFlags: Seq[Boolean],
+ needsValidation: Seq[Boolean],
+ values: Seq[Expression]): (Seq[Boolean], Seq[Boolean]) = {
+ val splices =
values.map(ImplicitlyFormattedAsJson.splicesThroughCollationCast)
+ (parserFlags.lazyZip(splices).map(_ || _),
needsValidation.lazyZip(splices).map(_ && !_))
+ }
+}
+
+/**
+ * An [[UnresolvedJsonConstructor]] whose owner is not known until resolution.
Deliberately not a
+ * `DefaultStringProducingExpression`: until a routine is ruled out it stands
for an ordinary call,
+ * so default-collation rules must not cast it (a shadow keeps its own result
type).
+ */
+sealed trait RoutedJsonConstructor extends UnresolvedJsonConstructor {
+ final override def routeThroughResolution: Boolean = true
+}
+
+case class UnresolvedJsonArray(
+ values: Seq[Expression],
+ formatJson: Seq[Boolean],
+ needsValidation: Seq[Boolean],
+ nullBehavior: JsonConstructorNullBehavior,
+ returning: DataType)
+ extends StockJsonConstructor {
+ assert(values.length == formatJson.length && values.length ==
needsValidation.length,
+ "UnresolvedJsonArray requires one formatJson and one needsValidation flag
per value")
+ assert(needsValidation.lazyZip(formatJson).forall((nv, fj) => !nv || fj),
+ "UnresolvedJsonArray needsValidation implies formatJson")
+ override def children: Seq[Expression] = values
+ override def prettyName: String = "json_array"
+ // Only the values are children, so `withNewChildren` (e.g. a
default-collation `Cast` wrapping a
+ // child) preserves the parse-time flags.
+ override protected def withNewChildrenInternal(
+ newChildren: IndexedSeq[Expression]): UnresolvedJsonArray = copy(values
= newChildren)
+
+ override def toBuiltin: JsonArray = {
+ val (finalFormatJson, finalNeedsValidation) = finalizeFlags(formatJson,
needsValidation, values)
+ JsonArray(values, finalFormatJson, finalNeedsValidation, nullBehavior,
returning)
+ }
+}
+
+/** A clause-free `JSON_ARRAY(...)` with a lexically nested constructor (see
[[JsonArray]]). */
+case class UnresolvedRoutedJsonArray(values: Seq[Expression], formatJson:
Seq[Boolean])
+ extends RoutedJsonConstructor {
+ assert(values.length == formatJson.length,
+ "UnresolvedRoutedJsonArray requires one formatJson flag per value")
+ override def children: Seq[Expression] = values
+ override def prettyName: String = "json_array"
+ override protected def withNewChildrenInternal(
+ newChildren: IndexedSeq[Expression]): UnresolvedRoutedJsonArray =
copy(values = newChildren)
+
+ override def toBuiltin: JsonArray = UnresolvedJsonArray(
+ values, formatJson, values.map(_ => false),
JsonConstructorNullBehavior.Absent, StringType
Review Comment:
**Nit (P3):** The routed wrappers now supply their own clause-free null
defaults: `ABSENT ON NULL` here and `NULL ON NULL` in
`UnresolvedRoutedJsonObject.toBuiltin`. Both are correct, but no test in
`JsonArraySuite`, `JsonObjectSuite`, or the JSON SQL tests runs a nested
clause-free call with a NULL value. A swapped default would turn
`json_array(json_array(1), NULL)` into `[[1],null]`, or drop the `"c"` key from
`json_object('a' VALUE json_object('b' VALUE 1), 'c' VALUE NULL)`, without any
test failing.
**Verification:**
- **Regression:** A clause-free JSON_ARRAY with a nested constructor and a
NULL element returns the array without the null, and a clause-free JSON_OBJECT
VALUE form with a nested constructor and a NULL value returns the key with
null, in both analyzers.
##########
sql/core/src/test/scala/org/apache/spark/sql/JsonObjectSuite.scala:
##########
@@ -1350,6 +1604,59 @@ world'))"""))
}
}
+ test("invalid: a direct star in VALUE/colon JSON_OBJECT forms is not
expanded") {
+ // Clause-free VALUE / colon forms route through function resolution, but
the stock built-in
+ // still rejects a direct member value `*`; only a shadowing routine may
receive it expanded.
+ Seq(
+ "SELECT json_object('a' VALUE *, 'b' VALUE json_object('c' VALUE 1)) " +
+ "FROM VALUES ('id', 7) AS t(a, b)",
+ "SELECT json_object('a': *, 'b': json_object('c': 1)) " +
+ "FROM VALUES ('id', 7) AS t(a, b)"
+ ).foreach { query =>
+ Seq(false, true).foreach { singlePass =>
+ withSQLConf(SQLConf.ANALYZER_SINGLE_PASS_RESOLVER_ENABLED.key ->
singlePass.toString) {
+ val e = intercept[AnalysisException] {
+ sql(query).queryExecution.analyzed
+ }
+ assert(e.getCondition == "INVALID_USAGE_OF_STAR_OR_REGEX",
+ s"singlePass=$singlePass for $query")
+ }
+ }
+ }
+ }
+
+ test("VALUE/colon JSON_OBJECT star expands for a shadow owner") {
+ withSQLConf(
+ SQLConf.PATH_ENABLED.key -> "true",
+ SQLConf.SESSION_FUNCTION_RESOLUTION_ORDER.key -> "second") {
+ try {
+ sql("CREATE TEMPORARY FUNCTION json_object(k1 STRING, v1 STRING, v2
INT, " +
+ "k2 STRING, v3 STRING) RETURNS STRING RETURN 'shadowed-5'")
+ sql("SET PATH = system.session, system.builtin")
+ Seq(
+ "SELECT json_object('a' VALUE *, 'b' VALUE json_object('c' VALUE 1))
" +
+ "FROM VALUES ('id', 7) AS t(a, b)",
+ "SELECT json_object('a': *, 'b': json_object('c': 1)) " +
+ "FROM VALUES ('id', 7) AS t(a, b)"
+ ).foreach { query =>
+ checkAnswer(sql(query), Row("shadowed-5"))
+ // Strict single-pass expands the nested star and routes to the
shadow owner too, but the
Review Comment:
**Nit (P3):** In both queries the `*` is a direct member value of the outer
call, which the previous test calls "a direct member value `*`". Elsewhere in
this PR, "nested star" means a star that belongs to an inner call such as
`array(*)` (for example in the `single-pass: routed JSON_OBJECT resolves and
expands a nested star` test below), and that star expands whoever owns the JSON
constructor. Calling this one "the nested star" makes the test read as covering
that case, rather than a direct star whose expansion depends on a shadow owning
the call.
See **Shared repair plan 2** in the review body.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -4391,25 +4391,32 @@ class AstBuilder extends DataTypeAstBuilder
*/
override def visitJsonArray(ctx: JsonArrayContext): Expression =
withOrigin(ctx) {
val arrayValues = ctx.values.asScala.map(v => expression(v.value)).toSeq
- // Route a flat, clause-free call through routine resolution so it can be
shadowed. Cheap
- // clause/nesting checks come first to skip the recursive per-value FORMAT
scans when a clause
- // already forces direct construction.
- val routeThroughResolution =
+ // A clause-free call (no RETURNING, no ON NULL, not a direct JSON
constructor argument, and no
+ // explicit FORMAT JSON on any value) routes through routine resolution so
a routine can shadow
+ // it. The cheap clause/nesting checks come first so the recursive
per-value implicit-JSON scan
+ // runs only when routing is still possible.
+ val clauseFree =
ctx.returning == null && ctx.nullBehavior == null &&
!isDirectJsonConstructorArgument(ctx) &&
- !ctx.values.asScala.exists(_.FORMAT() != null) &&
- !arrayValues.exists(JsonArray.isImplicitlyJson)
- if (routeThroughResolution) {
+ !ctx.values.asScala.exists(_.FORMAT() != null)
+ if (clauseFree &&
!arrayValues.exists(ImplicitlyFormattedAsJson.isImplicitlyJson)) {
UnresolvedFunction("json_array", arrayValues, isDistinct = false)
+ } else if (clauseFree) {
+ // Nested clause-free call: route so a routine can shadow it, but carry
the per-value splice
+ // decision, frozen now from the lexical argument, through resolution
(see
Review Comment:
**Nit (P3):** This says the per-value splice decision is "frozen now from
the lexical argument", and the direct-branch comment just below says a later
rewrite that wraps or swaps the child cannot change the flags. Both wrappers
now finalize through `StockJsonConstructor.finalizeFlags`, which ORs each
parser flag with `splicesThroughCollationCast` on the resolved child, so
analysis can still add a splice, for example to the second element of
`json_array(json_array(1), builtin.json_array(2))`. The parser flags are a
lower bound, as the updated `JsonArray` comments already say.
This extends an [existing
discussion](https://github.com/apache/spark/pull/59828#discussion_r4418287801).
See **Shared repair plan 1** in the review body.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/jsonExpressions.scala:
##########
@@ -1720,6 +1765,144 @@ 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.
+ *
+ * [[StockJsonConstructor]]s are owned by the stock built-in.
[[RoutedJsonConstructor]]s are
+ * clause-free calls a same-named routine (or an `injectFunction` replacement)
can shadow; they then
+ * resolve as a plain `prettyName(arguments...)` call.
+ */
+sealed trait UnresolvedJsonConstructor extends Unevaluable with
ImplicitlyFormattedAsJson {
+ def routeThroughResolution: Boolean
+ /** The flattened argument list a routed call passes to a same-named
routine. */
+ def arguments: Seq[Expression] = children
+ /** The stock built-in over the current children, with splice flags
finalized. */
+ def toBuiltin: TimeZoneAwareExpression
+ /** The ordinary call a routed constructor resolves as when a routine owns
the name. */
+ def toUnresolvedFunction: UnresolvedFunction =
CurrentOrigin.withOrigin(origin) {
+ UnresolvedFunction(prettyName, arguments, isDistinct = false)
+ }
+
+ override def dataType: DataType = throw new UnresolvedException("dataType")
+ override def nullable: Boolean = throw new UnresolvedException("nullable")
+ override lazy val resolved: Boolean = false
+ final override val nodePatterns: Seq[TreePattern] =
Seq(UNRESOLVED_JSON_CONSTRUCTOR)
+}
+
+/** An [[UnresolvedJsonConstructor]] the stock built-in owns (clause-bearing
or `FORMAT JSON`). */
Review Comment:
**Nit (P3):** The parser also builds a stock wrapper for a clause-free call
written directly as another constructor's value, such as the inner
`JSON_ARRAY(1)` in `JSON_ARRAY(JSON_ARRAY(1))`, because `visitJsonArray` and
`visitJsonObject` route only when `!isDirectJsonConstructorArgument(ctx)`. The
routing comment in `FunctionRegistry` has the same gap and also leaves out
`json_array` with explicit `FORMAT JSON`. A reader working out shadow
ownership, or the temp-function and cyclic-reference checks, from these lists
would conclude that value-nested calls can be shadowed, which they cannot.
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]
