You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2021/01/20 05:59:14 UTC

[GitHub] [iceberg] aokolnychyi opened a new pull request #2118: Spark: Prohibit subqueries in conditions of MERGE operations

aokolnychyi opened a new pull request #2118:
URL: https://github.com/apache/iceberg/pull/2118


   This PR disables subqueries in conditions of MERGE operations until we figure out the best way to support them.


----------------------------------------------------------------
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.

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


[GitHub] [iceberg] rdblue merged pull request #2118: Spark: Prohibit subqueries in conditions of MERGE operations

Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #2118:
URL: https://github.com/apache/iceberg/pull/2118


   


----------------------------------------------------------------
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.

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


[GitHub] [iceberg] dilipbiswal commented on pull request #2118: Spark: Prohibit subqueries in conditions of MERGE operations

Posted by GitBox <gi...@apache.org>.
dilipbiswal commented on pull request #2118:
URL: https://github.com/apache/iceberg/pull/2118#issuecomment-763795888


   LGTM


----------------------------------------------------------------
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.

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


[GitHub] [iceberg] aokolnychyi commented on a change in pull request #2118: Spark: Prohibit subqueries in conditions of MERGE operations

Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #2118:
URL: https://github.com/apache/iceberg/pull/2118#discussion_r561324225



##########
File path: spark3-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/MergeIntoTablePredicateCheck.scala
##########
@@ -0,0 +1,59 @@
+/*
+ * 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.catalyst.expressions.Expression
+import org.apache.spark.sql.catalyst.expressions.SubqueryExpression
+import org.apache.spark.sql.catalyst.plans.logical.DeleteAction
+import org.apache.spark.sql.catalyst.plans.logical.InsertAction
+import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
+import org.apache.spark.sql.catalyst.plans.logical.MergeIntoTable
+import org.apache.spark.sql.catalyst.plans.logical.UpdateAction
+
+object MergeIntoTablePredicateCheck extends (LogicalPlan => Unit) {
+
+  override def apply(plan: LogicalPlan): Unit = {
+    plan foreach {
+      case merge: MergeIntoTable =>
+        validateMergeIntoConditions(merge)
+      case _ => // OK
+    }
+  }
+
+  private def validateMergeIntoConditions(merge: MergeIntoTable): Unit = {
+    checkMergeIntoCondition(merge.mergeCondition, "SEARCH")
+    val actions = merge.matchedActions ++ merge.notMatchedActions
+    actions.foreach {
+      case DeleteAction(Some(cond)) => checkMergeIntoCondition(cond, "DELETE")
+      case UpdateAction(Some(cond), _) => checkMergeIntoCondition(cond, "UPDATE")
+      case InsertAction(Some(cond), _) => checkMergeIntoCondition(cond, "INSERT")
+      case _ => // OK

Review comment:
       There may be actions without conditions too. 

##########
File path: spark3-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/MergeIntoTablePredicateCheck.scala
##########
@@ -0,0 +1,59 @@
+/*
+ * 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.catalyst.expressions.Expression
+import org.apache.spark.sql.catalyst.expressions.SubqueryExpression
+import org.apache.spark.sql.catalyst.plans.logical.DeleteAction
+import org.apache.spark.sql.catalyst.plans.logical.InsertAction
+import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
+import org.apache.spark.sql.catalyst.plans.logical.MergeIntoTable
+import org.apache.spark.sql.catalyst.plans.logical.UpdateAction
+
+object MergeIntoTablePredicateCheck extends (LogicalPlan => Unit) {
+
+  override def apply(plan: LogicalPlan): Unit = {
+    plan foreach {
+      case merge: MergeIntoTable =>
+        validateMergeIntoConditions(merge)
+      case _ => // OK
+    }
+  }
+
+  private def validateMergeIntoConditions(merge: MergeIntoTable): Unit = {
+    checkMergeIntoCondition(merge.mergeCondition, "SEARCH")
+    val actions = merge.matchedActions ++ merge.notMatchedActions
+    actions.foreach {
+      case DeleteAction(Some(cond)) => checkMergeIntoCondition(cond, "DELETE")
+      case UpdateAction(Some(cond), _) => checkMergeIntoCondition(cond, "UPDATE")
+      case InsertAction(Some(cond), _) => checkMergeIntoCondition(cond, "INSERT")
+      case _ => // OK
+    }
+  }
+
+  private def checkMergeIntoCondition(cond: Expression, condName: String): Unit = {
+    // Spark already validates the conditions are deterministic and don't contain aggregates
+    if (SubqueryExpression.hasSubquery(cond)) {
+      throw new AnalysisException(
+        s"Subqueries are not supported in $condName conditions of MERGE operation: ${cond.sql}")

Review comment:
       Let me rework this bit.

##########
File path: spark3-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/MergeIntoTablePredicateCheck.scala
##########
@@ -0,0 +1,59 @@
+/*
+ * 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.catalyst.expressions.Expression
+import org.apache.spark.sql.catalyst.expressions.SubqueryExpression
+import org.apache.spark.sql.catalyst.plans.logical.DeleteAction
+import org.apache.spark.sql.catalyst.plans.logical.InsertAction
+import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
+import org.apache.spark.sql.catalyst.plans.logical.MergeIntoTable
+import org.apache.spark.sql.catalyst.plans.logical.UpdateAction
+
+object MergeIntoTablePredicateCheck extends (LogicalPlan => Unit) {
+
+  override def apply(plan: LogicalPlan): Unit = {
+    plan foreach {
+      case merge: MergeIntoTable =>
+        validateMergeIntoConditions(merge)
+      case _ => // OK
+    }
+  }
+
+  private def validateMergeIntoConditions(merge: MergeIntoTable): Unit = {
+    checkMergeIntoCondition(merge.mergeCondition, "SEARCH")
+    val actions = merge.matchedActions ++ merge.notMatchedActions
+    actions.foreach {
+      case DeleteAction(Some(cond)) => checkMergeIntoCondition(cond, "DELETE")
+      case UpdateAction(Some(cond), _) => checkMergeIntoCondition(cond, "UPDATE")
+      case InsertAction(Some(cond), _) => checkMergeIntoCondition(cond, "INSERT")
+      case _ => // OK
+    }
+  }
+
+  private def checkMergeIntoCondition(cond: Expression, condName: String): Unit = {
+    // Spark already validates the conditions are deterministic and don't contain aggregates
+    if (SubqueryExpression.hasSubquery(cond)) {
+      throw new AnalysisException(
+        s"Subqueries are not supported in $condName conditions of MERGE operation: ${cond.sql}")

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.

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


[GitHub] [iceberg] aokolnychyi commented on a change in pull request #2118: Spark: Prohibit subqueries in conditions of MERGE operations

Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #2118:
URL: https://github.com/apache/iceberg/pull/2118#discussion_r561101471



##########
File path: spark3-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/MergeIntoTablePredicateCheck.scala
##########
@@ -0,0 +1,59 @@
+/*
+ * 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.catalyst.expressions.Expression
+import org.apache.spark.sql.catalyst.expressions.SubqueryExpression
+import org.apache.spark.sql.catalyst.plans.logical.DeleteAction
+import org.apache.spark.sql.catalyst.plans.logical.InsertAction
+import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
+import org.apache.spark.sql.catalyst.plans.logical.MergeIntoTable
+import org.apache.spark.sql.catalyst.plans.logical.UpdateAction
+
+object MergeIntoTablePredicateCheck extends (LogicalPlan => Unit) {

Review comment:
       I am fine either way. It did seem like the logic was separate but we can refactor this part out once we have UPDATEs.




----------------------------------------------------------------
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.

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


[GitHub] [iceberg] dilipbiswal commented on a change in pull request #2118: Spark: Prohibit subqueries in conditions of MERGE operations

Posted by GitBox <gi...@apache.org>.
dilipbiswal commented on a change in pull request #2118:
URL: https://github.com/apache/iceberg/pull/2118#discussion_r560792554



##########
File path: spark3-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/MergeIntoTablePredicateCheck.scala
##########
@@ -0,0 +1,59 @@
+/*
+ * 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.catalyst.expressions.Expression
+import org.apache.spark.sql.catalyst.expressions.SubqueryExpression
+import org.apache.spark.sql.catalyst.plans.logical.DeleteAction
+import org.apache.spark.sql.catalyst.plans.logical.InsertAction
+import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
+import org.apache.spark.sql.catalyst.plans.logical.MergeIntoTable
+import org.apache.spark.sql.catalyst.plans.logical.UpdateAction
+
+object MergeIntoTablePredicateCheck extends (LogicalPlan => Unit) {
+
+  override def apply(plan: LogicalPlan): Unit = {
+    plan foreach {
+      case merge: MergeIntoTable =>
+        validateMergeIntoConditions(merge)
+      case _ => // OK
+    }
+  }
+
+  private def validateMergeIntoConditions(merge: MergeIntoTable): Unit = {
+    checkMergeIntoCondition(merge.mergeCondition, "SEARCH")
+    val actions = merge.matchedActions ++ merge.notMatchedActions
+    actions.foreach {
+      case DeleteAction(Some(cond)) => checkMergeIntoCondition(cond, "DELETE")
+      case UpdateAction(Some(cond), _) => checkMergeIntoCondition(cond, "UPDATE")
+      case InsertAction(Some(cond), _) => checkMergeIntoCondition(cond, "INSERT")
+      case _ => // OK
+    }
+  }
+
+  private def checkMergeIntoCondition(cond: Expression, condName: String): Unit = {
+    // Spark already validates the conditions are deterministic and don't contain aggregates
+    if (SubqueryExpression.hasSubquery(cond)) {
+      throw new AnalysisException(
+        s"Subqueries are not supported in $condName conditions of MERGE INTO operation: $cond")

Review comment:
       @aokolnychyi should we do ${cond.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.

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


[GitHub] [iceberg] aokolnychyi closed pull request #2118: Spark: Prohibit subqueries in conditions of MERGE operations

Posted by GitBox <gi...@apache.org>.
aokolnychyi closed pull request #2118:
URL: https://github.com/apache/iceberg/pull/2118


   


----------------------------------------------------------------
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.

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


[GitHub] [iceberg] aokolnychyi commented on a change in pull request #2118: Spark: Prohibit subqueries in conditions of MERGE operations

Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #2118:
URL: https://github.com/apache/iceberg/pull/2118#discussion_r561331201



##########
File path: spark3-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/MergeIntoTablePredicateCheck.scala
##########
@@ -0,0 +1,59 @@
+/*
+ * 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.catalyst.expressions.Expression
+import org.apache.spark.sql.catalyst.expressions.SubqueryExpression
+import org.apache.spark.sql.catalyst.plans.logical.DeleteAction
+import org.apache.spark.sql.catalyst.plans.logical.InsertAction
+import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
+import org.apache.spark.sql.catalyst.plans.logical.MergeIntoTable
+import org.apache.spark.sql.catalyst.plans.logical.UpdateAction
+
+object MergeIntoTablePredicateCheck extends (LogicalPlan => Unit) {
+
+  override def apply(plan: LogicalPlan): Unit = {
+    plan foreach {
+      case merge: MergeIntoTable =>
+        validateMergeIntoConditions(merge)
+      case _ => // OK
+    }
+  }
+
+  private def validateMergeIntoConditions(merge: MergeIntoTable): Unit = {
+    checkMergeIntoCondition(merge.mergeCondition, "SEARCH")
+    val actions = merge.matchedActions ++ merge.notMatchedActions
+    actions.foreach {
+      case DeleteAction(Some(cond)) => checkMergeIntoCondition(cond, "DELETE")
+      case UpdateAction(Some(cond), _) => checkMergeIntoCondition(cond, "UPDATE")
+      case InsertAction(Some(cond), _) => checkMergeIntoCondition(cond, "INSERT")
+      case _ => // OK
+    }
+  }
+
+  private def checkMergeIntoCondition(cond: Expression, condName: String): Unit = {
+    // Spark already validates the conditions are deterministic and don't contain aggregates
+    if (SubqueryExpression.hasSubquery(cond)) {
+      throw new AnalysisException(
+        s"Subqueries are not supported in $condName conditions of MERGE operation: ${cond.sql}")

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.

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


[GitHub] [iceberg] aokolnychyi commented on a change in pull request #2118: Spark: Prohibit subqueries in conditions of MERGE operations

Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #2118:
URL: https://github.com/apache/iceberg/pull/2118#discussion_r561100302



##########
File path: spark3-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/MergeIntoTablePredicateCheck.scala
##########
@@ -0,0 +1,59 @@
+/*
+ * 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.catalyst.expressions.Expression
+import org.apache.spark.sql.catalyst.expressions.SubqueryExpression
+import org.apache.spark.sql.catalyst.plans.logical.DeleteAction
+import org.apache.spark.sql.catalyst.plans.logical.InsertAction
+import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
+import org.apache.spark.sql.catalyst.plans.logical.MergeIntoTable
+import org.apache.spark.sql.catalyst.plans.logical.UpdateAction
+
+object MergeIntoTablePredicateCheck extends (LogicalPlan => Unit) {
+
+  override def apply(plan: LogicalPlan): Unit = {
+    plan foreach {
+      case merge: MergeIntoTable =>
+        validateMergeIntoConditions(merge)
+      case _ => // OK
+    }
+  }
+
+  private def validateMergeIntoConditions(merge: MergeIntoTable): Unit = {
+    checkMergeIntoCondition(merge.mergeCondition, "SEARCH")
+    val actions = merge.matchedActions ++ merge.notMatchedActions
+    actions.foreach {
+      case DeleteAction(Some(cond)) => checkMergeIntoCondition(cond, "DELETE")
+      case UpdateAction(Some(cond), _) => checkMergeIntoCondition(cond, "UPDATE")
+      case InsertAction(Some(cond), _) => checkMergeIntoCondition(cond, "INSERT")
+      case _ => // OK
+    }
+  }
+
+  private def checkMergeIntoCondition(cond: Expression, condName: String): Unit = {
+    // Spark already validates the conditions are deterministic and don't contain aggregates
+    if (SubqueryExpression.hasSubquery(cond)) {
+      throw new AnalysisException(
+        s"Subqueries are not supported in $condName conditions of MERGE INTO operation: $cond")

Review comment:
       Let me update.




----------------------------------------------------------------
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.

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


[GitHub] [iceberg] aokolnychyi commented on a change in pull request #2118: Spark: Prohibit subqueries in conditions of MERGE operations

Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #2118:
URL: https://github.com/apache/iceberg/pull/2118#discussion_r561324225



##########
File path: spark3-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/MergeIntoTablePredicateCheck.scala
##########
@@ -0,0 +1,59 @@
+/*
+ * 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.catalyst.expressions.Expression
+import org.apache.spark.sql.catalyst.expressions.SubqueryExpression
+import org.apache.spark.sql.catalyst.plans.logical.DeleteAction
+import org.apache.spark.sql.catalyst.plans.logical.InsertAction
+import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
+import org.apache.spark.sql.catalyst.plans.logical.MergeIntoTable
+import org.apache.spark.sql.catalyst.plans.logical.UpdateAction
+
+object MergeIntoTablePredicateCheck extends (LogicalPlan => Unit) {
+
+  override def apply(plan: LogicalPlan): Unit = {
+    plan foreach {
+      case merge: MergeIntoTable =>
+        validateMergeIntoConditions(merge)
+      case _ => // OK
+    }
+  }
+
+  private def validateMergeIntoConditions(merge: MergeIntoTable): Unit = {
+    checkMergeIntoCondition(merge.mergeCondition, "SEARCH")
+    val actions = merge.matchedActions ++ merge.notMatchedActions
+    actions.foreach {
+      case DeleteAction(Some(cond)) => checkMergeIntoCondition(cond, "DELETE")
+      case UpdateAction(Some(cond), _) => checkMergeIntoCondition(cond, "UPDATE")
+      case InsertAction(Some(cond), _) => checkMergeIntoCondition(cond, "INSERT")
+      case _ => // OK

Review comment:
       There may be actions without conditions too. 

##########
File path: spark3-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/MergeIntoTablePredicateCheck.scala
##########
@@ -0,0 +1,59 @@
+/*
+ * 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.catalyst.expressions.Expression
+import org.apache.spark.sql.catalyst.expressions.SubqueryExpression
+import org.apache.spark.sql.catalyst.plans.logical.DeleteAction
+import org.apache.spark.sql.catalyst.plans.logical.InsertAction
+import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
+import org.apache.spark.sql.catalyst.plans.logical.MergeIntoTable
+import org.apache.spark.sql.catalyst.plans.logical.UpdateAction
+
+object MergeIntoTablePredicateCheck extends (LogicalPlan => Unit) {
+
+  override def apply(plan: LogicalPlan): Unit = {
+    plan foreach {
+      case merge: MergeIntoTable =>
+        validateMergeIntoConditions(merge)
+      case _ => // OK
+    }
+  }
+
+  private def validateMergeIntoConditions(merge: MergeIntoTable): Unit = {
+    checkMergeIntoCondition(merge.mergeCondition, "SEARCH")
+    val actions = merge.matchedActions ++ merge.notMatchedActions
+    actions.foreach {
+      case DeleteAction(Some(cond)) => checkMergeIntoCondition(cond, "DELETE")
+      case UpdateAction(Some(cond), _) => checkMergeIntoCondition(cond, "UPDATE")
+      case InsertAction(Some(cond), _) => checkMergeIntoCondition(cond, "INSERT")
+      case _ => // OK
+    }
+  }
+
+  private def checkMergeIntoCondition(cond: Expression, condName: String): Unit = {
+    // Spark already validates the conditions are deterministic and don't contain aggregates
+    if (SubqueryExpression.hasSubquery(cond)) {
+      throw new AnalysisException(
+        s"Subqueries are not supported in $condName conditions of MERGE operation: ${cond.sql}")

Review comment:
       Let me rework this bit.




----------------------------------------------------------------
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.

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


[GitHub] [iceberg] dilipbiswal commented on a change in pull request #2118: Spark: Prohibit subqueries in conditions of MERGE operations

Posted by GitBox <gi...@apache.org>.
dilipbiswal commented on a change in pull request #2118:
URL: https://github.com/apache/iceberg/pull/2118#discussion_r560793573



##########
File path: spark3-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/MergeIntoTablePredicateCheck.scala
##########
@@ -0,0 +1,59 @@
+/*
+ * 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.catalyst.expressions.Expression
+import org.apache.spark.sql.catalyst.expressions.SubqueryExpression
+import org.apache.spark.sql.catalyst.plans.logical.DeleteAction
+import org.apache.spark.sql.catalyst.plans.logical.InsertAction
+import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
+import org.apache.spark.sql.catalyst.plans.logical.MergeIntoTable
+import org.apache.spark.sql.catalyst.plans.logical.UpdateAction
+
+object MergeIntoTablePredicateCheck extends (LogicalPlan => Unit) {

Review comment:
       @aokolnychyi I am thinking if we should have a single rule for checking all the plans that iceberg rewrites  instead of having a separate check for each one ?




----------------------------------------------------------------
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.

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


[GitHub] [iceberg] aokolnychyi commented on a change in pull request #2118: Spark: Prohibit subqueries in conditions of MERGE operations

Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #2118:
URL: https://github.com/apache/iceberg/pull/2118#discussion_r561103858



##########
File path: spark3-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/MergeIntoTablePredicateCheck.scala
##########
@@ -0,0 +1,59 @@
+/*
+ * 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.catalyst.expressions.Expression
+import org.apache.spark.sql.catalyst.expressions.SubqueryExpression
+import org.apache.spark.sql.catalyst.plans.logical.DeleteAction
+import org.apache.spark.sql.catalyst.plans.logical.InsertAction
+import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
+import org.apache.spark.sql.catalyst.plans.logical.MergeIntoTable
+import org.apache.spark.sql.catalyst.plans.logical.UpdateAction
+
+object MergeIntoTablePredicateCheck extends (LogicalPlan => Unit) {
+
+  override def apply(plan: LogicalPlan): Unit = {
+    plan foreach {
+      case merge: MergeIntoTable =>
+        validateMergeIntoConditions(merge)
+      case _ => // OK
+    }
+  }
+
+  private def validateMergeIntoConditions(merge: MergeIntoTable): Unit = {
+    checkMergeIntoCondition(merge.mergeCondition, "SEARCH")
+    val actions = merge.matchedActions ++ merge.notMatchedActions
+    actions.foreach {
+      case DeleteAction(Some(cond)) => checkMergeIntoCondition(cond, "DELETE")
+      case UpdateAction(Some(cond), _) => checkMergeIntoCondition(cond, "UPDATE")
+      case InsertAction(Some(cond), _) => checkMergeIntoCondition(cond, "INSERT")
+      case _ => // OK
+    }
+  }
+
+  private def checkMergeIntoCondition(cond: Expression, condName: String): Unit = {
+    // Spark already validates the conditions are deterministic and don't contain aggregates
+    if (SubqueryExpression.hasSubquery(cond)) {
+      throw new AnalysisException(
+        s"Subqueries are not supported in $condName conditions of MERGE INTO operation: $cond")

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.

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


[GitHub] [iceberg] dilipbiswal commented on pull request #2118: Spark: Prohibit subqueries in conditions of MERGE operations

Posted by GitBox <gi...@apache.org>.
dilipbiswal commented on pull request #2118:
URL: https://github.com/apache/iceberg/pull/2118#issuecomment-763452212


   Looks good to me.


----------------------------------------------------------------
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.

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


[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #2118: Spark: Prohibit subqueries in conditions of MERGE operations

Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2118:
URL: https://github.com/apache/iceberg/pull/2118#discussion_r561229221



##########
File path: spark3-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/MergeIntoTablePredicateCheck.scala
##########
@@ -0,0 +1,59 @@
+/*
+ * 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.catalyst.expressions.Expression
+import org.apache.spark.sql.catalyst.expressions.SubqueryExpression
+import org.apache.spark.sql.catalyst.plans.logical.DeleteAction
+import org.apache.spark.sql.catalyst.plans.logical.InsertAction
+import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
+import org.apache.spark.sql.catalyst.plans.logical.MergeIntoTable
+import org.apache.spark.sql.catalyst.plans.logical.UpdateAction
+
+object MergeIntoTablePredicateCheck extends (LogicalPlan => Unit) {
+
+  override def apply(plan: LogicalPlan): Unit = {
+    plan foreach {
+      case merge: MergeIntoTable =>
+        validateMergeIntoConditions(merge)
+      case _ => // OK
+    }
+  }
+
+  private def validateMergeIntoConditions(merge: MergeIntoTable): Unit = {
+    checkMergeIntoCondition(merge.mergeCondition, "SEARCH")
+    val actions = merge.matchedActions ++ merge.notMatchedActions
+    actions.foreach {
+      case DeleteAction(Some(cond)) => checkMergeIntoCondition(cond, "DELETE")
+      case UpdateAction(Some(cond), _) => checkMergeIntoCondition(cond, "UPDATE")
+      case InsertAction(Some(cond), _) => checkMergeIntoCondition(cond, "INSERT")
+      case _ => // OK

Review comment:
       Are we actually OK if something else gets through here? Shouldn't this be a "Unknown or unsupported action"?




----------------------------------------------------------------
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.

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


[GitHub] [iceberg] rdblue merged pull request #2118: Spark: Prohibit subqueries in conditions of MERGE operations

Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #2118:
URL: https://github.com/apache/iceberg/pull/2118


   


----------------------------------------------------------------
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.

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


[GitHub] [iceberg] dilipbiswal commented on a change in pull request #2118: Spark: Prohibit subqueries in conditions of MERGE operations

Posted by GitBox <gi...@apache.org>.
dilipbiswal commented on a change in pull request #2118:
URL: https://github.com/apache/iceberg/pull/2118#discussion_r560792554



##########
File path: spark3-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/MergeIntoTablePredicateCheck.scala
##########
@@ -0,0 +1,59 @@
+/*
+ * 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.catalyst.expressions.Expression
+import org.apache.spark.sql.catalyst.expressions.SubqueryExpression
+import org.apache.spark.sql.catalyst.plans.logical.DeleteAction
+import org.apache.spark.sql.catalyst.plans.logical.InsertAction
+import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
+import org.apache.spark.sql.catalyst.plans.logical.MergeIntoTable
+import org.apache.spark.sql.catalyst.plans.logical.UpdateAction
+
+object MergeIntoTablePredicateCheck extends (LogicalPlan => Unit) {
+
+  override def apply(plan: LogicalPlan): Unit = {
+    plan foreach {
+      case merge: MergeIntoTable =>
+        validateMergeIntoConditions(merge)
+      case _ => // OK
+    }
+  }
+
+  private def validateMergeIntoConditions(merge: MergeIntoTable): Unit = {
+    checkMergeIntoCondition(merge.mergeCondition, "SEARCH")
+    val actions = merge.matchedActions ++ merge.notMatchedActions
+    actions.foreach {
+      case DeleteAction(Some(cond)) => checkMergeIntoCondition(cond, "DELETE")
+      case UpdateAction(Some(cond), _) => checkMergeIntoCondition(cond, "UPDATE")
+      case InsertAction(Some(cond), _) => checkMergeIntoCondition(cond, "INSERT")
+      case _ => // OK
+    }
+  }
+
+  private def checkMergeIntoCondition(cond: Expression, condName: String): Unit = {
+    // Spark already validates the conditions are deterministic and don't contain aggregates
+    if (SubqueryExpression.hasSubquery(cond)) {
+      throw new AnalysisException(
+        s"Subqueries are not supported in $condName conditions of MERGE INTO operation: $cond")

Review comment:
       @aokolnychyi Nit: should we do ${cond.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.

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


[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #2118: Spark: Prohibit subqueries in conditions of MERGE operations

Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2118:
URL: https://github.com/apache/iceberg/pull/2118#discussion_r561228551



##########
File path: spark3-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/MergeIntoTablePredicateCheck.scala
##########
@@ -0,0 +1,59 @@
+/*
+ * 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.catalyst.expressions.Expression
+import org.apache.spark.sql.catalyst.expressions.SubqueryExpression
+import org.apache.spark.sql.catalyst.plans.logical.DeleteAction
+import org.apache.spark.sql.catalyst.plans.logical.InsertAction
+import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
+import org.apache.spark.sql.catalyst.plans.logical.MergeIntoTable
+import org.apache.spark.sql.catalyst.plans.logical.UpdateAction
+
+object MergeIntoTablePredicateCheck extends (LogicalPlan => Unit) {
+
+  override def apply(plan: LogicalPlan): Unit = {
+    plan foreach {
+      case merge: MergeIntoTable =>
+        validateMergeIntoConditions(merge)
+      case _ => // OK
+    }
+  }
+
+  private def validateMergeIntoConditions(merge: MergeIntoTable): Unit = {
+    checkMergeIntoCondition(merge.mergeCondition, "SEARCH")
+    val actions = merge.matchedActions ++ merge.notMatchedActions
+    actions.foreach {
+      case DeleteAction(Some(cond)) => checkMergeIntoCondition(cond, "DELETE")
+      case UpdateAction(Some(cond), _) => checkMergeIntoCondition(cond, "UPDATE")
+      case InsertAction(Some(cond), _) => checkMergeIntoCondition(cond, "INSERT")
+      case _ => // OK
+    }
+  }
+
+  private def checkMergeIntoCondition(cond: Expression, condName: String): Unit = {
+    // Spark already validates the conditions are deterministic and don't contain aggregates
+    if (SubqueryExpression.hasSubquery(cond)) {
+      throw new AnalysisException(
+        s"Subqueries are not supported in $condName conditions of MERGE operation: ${cond.sql}")

Review comment:
       My only comment here is that this message may be a bit misleading, since a it will print
   "Subqueries are not supported in DELETE conditions ...."
   But this kind of implies they are supported in some other conditions. Perhaps instead something like
   
   "Cannot perform merge, subqueries are not allowed in MERGE conditions. Subquery found in $condName condition of MERGE: $(cond.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.

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


[GitHub] [iceberg] karuppayya commented on pull request #2118: Spark: Prohibit subqueries in conditions of MERGE operations

Posted by GitBox <gi...@apache.org>.
karuppayya commented on pull request #2118:
URL: https://github.com/apache/iceberg/pull/2118#issuecomment-763500420


   Change looks good to me.


----------------------------------------------------------------
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.

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