rdblue commented on code in PR #9423:
URL: https://github.com/apache/iceberg/pull/9423#discussion_r1463815754
##########
spark/v3.5/spark-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveViews.scala:
##########
@@ -59,6 +62,10 @@ case class ResolveViews(spark: SparkSession) extends
Rule[LogicalPlan] with Look
loadView(catalog, ident)
.map(_ => ResolvedV2View(catalog.asViewCatalog, ident))
.getOrElse(u)
+
+ case c@CreateIcebergView(ResolvedIdentifier(_: ViewCatalog, ident), _, _,
_, _, query, _, _) =>
+ ViewHelper.verifyTemporaryObjectsNotExists(false,
Spark3Util.toV1TableIdentifier(ident), query, Seq.empty)
Review Comment:
This is a validation that needs to be done, but I don't think that calling
the Spark version is a good choice.
This coerces a multipart identifier to a v1 identifier, which is a bad idea
because multi-part identifiers can't necessarily be converted. The method used
here will throw an exception if the namespace has too many parts, which would
break view creation in catalogs with nested namespaces.
This also passes `Seq.empty` instead of a list of the temporary functions
that are referenced, which isn't guaranteed to be correct.
Last, checks like this one should be done in a validation rule, not in
`ResolveViews`. I think this was originally in a validation rule. Why did that
change?
--
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
