vladimirg-db commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2675873589
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -2077,25 +2077,25 @@ class Analyzer(
plan.resolveExpressionsWithPruning(_.containsAnyPattern(UNRESOLVED_FUNCTION)) {
case f @ UnresolvedFunction(nameParts, _, _, _, _, _, _) =>
- if (functionResolution.lookupBuiltinOrTempFunction(nameParts,
Some(f)).isDefined) {
+ // Check cache first for persistent functions to avoid repeated
external catalog lookups.
Review Comment:
Please reflect the changes in the `LookupFunctions` class scaladoc instead
of adding the inline comments
##########
sql/core/src/test/resources/sql-tests/analyzer-results/identifier-clause-legacy.sql.out:
##########
@@ -991,41 +991,30 @@ org.apache.spark.sql.AnalysisException
-- !query
CREATE TEMPORARY FUNCTION IDENTIFIER('default.my' || 'DoubleAvg') AS
'test.org.apache.spark.sql.MyDoubleAvg'
Review Comment:
Can you please add new golden file tests specifically to test
builtin/temp/persistent function name disambiguation?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionResolution.scala:
##########
@@ -47,6 +47,40 @@ class FunctionResolution(
private val trimWarningEnabled = new AtomicBoolean(true)
+ /**
+ * Checks if a multi-part name is qualified with a specific namespace.
+ * Supports both 2-part (namespace.name) and 3-part (system.namespace.name)
qualifications.
+ *
+ * @param nameParts The multi-part name to check
+ * @param namespace The namespace to check for (e.g., "builtin", "session")
+ * @return true if qualified with the given namespace
+ */
+ private def isQualifiedWithNamespace(nameParts: Seq[String], namespace:
String): Boolean = {
Review Comment:
Propose to put functions in the order of their usage top-down, because it's
a natural reading order (iow these private functions after the public
`lookupBuiltinOrTempFunction`)
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionResolution.scala:
##########
@@ -89,17 +130,87 @@ class FunctionResolution(
}
}
+ /**
+ * Validates that a function exists and can be used in the given context.
+ * This is used by the LookupFunctions analyzer rule for early validation.
+ *
+ * @param nameParts The function name parts.
+ * @param node The UnresolvedFunction node for error reporting.
+ * @return true if the function is a builtin or temporary function, false if
it's persistent.
+ */
+ def validateFunctionExistence(
+ nameParts: Seq[String],
+ node: UnresolvedFunction): Boolean = {
+
+ // Check if function exists as scalar function.
+ val existsAsScalar = lookupBuiltinOrTempFunction(nameParts,
Some(node)).isDefined
+
+ if (existsAsScalar) {
+ // Function exists in scalar registry, can be used in scalar context.
+ return true // It's a builtin or temp function
+ }
+
+ // Check if function exists as table function.
+ val existsAsTable = lookupBuiltinOrTempTableFunction(nameParts).isDefined
+
+ if (existsAsTable) {
+ // Function exists ONLY in table registry - cannot be used in scalar
context.
+ throw
QueryCompilationErrors.notAScalarFunctionError(nameParts.mkString("."), node)
Review Comment:
The organization of this method is weird - sometimes it returns a boolean,
and sometimes it throws an error. I propose to move error throwing outside.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionResolution.scala:
##########
@@ -89,17 +130,87 @@ class FunctionResolution(
}
}
+ /**
+ * Validates that a function exists and can be used in the given context.
+ * This is used by the LookupFunctions analyzer rule for early validation.
+ *
+ * @param nameParts The function name parts.
+ * @param node The UnresolvedFunction node for error reporting.
+ * @return true if the function is a builtin or temporary function, false if
it's persistent.
+ */
+ def validateFunctionExistence(
Review Comment:
> true if the function is a builtin or temporary function, false if it's
persistent.
Then it should be called `isBuiltinOrTemporaryFunction`
##########
sql/core/src/test/scala/org/apache/spark/sql/catalyst/analysis/FunctionQualificationSuite.scala:
##########
@@ -0,0 +1,745 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.catalyst.analysis
+
+import org.apache.spark.sql.AnalysisException
+import org.apache.spark.sql.test.SharedSparkSession
+
+/**
+ * Test suite for function qualification with builtin and session namespaces.
+ * Tests qualified function references, qualified DDL, shadowing, and unified
namespace resolution.
+ */
+class FunctionQualificationSuite extends SharedSparkSession {
+
+ // ==================== Qualified Function References ====================
+
+ test("unqualified builtin function works") {
Review Comment:
I think that everything tested here may be tested using a golden file. That
would be easier and less code. Please convert to a golden file
--
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]
