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 2024/01/05 11:44:51 UTC
[PR] Spark: Support dropping views [iceberg]
nastra opened a new pull request, #9421:
URL: https://github.com/apache/iceberg/pull/9421
This PR introduces DROP support for Iceberg views and requires a `pre-substitution` batch in
`IcebergSparkSqlExtensionsParser` because `ResolveSessionCatalog` exits early in https://github.com/apache/spark/blob/branch-3.5/sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala#L224-L229 for V2 commands
--
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 dropping views [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #9421:
URL: https://github.com/apache/iceberg/pull/9421#discussion_r1452825545
##########
spark/v3.5/spark-extensions/src/main/scala/org/apache/spark/sql/execution/datasources/v2/ExtendedDataSourceV2Strategy.scala:
##########
@@ -90,6 +93,9 @@ case class ExtendedDataSourceV2Strategy(spark: SparkSession) extends Strategy wi
case OrderAwareCoalesce(numPartitions, coalescer, child) =>
OrderAwareCoalesceExec(numPartitions, coalescer, planLater(child)) :: Nil
+ case DropIcebergView(ResolvedIdentifier(viewCatalog: ViewCatalog, ident), ifExists) =>
Review Comment:
I'm not sure what happens if this isn't a `ViewCatalog`, but the new rewrite rule should ensure that it always is.
--
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 dropping views [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #9421:
URL: https://github.com/apache/iceberg/pull/9421#discussion_r1452824768
##########
spark/v3.5/spark-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/HijackViewCommands.scala:
##########
@@ -0,0 +1,53 @@
+/*
+ * 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.SparkSession
+import org.apache.spark.sql.catalyst.plans.logical.DropView
+import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
+import org.apache.spark.sql.catalyst.plans.logical.views.DropIcebergView
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.connector.catalog.CatalogManager
+import org.apache.spark.sql.connector.catalog.CatalogPlugin
+import org.apache.spark.sql.connector.catalog.LookupCatalog
+import org.apache.spark.sql.connector.catalog.ViewCatalog
+
+/**
+ * ResolveSessionCatalog exits early for some v2 View commands,
+ * thus they are pre-substituted here and then handled in ResolveViews
+ */
+case class HijackViewCommands(spark: SparkSession) extends Rule[LogicalPlan] with LookupCatalog {
+
+ protected lazy val catalogManager: CatalogManager = spark.sessionState.catalogManager
+
+ override def apply(plan: LogicalPlan): LogicalPlan = plan.resolveOperatorsUp {
+ case DropView(UnresolvedIdentifier(nameParts, allowTemp), ifExists)
+ if isViewCatalog(catalogManager.currentCatalog) && !isTempView(nameParts) =>
Review Comment:
I don't think this test is correct. The pattern using `UnresolvedIdentifier` will match any `DROP VIEW` plan and convert it to an Iceberg plan. The `isViewCatalog` check needs to test the catalog responsible for the view, not the current catalog. The current catalog check makes the v1 drop view test work when `USE spark_catalog` was called, but it would have the wrong behavior if an Iceberg catalog were the current.
I think the logic should use `CatalogAndIdentifier` from `LookupCatalog` like Spark does in `ResolveCatalogs`:
```scala
case d@DropView(UnresolvedIdentifier(nameParts, allowTemp), ifExists)
if !isTempView(allowTemp, nameParts) =>
nameParts match {
case CatalogAndIdentifier(catalog, ident) if isViewCatalog(catalog) =>
DropIcebergView(ResolvedIdentifier(catalog, ident), ifExists)
case _ =>
d
}
```
That way we know that the command is only replaced if a v2 `ViewCatalog` is responsible for it.
You can also make this easier for the next time by creating a custom pattern with `unapply`:
```scala
object ResolvedView {
def unapply(unresolved: UnresolvedIdentifier): Option[ResolvedIdentifier] = unresolved match {
case UnresolvedIdentifier(nameParts, true) if isTempView(nameParts) =>
None
case UnresolvedIdentifier(CatalogAndIdentifier(catalog, ident), _) if isViewCatalog(catalog) =>
Some(ResolvedIdentifier(catalog, ident))
case _ =>
None
}
}
```
Then the rule is much simpler:
```scala
override def apply(plan: LogicalPlan): LogicalPlan = plan.resolveOperatorsUp {
case DropView(ResolvedView(resolved), ifExists) =>
DropIcebergView(resolved, ifExists)
}
```
I've opened https://github.com/nastra/iceberg/pull/138 with these changes and test updates to catch this case by changing back to the test catalog.
--
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 dropping views [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9421:
URL: https://github.com/apache/iceberg/pull/9421#discussion_r1442789176
##########
spark/v3.5/spark-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveViews.scala:
##########
@@ -111,8 +116,8 @@ case class ResolveViews(spark: SparkSession) extends Rule[LogicalPlan] with Look
}
private def qualifyFunctionIdentifiers(
- plan: LogicalPlan,
- catalogAndNamespace: Seq[String]): LogicalPlan = plan transformExpressions {
+ plan: LogicalPlan,
+ catalogAndNamespace: Seq[String]): LogicalPlan = plan transformExpressions {
Review Comment:
formatting was off here
--
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 dropping views [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9421:
URL: https://github.com/apache/iceberg/pull/9421#discussion_r1443969970
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestViews.java:
##########
@@ -635,6 +633,51 @@ private Catalog tableCatalog() {
return Spark3Util.loadIcebergCatalog(spark, catalogName);
}
+ @Test
+ public void dropView() {
+ String viewName = "viewToBeDropped";
+ String sql = String.format("SELECT id FROM %s", tableName);
+
+ ViewCatalog viewCatalog = viewCatalog();
+
+ TableIdentifier identifier = TableIdentifier.of(NAMESPACE, viewName);
+ viewCatalog
+ .buildView(identifier)
+ .withQuery("spark", sql)
+ .withDefaultNamespace(NAMESPACE)
+ .withDefaultCatalog(catalogName)
+ .withSchema(schema(sql))
+ .create();
+
+ assertThat(viewCatalog.viewExists(identifier)).isTrue();
+
+ sql("DROP VIEW %s", viewName);
+ assertThat(viewCatalog.viewExists(identifier)).isFalse();
+ }
+
+ @Test
+ public void dropNonExistingView() {
+ assertThatThrownBy(() -> sql("DROP VIEW non_existing"))
+ .isInstanceOf(AnalysisException.class)
+ .hasMessageContaining("The view %s.%s cannot be found", NAMESPACE, "non_existing");
+
+ assertThatNoException().isThrownBy(() -> sql("DROP VIEW IF EXISTS non_existing"));
+ }
+
+ @Test
+ public void dropGlobalTempView() {
+ String globalTempView = "globalViewToBeDropped";
Review Comment:
yes this is mainly to make sure we didn't mess things up with normal views. I've added a comment and also added a test with v1 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 dropping views [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #9421:
URL: https://github.com/apache/iceberg/pull/9421
--
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 dropping views [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on PR #9421:
URL: https://github.com/apache/iceberg/pull/9421#issuecomment-1892917215
I think once this includes the changes from https://github.com/nastra/iceberg/pull/138, I'm +1.
--
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 dropping views [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #9421:
URL: https://github.com/apache/iceberg/pull/9421#discussion_r1443156571
##########
spark/v3.5/spark-extensions/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DropV2ViewExec.scala:
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.execution.datasources.v2
+
+import org.apache.spark.sql.catalyst.InternalRow
+import org.apache.spark.sql.catalyst.analysis.NoSuchViewException
+import org.apache.spark.sql.catalyst.expressions.Attribute
+import org.apache.spark.sql.connector.catalog.Identifier
+import org.apache.spark.sql.connector.catalog.ViewCatalog
+
+
+case class DropV2ViewExec(
+ catalog: ViewCatalog,
+ ident: Identifier,
+ ifExists: Boolean) extends LeafV2CommandExec {
+
+ override lazy val output: Seq[Attribute] = Nil
+
+ override protected def run(): Seq[InternalRow] = {
+ if (catalog.viewExists(ident)) {
+ catalog.dropView(ident)
Review Comment:
Doesn't this return a boolean? If so, we could avoid loading or checking existence and just call `dropView` directly. Then if that returns `false` and `ifExists` is false then we could throw `NoSuchViewException`.
--
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 dropping views [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #9421:
URL: https://github.com/apache/iceberg/pull/9421#discussion_r1443152002
##########
spark/v3.5/spark-extensions/src/main/scala/org/apache/spark/sql/catalyst/parser/extensions/IcebergSparkSqlExtensionsParser.scala:
##########
@@ -122,7 +128,27 @@ class IcebergSparkSqlExtensionsParser(delegate: ParserInterface) extends ParserI
if (isIcebergCommand(sqlTextAfterSubstitution)) {
parse(sqlTextAfterSubstitution) { parser => astBuilder.visit(parser.singleStatement()) }.asInstanceOf[LogicalPlan]
} else {
- delegate.parsePlan(sqlText)
+ ViewSubstitutionExecutor.execute(delegate.parsePlan(sqlText))
+ }
+ }
+
+ private object ViewSubstitutionExecutor extends RuleExecutor[LogicalPlan] {
+ private val fixedPoint = FixedPoint(
+ maxIterations,
+ errorOnExceed = true,
+ maxIterationsSetting = SQLConf.ANALYZER_MAX_ITERATIONS.key)
+
+ override protected def batches: Seq[Batch] = Seq(Batch("pre-substitution", fixedPoint, V2ViewSubstitution))
Review Comment:
Do we want to call this pre-substitution still? I originally used that because I thought we wanted substitution rules in it. But it turns out that we don't need this for view substitution, only for command hijacking. Maybe a "Hijack Commands" batch?
--
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 dropping views [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9421:
URL: https://github.com/apache/iceberg/pull/9421#discussion_r1443974708
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestViews.java:
##########
@@ -635,6 +633,51 @@ private Catalog tableCatalog() {
return Spark3Util.loadIcebergCatalog(spark, catalogName);
}
+ @Test
+ public void dropView() {
+ String viewName = "viewToBeDropped";
+ String sql = String.format("SELECT id FROM %s", tableName);
+
+ ViewCatalog viewCatalog = viewCatalog();
+
+ TableIdentifier identifier = TableIdentifier.of(NAMESPACE, viewName);
+ viewCatalog
+ .buildView(identifier)
+ .withQuery("spark", sql)
+ .withDefaultNamespace(NAMESPACE)
+ .withDefaultCatalog(catalogName)
+ .withSchema(schema(sql))
+ .create();
+
+ assertThat(viewCatalog.viewExists(identifier)).isTrue();
+
+ sql("DROP VIEW %s", viewName);
+ assertThat(viewCatalog.viewExists(identifier)).isFalse();
+ }
+
+ @Test
+ public void dropNonExistingView() {
+ assertThatThrownBy(() -> sql("DROP VIEW non_existing"))
+ .isInstanceOf(AnalysisException.class)
+ .hasMessageContaining("The view %s.%s cannot be found", NAMESPACE, "non_existing");
+
+ assertThatNoException().isThrownBy(() -> sql("DROP VIEW IF EXISTS non_existing"));
Review Comment:
done
--
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 dropping views [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #9421:
URL: https://github.com/apache/iceberg/pull/9421#discussion_r1443275368
##########
spark/v3.5/spark-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveViews.scala:
##########
@@ -45,14 +47,17 @@ case class ResolveViews(spark: SparkSession) extends Rule[LogicalPlan] with Look
protected lazy val catalogManager: CatalogManager = spark.sessionState.catalogManager
override def apply(plan: LogicalPlan): LogicalPlan = plan resolveOperators {
- case u@UnresolvedRelation(nameParts, _, _)
- if catalogManager.v1SessionCatalog.isTempView(nameParts) =>
+ case u@UnresolvedRelation(nameParts, _, _) if isTempView(nameParts) =>
u
case u@UnresolvedRelation(parts@CatalogAndIdentifier(catalog, ident), _, _) =>
loadView(catalog, ident)
.map(createViewRelation(parts, _))
.getOrElse(u)
+
+ case DropIcebergView(r@ResolvedIdentifier(catalog, ident), ifExists)
+ if isTempView(ident.asMultipartIdentifier) || !isViewCatalog(catalog) =>
+ DropView(r, ifExists)
Review Comment:
Why do we need to convert back to a non-Iceberg plan? Is it that we always convert in the pre-substitution batch? If so, is it reasonable to detect whether it is an Iceberg `ViewCatalog` before converting rather than converting back?
--
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 dropping views [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9421:
URL: https://github.com/apache/iceberg/pull/9421#discussion_r1453292988
##########
spark/v3.5/spark-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/HijackViewCommands.scala:
##########
@@ -0,0 +1,53 @@
+/*
+ * 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.SparkSession
+import org.apache.spark.sql.catalyst.plans.logical.DropView
+import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
+import org.apache.spark.sql.catalyst.plans.logical.views.DropIcebergView
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.connector.catalog.CatalogManager
+import org.apache.spark.sql.connector.catalog.CatalogPlugin
+import org.apache.spark.sql.connector.catalog.LookupCatalog
+import org.apache.spark.sql.connector.catalog.ViewCatalog
+
+/**
+ * ResolveSessionCatalog exits early for some v2 View commands,
+ * thus they are pre-substituted here and then handled in ResolveViews
+ */
+case class HijackViewCommands(spark: SparkSession) extends Rule[LogicalPlan] with LookupCatalog {
+
+ protected lazy val catalogManager: CatalogManager = spark.sessionState.catalogManager
+
+ override def apply(plan: LogicalPlan): LogicalPlan = plan.resolveOperatorsUp {
+ case DropView(UnresolvedIdentifier(nameParts, allowTemp), ifExists)
+ if isViewCatalog(catalogManager.currentCatalog) && !isTempView(nameParts) =>
Review Comment:
thanks for spotting and fixing this :100:
--
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 dropping views [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #9421:
URL: https://github.com/apache/iceberg/pull/9421#issuecomment-1893573458
thanks for reviewing @rdblue. I'll go ahead and merge this, since everything should be addressed
--
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 dropping views [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9421:
URL: https://github.com/apache/iceberg/pull/9421#discussion_r1443962986
##########
spark/v3.5/spark-extensions/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DropV2ViewExec.scala:
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.execution.datasources.v2
+
+import org.apache.spark.sql.catalyst.InternalRow
+import org.apache.spark.sql.catalyst.analysis.NoSuchViewException
+import org.apache.spark.sql.catalyst.expressions.Attribute
+import org.apache.spark.sql.connector.catalog.Identifier
+import org.apache.spark.sql.connector.catalog.ViewCatalog
+
+
+case class DropV2ViewExec(
+ catalog: ViewCatalog,
+ ident: Identifier,
+ ifExists: Boolean) extends LeafV2CommandExec {
+
+ override lazy val output: Seq[Attribute] = Nil
+
+ override protected def run(): Seq[InternalRow] = {
+ if (catalog.viewExists(ident)) {
+ catalog.dropView(ident)
Review Comment:
makes sense, I've updated that
--
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 dropping views [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #9421:
URL: https://github.com/apache/iceberg/pull/9421#discussion_r1443153917
##########
spark/v3.5/spark-extensions/src/main/scala/org/apache/spark/sql/catalyst/parser/extensions/IcebergSparkSqlExtensionsParser.scala:
##########
@@ -122,7 +128,27 @@ class IcebergSparkSqlExtensionsParser(delegate: ParserInterface) extends ParserI
if (isIcebergCommand(sqlTextAfterSubstitution)) {
parse(sqlTextAfterSubstitution) { parser => astBuilder.visit(parser.singleStatement()) }.asInstanceOf[LogicalPlan]
} else {
- delegate.parsePlan(sqlText)
+ ViewSubstitutionExecutor.execute(delegate.parsePlan(sqlText))
+ }
+ }
+
+ private object ViewSubstitutionExecutor extends RuleExecutor[LogicalPlan] {
+ private val fixedPoint = FixedPoint(
+ maxIterations,
+ errorOnExceed = true,
+ maxIterationsSetting = SQLConf.ANALYZER_MAX_ITERATIONS.key)
+
+ override protected def batches: Seq[Batch] = Seq(Batch("pre-substitution", fixedPoint, V2ViewSubstitution))
Review Comment:
And it just occurred to me that we may not need an executor at all if we don't need to run to a fixed point. Can we just apply a command hijacking rule by itself instead?
--
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 dropping views [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #9421:
URL: https://github.com/apache/iceberg/pull/9421#discussion_r1443227383
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestViews.java:
##########
@@ -635,6 +633,51 @@ private Catalog tableCatalog() {
return Spark3Util.loadIcebergCatalog(spark, catalogName);
}
+ @Test
+ public void dropView() {
+ String viewName = "viewToBeDropped";
+ String sql = String.format("SELECT id FROM %s", tableName);
+
+ ViewCatalog viewCatalog = viewCatalog();
+
+ TableIdentifier identifier = TableIdentifier.of(NAMESPACE, viewName);
+ viewCatalog
+ .buildView(identifier)
+ .withQuery("spark", sql)
+ .withDefaultNamespace(NAMESPACE)
+ .withDefaultCatalog(catalogName)
+ .withSchema(schema(sql))
+ .create();
+
+ assertThat(viewCatalog.viewExists(identifier)).isTrue();
+
+ sql("DROP VIEW %s", viewName);
+ assertThat(viewCatalog.viewExists(identifier)).isFalse();
+ }
+
+ @Test
+ public void dropNonExistingView() {
+ assertThatThrownBy(() -> sql("DROP VIEW non_existing"))
+ .isInstanceOf(AnalysisException.class)
+ .hasMessageContaining("The view %s.%s cannot be found", NAMESPACE, "non_existing");
+
+ assertThatNoException().isThrownBy(() -> sql("DROP VIEW IF EXISTS non_existing"));
+ }
+
+ @Test
+ public void dropGlobalTempView() {
+ String globalTempView = "globalViewToBeDropped";
Review Comment:
Are these cases just to validate that we don't mess up normal view creation/deletion? If so, can you add a comment?
Do we also want to have one for v1 views in a different catalog?
--
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 dropping views [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #9421:
URL: https://github.com/apache/iceberg/pull/9421#discussion_r1443226670
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestViews.java:
##########
@@ -635,6 +633,51 @@ private Catalog tableCatalog() {
return Spark3Util.loadIcebergCatalog(spark, catalogName);
}
+ @Test
+ public void dropView() {
+ String viewName = "viewToBeDropped";
+ String sql = String.format("SELECT id FROM %s", tableName);
+
+ ViewCatalog viewCatalog = viewCatalog();
+
+ TableIdentifier identifier = TableIdentifier.of(NAMESPACE, viewName);
+ viewCatalog
+ .buildView(identifier)
+ .withQuery("spark", sql)
+ .withDefaultNamespace(NAMESPACE)
+ .withDefaultCatalog(catalogName)
+ .withSchema(schema(sql))
+ .create();
+
+ assertThat(viewCatalog.viewExists(identifier)).isTrue();
+
+ sql("DROP VIEW %s", viewName);
+ assertThat(viewCatalog.viewExists(identifier)).isFalse();
+ }
+
+ @Test
+ public void dropNonExistingView() {
+ assertThatThrownBy(() -> sql("DROP VIEW non_existing"))
+ .isInstanceOf(AnalysisException.class)
+ .hasMessageContaining("The view %s.%s cannot be found", NAMESPACE, "non_existing");
+
+ assertThatNoException().isThrownBy(() -> sql("DROP VIEW IF EXISTS non_existing"));
Review Comment:
Nit: I think that `IF EXISTS` is a separate option and probably deserves a separate test case.
--
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 dropping views [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #9421:
URL: https://github.com/apache/iceberg/pull/9421#discussion_r1452824967
##########
spark/v3.5/spark-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/HijackViewCommands.scala:
##########
@@ -0,0 +1,53 @@
+/*
+ * 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.SparkSession
+import org.apache.spark.sql.catalyst.plans.logical.DropView
+import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
+import org.apache.spark.sql.catalyst.plans.logical.views.DropIcebergView
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.connector.catalog.CatalogManager
+import org.apache.spark.sql.connector.catalog.CatalogPlugin
+import org.apache.spark.sql.connector.catalog.LookupCatalog
+import org.apache.spark.sql.connector.catalog.ViewCatalog
+
+/**
+ * ResolveSessionCatalog exits early for some v2 View commands,
+ * thus they are pre-substituted here and then handled in ResolveViews
+ */
+case class HijackViewCommands(spark: SparkSession) extends Rule[LogicalPlan] with LookupCatalog {
Review Comment:
While I think this is based on my wording, I think "hijack" probably isn't the best word for what we're doing here. We typically use "rewrite" instead. I made this change in my PR.
--
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 dropping views [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9421:
URL: https://github.com/apache/iceberg/pull/9421#discussion_r1453073856
##########
spark/v3.5/spark-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/HijackViewCommands.scala:
##########
@@ -0,0 +1,53 @@
+/*
+ * 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.SparkSession
+import org.apache.spark.sql.catalyst.plans.logical.DropView
+import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
+import org.apache.spark.sql.catalyst.plans.logical.views.DropIcebergView
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.connector.catalog.CatalogManager
+import org.apache.spark.sql.connector.catalog.CatalogPlugin
+import org.apache.spark.sql.connector.catalog.LookupCatalog
+import org.apache.spark.sql.connector.catalog.ViewCatalog
+
+/**
+ * ResolveSessionCatalog exits early for some v2 View commands,
+ * thus they are pre-substituted here and then handled in ResolveViews
+ */
+case class HijackViewCommands(spark: SparkSession) extends Rule[LogicalPlan] with LookupCatalog {
Review Comment:
`RewriteViewCommands` also makes sense to me, so let's go with that
--
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