ganeshashree commented on code in PR #58450:
URL: https://github.com/apache/spark/pull/58450#discussion_r3946357064
##########
sql/core/src/test/scala/org/apache/spark/sql/JsonArraySuite.scala:
##########
@@ -466,18 +492,272 @@ 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") {
+ // A routed (plain or qualified) call carries no lexical FORMAT JSON, so a
nested JSON
+ // constructor argument is treated as a plain value and quoted, unlike the
JSON_ARRAY(...)
+ // grammar which splices it (see the unqualified
`json_array(json_array(1))` -> `[[1]]` cases
+ // above). A nested constructor reaches the routed builder only via a
qualified outer call:
+ // an unqualified nested constructor 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 builtin.json_array(json_query('{"a":{"x":1}}', '$.a'))"""),
+ Row("""["{\"x\":1}"]"""))
+ }
+
+ test("invalid: a bare star argument in plain JSON_ARRAY is not expanded") {
+ Seq("json_array", "builtin.json_array",
"system.builtin.json_array").foreach { func =>
+ val e = intercept[AnalysisException] {
+ sql(s"SELECT $func(*) FROM VALUES (1, 'x') AS t(a, b)").collect()
+ }
+ assert(e.getCondition == "INVALID_USAGE_OF_STAR_OR_REGEX", s"for
$func(*)")
+ }
+ }
+
+ test("invalid: a bare star argument in clause-bearing JSON_ARRAY is not
expanded") {
+ val e = intercept[AnalysisException] {
+ sql("SELECT json_array(* NULL ON NULL) FROM VALUES (1, 'x') AS t(a,
b)").collect()
+ }
+ assert(e.getCondition == "INVALID_USAGE_OF_STAR_OR_REGEX")
+ }
+
+ test("JSON_ARRAY expands a star nested in a sibling constructor (array(*))")
{
+ // Only a bare `*` element is rejected. A star nested in `array(...)`
belongs to that call and
+ // is expanded there, exactly as `array(array(*))` would, then JSON_ARRAY
wraps the result.
+ checkAnswer(
+ sql("SELECT json_array(array(*)) FROM VALUES (1, 2) AS t(a, b)"),
+ Row("[[1,2]]"))
+ // Clause-bearing form (a direct-construction JsonArray node) behaves the
same.
+ checkAnswer(
+ sql("SELECT json_array(array(*) NULL ON NULL) FROM VALUES (1, 2) AS t(a,
b)"),
+ Row("[[1,2]]"))
+ // Alongside count(*): the array's star expands, count(*) is rewritten,
neither is rejected.
+ checkAnswer(
+ sql("SELECT json_array(count(*), array(max(a))) FROM VALUES (1), (2) AS
t(a)"),
+ Row("[2,[2]]"))
+ }
+
+ test("single-pass: JSON_ARRAY expands a star nested in array(*)") {
+ withSQLConf(SQLConf.ANALYZER_SINGLE_PASS_RESOLVER_ENABLED.key -> "true") {
+ Seq(
+ "SELECT json_array(array(*)) FROM VALUES (1, 2) AS t(a, b)",
+ "SELECT json_array(array(*) NULL ON NULL) FROM VALUES (1, 2) AS t(a,
b)"
+ ).foreach { query =>
+ // Analyze only: the single-pass analyzer cannot execute every
operator, so assert the
Review Comment:
Done.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionResolution.scala:
##########
@@ -404,6 +388,94 @@ 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. Parser-built `count(*)` is normalized to
`count(1)` in
+ * `AstBuilder` so it skips this probe, but a DataFrame `count("*")` keeps
its star and does reach
+ * the probe during analyzer normalization.
+ */
+ def unqualifiedFunctionResolvesToBuiltinBeforeAnyShadow(functionName:
String): Boolean = {
+ // Walk the PATH in order and stop at the first entry that owns the name.
The default order puts
+ // system.builtin first, so the common case returns on the first entry
with no catalog lookup;
+ // only a custom PATH that lists a persistent catalog ahead of
system.builtin reaches the probe
+ // below (one lookup per such preceding entry, recomputed on each call --
not cached).
+ sqlResolutionPathEntriesForAnalysis.foreach { pathEntry =>
+ val candidate = pathEntry :+ functionName
+ FunctionResolution.sessionNamespaceKind(candidate) match {
+ case
Some(org.apache.spark.sql.catalyst.catalog.SessionCatalog.Builtin) =>
+ return true
+ case Some(org.apache.spark.sql.catalyst.catalog.SessionCatalog.Temp) =>
+ // Honor stored-view temp visibility so this probe picks the same
owner the resolver
+ // would: a temp not captured by the view is hidden here too, just
as the persistent
+ // branch below expands through the view's frozen catalog.
+ if
(v1SessionCatalog.isTemporaryFunctionVisible(FunctionIdentifier(functionName)))
{
+ return false
+ }
+ case None =>
+ if (persistentFunctionExists(candidate)) {
+ return false
+ }
+ }
+ }
+ false
+ }
+
+ /**
+ * Returns true when a function reference resolves to the system built-in
with the requested name.
+ * This mirrors [[resolveFunction]] for special parser/analyzer rewrites
that must run only for
+ * Spark's built-ins. In particular, two-part `builtin.name` is not always a
system built-in:
+ * with `spark.sql.legacy.persistentCatalogFirst=true`, an existing
persistent
+ * `current_catalog.builtin.name` takes precedence.
+ */
+ def functionNameResolvesToBuiltin(nameParts: Seq[String], expectedName:
String): Boolean = {
+ if (!FunctionRegistry.functionSet.contains(
+ FunctionRegistry.builtinFunctionIdentifier(expectedName)) ||
+ !FunctionResolution.isUnqualifiedOrBuiltinFunctionName(nameParts,
expectedName)) {
+ return false
+ }
+ nameParts.length match {
+ case 1 =>
+ unqualifiedFunctionResolvesToBuiltinBeforeAnyShadow(nameParts.head)
+ case 2 =>
+ conf.prioritizeSystemCatalog || !persistentFunctionExists(nameParts)
+ case 3 =>
+ true
+ case _ =>
+ false
+ }
+ }
+
+ // All routed SQL/JSON constructors forbid a bare `*` argument. Derived from
the single registry
Review Comment:
Done.
--
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]
