You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "nastra (via GitHub)" <gi...@apache.org> on 2023/12/19 15:50:32 UTC
[PR] Spark: Support renaming views [iceberg]
nastra opened a new pull request, #9343:
URL: https://github.com/apache/iceberg/pull/9343
This is based on https://github.com/apache/iceberg/pull/9340 and adds support for resolving and renaming views
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Support renaming views [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #9343:
URL: https://github.com/apache/iceberg/pull/9343#discussion_r1442153222
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestViews.java:
##########
@@ -590,10 +591,7 @@ public void fullFunctionIdentifier() {
@Test
public void fullFunctionIdentifierNotRewrittenLoadFailure() {
String viewName = "fullFunctionIdentifierNotRewrittenLoadFailure";
- String sql =
- String.format(
- "SELECT spark_catalog.system.bucket(100, 'a') AS bucket_result, 'a' AS value",
- catalogName);
+ String sql = "SELECT spark_catalog.system.bucket(100, 'a') AS bucket_result, 'a' AS value";
Review Comment:
:sweat_smile:
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Support renaming views [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #9343:
URL: https://github.com/apache/iceberg/pull/9343#discussion_r1452827023
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestViews.java:
##########
@@ -635,6 +633,118 @@ private Catalog tableCatalog() {
return Spark3Util.loadIcebergCatalog(spark, catalogName);
}
+ @Test
+ public void renameView() throws NoSuchTableException {
Review Comment:
Can you add tests for names that reference temp views? I think we want to know what Spark does. My initial opinion (which could change if Spark does something else!) is that it should fail because the reference is ambiguous.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Support renaming views [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #9343:
URL: https://github.com/apache/iceberg/pull/9343#discussion_r1453638953
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestViews.java:
##########
@@ -635,6 +633,118 @@ private Catalog tableCatalog() {
return Spark3Util.loadIcebergCatalog(spark, catalogName);
}
+ @Test
+ public void renameView() throws NoSuchTableException {
+ insertRows(10);
+ String viewName = viewName("originalView");
+ String renamedView = viewName("renamedView");
+ String sql = String.format("SELECT id FROM %s", tableName);
+
+ ViewCatalog viewCatalog = viewCatalog();
+
+ viewCatalog
Review Comment:
No, I don't think this is a good direction. We want the tests to be independent. This allows us to more easily test just rename.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Support renaming views [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9343:
URL: https://github.com/apache/iceberg/pull/9343#discussion_r1442615912
##########
spark/v3.5/spark-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveViews.scala:
##########
@@ -53,6 +57,17 @@ case class ResolveViews(spark: SparkSession) extends Rule[LogicalPlan] with Look
loadView(catalog, ident)
.map(createViewRelation(parts, _))
.getOrElse(u)
+
+ case u@UnresolvedTableOrView(CatalogAndIdentifier(catalog, ident), _, _) =>
+ loadView(catalog, ident)
+ .map(_ => ResolvedV2View(catalog.asViewCatalog, ident))
+ .getOrElse(u)
+
+ case RenameTable(ResolvedV2View(oldCatalog, oldIdent), CatalogAndIdentifier(newCatalog, newIdent), true) =>
Review Comment:
I added this to `isView@true` so that it's clearer. I checked the Spark code and the `to` identifier isn't resolved anywhere and just passed as-is when doing a rename
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Support renaming views [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9343:
URL: https://github.com/apache/iceberg/pull/9343#discussion_r1453045834
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestViews.java:
##########
@@ -635,6 +633,118 @@ private Catalog tableCatalog() {
return Spark3Util.loadIcebergCatalog(spark, catalogName);
}
+ @Test
+ public void renameView() throws NoSuchTableException {
+ insertRows(10);
+ String viewName = viewName("originalView");
+ String renamedView = viewName("renamedView");
+ String sql = String.format("SELECT id FROM %s", tableName);
+
+ ViewCatalog viewCatalog = viewCatalog();
+
+ viewCatalog
Review Comment:
@rdblue and I talked about this and view creation via SQL requires some additional complexity, so we decided to get a few other smaller PRs/functionality in. https://github.com/apache/iceberg/pull/9421 is introducing some of that complexity and once #9421 is in, adding view creation via SQL should be much smaller in terms of complexity/diffset.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Support renaming views [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9343:
URL: https://github.com/apache/iceberg/pull/9343#discussion_r1453069623
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestViews.java:
##########
@@ -635,6 +633,118 @@ private Catalog tableCatalog() {
return Spark3Util.loadIcebergCatalog(spark, catalogName);
}
+ @Test
+ public void renameView() throws NoSuchTableException {
Review Comment:
I added a test that renames a view which is hidden by a temp view. The behavior is in-line with what we've seen when reading from a view that is hidden by a temp view. Doing a rename will first rename the temp view and re-executing the same rename will then do it for the Iceberg view.
I think for now we don't need any additional checks when handling `UnresolvedTableOrView` in `ResolveViews`
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Support renaming views [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #9343:
URL: https://github.com/apache/iceberg/pull/9343#discussion_r1452826314
##########
spark/v3.5/spark-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveViews.scala:
##########
@@ -53,6 +54,11 @@ case class ResolveViews(spark: SparkSession) extends Rule[LogicalPlan] with Look
loadView(catalog, ident)
.map(createViewRelation(parts, _))
.getOrElse(u)
+
+ case u@UnresolvedTableOrView(CatalogAndIdentifier(catalog, ident), _, _) =>
Review Comment:
Do we need to check whether this matches temp views?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Support renaming views [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #9343:
URL: https://github.com/apache/iceberg/pull/9343
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Support renaming views [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #9343:
URL: https://github.com/apache/iceberg/pull/9343#discussion_r1442152596
##########
spark/v3.5/spark-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveViews.scala:
##########
@@ -53,6 +57,17 @@ case class ResolveViews(spark: SparkSession) extends Rule[LogicalPlan] with Look
loadView(catalog, ident)
.map(createViewRelation(parts, _))
.getOrElse(u)
+
+ case u@UnresolvedTableOrView(CatalogAndIdentifier(catalog, ident), _, _) =>
+ loadView(catalog, ident)
+ .map(_ => ResolvedV2View(catalog.asViewCatalog, ident))
+ .getOrElse(u)
+
+ case RenameTable(ResolvedV2View(oldCatalog, oldIdent), CatalogAndIdentifier(newCatalog, newIdent), true) =>
Review Comment:
Is it possible for the rule on line 61 to resolve the multi-part `to` identifier before this rule runs?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Support renaming views [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #9343:
URL: https://github.com/apache/iceberg/pull/9343#discussion_r1452861042
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestViews.java:
##########
@@ -635,6 +633,118 @@ private Catalog tableCatalog() {
return Spark3Util.loadIcebergCatalog(spark, catalogName);
}
+ @Test
+ public void renameView() throws NoSuchTableException {
+ insertRows(10);
+ String viewName = viewName("originalView");
+ String renamedView = viewName("renamedView");
+ String sql = String.format("SELECT id FROM %s", tableName);
+
+ ViewCatalog viewCatalog = viewCatalog();
+
+ viewCatalog
Review Comment:
The SQL tests will be more cleaner if we support the CREATE VIEW SQL first before RENAME VIEW SQL?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Support renaming views [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #9343:
URL: https://github.com/apache/iceberg/pull/9343#discussion_r1452826597
##########
spark/v3.5/spark-extensions/src/main/scala/org/apache/spark/sql/execution/datasources/v2/ExtendedDataSourceV2Strategy.scala:
##########
@@ -90,9 +94,20 @@ case class ExtendedDataSourceV2Strategy(spark: SparkSession) extends Strategy wi
case OrderAwareCoalesce(numPartitions, coalescer, child) =>
OrderAwareCoalesceExec(numPartitions, coalescer, planLater(child)) :: Nil
+ case RenameTable(ResolvedV2View(oldCatalog: ViewCatalog, oldIdent), newName, isView@true) =>
+ val newIdent = Spark3Util.catalogAndIdentifier(spark, newName.toList.asJava)
+ if (oldCatalog.name != newIdent.catalog().name()) {
+ cannotMoveViewBetweenCatalogs(oldCatalog.name, newIdent.catalog().name())
+ }
+ RenameV2ViewExec(oldCatalog, oldIdent, newIdent.identifier()) :: Nil
+
case _ => Nil
}
+ private def cannotMoveViewBetweenCatalogs(oldCatalog: String, newCatalog: String): Throwable =
Review Comment:
Why have a method for a one-line implementation?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Support renaming views [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #9343:
URL: https://github.com/apache/iceberg/pull/9343#discussion_r1452826597
##########
spark/v3.5/spark-extensions/src/main/scala/org/apache/spark/sql/execution/datasources/v2/ExtendedDataSourceV2Strategy.scala:
##########
@@ -90,9 +94,20 @@ case class ExtendedDataSourceV2Strategy(spark: SparkSession) extends Strategy wi
case OrderAwareCoalesce(numPartitions, coalescer, child) =>
OrderAwareCoalesceExec(numPartitions, coalescer, planLater(child)) :: Nil
+ case RenameTable(ResolvedV2View(oldCatalog: ViewCatalog, oldIdent), newName, isView@true) =>
+ val newIdent = Spark3Util.catalogAndIdentifier(spark, newName.toList.asJava)
+ if (oldCatalog.name != newIdent.catalog().name()) {
+ cannotMoveViewBetweenCatalogs(oldCatalog.name, newIdent.catalog().name())
+ }
+ RenameV2ViewExec(oldCatalog, oldIdent, newIdent.identifier()) :: Nil
+
case _ => Nil
}
+ private def cannotMoveViewBetweenCatalogs(oldCatalog: String, newCatalog: String): Throwable =
Review Comment:
Nit: Why have a method for a one-line implementation?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Support renaming views [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #9343:
URL: https://github.com/apache/iceberg/pull/9343#discussion_r1442151540
##########
spark/v3.5/spark-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveViews.scala:
##########
@@ -53,6 +57,17 @@ case class ResolveViews(spark: SparkSession) extends Rule[LogicalPlan] with Look
loadView(catalog, ident)
.map(createViewRelation(parts, _))
.getOrElse(u)
+
+ case u@UnresolvedTableOrView(CatalogAndIdentifier(catalog, ident), _, _) =>
+ loadView(catalog, ident)
+ .map(_ => ResolvedV2View(catalog.asViewCatalog, ident))
+ .getOrElse(u)
+
+ case RenameTable(ResolvedV2View(oldCatalog, oldIdent), CatalogAndIdentifier(newCatalog, newIdent), true) =>
Review Comment:
What is `true`? Can we add a name to this so that it is more readable?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org