Re: [PR] [SPARK-59685][SQL] Keep clause-free SQL/JSON constructor canonical SQL bound to the built-in [spark]

["cloud-fan (via GitHub)" <gi...@apache.org>]

cloud-fan commented on code in PR #58938:
URL: https://github.com/apache/spark/pull/58938#discussion_r4078907479


##########
sql/core/src/test/scala/org/apache/spark/sql/JsonValueSuite.scala:
##########
@@ -61,6 +61,48 @@ class JsonValueSuite extends QueryTest with SharedSparkSession {
     }
   }
 
+  test("SPARK-59685: default-clause JSON_VALUE canonical SQL reparses to the built-in under a " +
+      "shadowing PATH") {
+    withSQLConf(
+      SQLConf.PATH_ENABLED.key -> "true",
+      SQLConf.SESSION_FUNCTION_RESOLUTION_ORDER.key -> "second") {
+      try {
+        sql("CREATE TEMPORARY FUNCTION json_value(a STRING, b STRING) RETURNS STRING " +
+          "RETURN 'shadowed'")
+        sql("SET PATH = system.session, system.builtin")
+        // A built-in JSON_VALUE whose only clause is the default RETURNING STRING: the clause makes
+        // it the built-in even under the shadow, but its canonical `sql` would drop the default.
+        val jsonValue = sql(s"SELECT json_value('$doc', '$$.name' RETURNING STRING)")
+          .queryExecution.analyzed.expressions
+          .flatMap(_.collect { case jv: JsonValue => jv }).head
+        // The rendering must reparse back to the built-in, not the same-named routine on the PATH.
+        val reparsed = sql(s"SELECT ${jsonValue.sql}")
+        assert(reparsed.queryExecution.analyzed.expressions
+          .exists(_.exists(_.isInstanceOf[JsonValue])),
+          s"canonical SQL bound the shadow instead of the built-in: ${jsonValue.sql}")
+        checkAnswer(reparsed, Row("Ada"))
+      } finally {
+        sql("SET PATH = DEFAULT_PATH")
+        sql("DROP TEMPORARY FUNCTION IF EXISTS json_value")
+      }
+    }
+  }
+
+  test("SPARK-59685: a default JSON_VALUE keeps a clean auto-generated column name") {

Review Comment:
   **Non-blocking (P2):** The new display-rendering path also applies `renderChild` to both `DEFAULT` expressions, but these generated-name tests exercise only the main JSON input. Could you add cases with a routed child such as `JSON_ARRAY(1)` in both the `ON EMPTY` and `ON ERROR` default positions? Otherwise reverting either branch to `default.get.sql` would leak the nested `RETURNING STRING` into the schema field name while all current assertions still pass.



-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org