nastra commented on code in PR #9675:
URL: https://github.com/apache/iceberg/pull/9675#discussion_r1484192464


##########
spark/v3.5/spark-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/RewriteViewCommands.scala:
##########
@@ -149,4 +167,20 @@ case class RewriteViewCommands(spark: SparkSession) 
extends Rule[LogicalPlan] wi
         None
     }
   }
+
+  /**
+   * Collect the names of all temporary functions.
+   */
+  private def collectTemporaryFunctions(child: LogicalPlan): Seq[String] = {
+    val tempFunctions = new ArrayBuffer[String]()
+    
child.resolveExpressionsWithPruning(_.containsAnyPattern(UNRESOLVED_FUNCTION)) {
+      case f@UnresolvedFunction(nameParts, _, _, _, _) if 
isTempFunction(nameParts.asFunctionIdentifier) =>

Review Comment:
   > This has a bug that is the same as one I pointed out for checking temp 
table references: if the identifier is too long then asFunctionIdentifier is 
going to throw an exception. isTempFunction should be passed Seq[String] and 
should return false if it is too many parts to be a temporary function name.
   
   You're right, I missed this and added a fix + tests
   
   > In addition, this needs to handle names that include the catalog, like 
tabular.system.bucket.
   
   Can you elaborate what you mean by `this needs to handle names that include 
the catalog`?
   
   According to 
https://spark.apache.org/docs/latest/sql-ref-syntax-ddl-create-function.html 
the `function_name` is defined as `[ database_name. ] function_name`. 
   I checked the behavior in Spark and a `database_name` can only be specified 
for non-temp functions, meaning that temp functions are only allowed to have a 
single-part identifier when they are being created.
   
   At read time, only the simple function name without the `catalog` / 
`database` part are allowed for temporary functions, otherwise the temp 
function can't be found. 
   
   Or did you mean to add some test that use catalog+schema in the identifier 
to make sure this doesn't fail here when detecting temp functions (I've added 
such tests btw)?



-- 
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: issues-unsubscr...@iceberg.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org

Reply via email to