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 2022/05/23 23:28:12 UTC
[GitHub] [iceberg] RussellSpitzer opened a new pull request, #4848: Spark: Fix Alignment of Merge Commands with Mixed Case
RussellSpitzer opened a new pull request, #4848:
URL: https://github.com/apache/iceberg/pull/4848
Prior to this a mixed-case insert statement would fail to be marked
as aligned after our alignement rule was applied. This would occur
because Spark is allowed to opperate without case sensitivity. Although
we would correctly align the fields, our check for alignment required
an exact match even with the system was set to be case-insensitive. Failing this
check would mean our RewriteToMerge analysis rule would never get applied.
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4848: Spark: Fix Alignment of Merge Commands with Mixed Case
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4848:
URL: https://github.com/apache/iceberg/pull/4848#discussion_r880060339
##########
spark/v3.2/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMerge.java:
##########
@@ -1186,6 +1186,32 @@ public void testMergeAlignsUpdateAndInsertActions() {
sql("SELECT * FROM %s ORDER BY id", tableName));
}
+ @Test
+ public void testMergeMixedCaseAlignsUpdateAndInsertActions() {
+ createAndInitTable("id INT, a INT, b STRING", "{ \"id\": 1, \"a\": 2, \"b\": \"str\" }");
+ createOrReplaceView(
+ "source",
+ "{ \"id\": 1, \"c1\": -2, \"c2\": \"new_str_1\" }\n" +
+ "{ \"id\": 2, \"c1\": -20, \"c2\": \"new_str_2\" }");
Review Comment:
Hahaha that happens to me all the time. So annoying.
--
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
[GitHub] [iceberg] aokolnychyi commented on a diff in pull request #4848: Spark: Fix Alignment of Merge Commands with Mixed Case
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on code in PR #4848:
URL: https://github.com/apache/iceberg/pull/4848#discussion_r879962647
##########
spark/v3.2/spark-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/AlignRowLevelCommandAssignments.scala:
##########
@@ -84,7 +84,16 @@ object AlignRowLevelCommandAssignments
throw new AnalysisException("Not matched actions can only contain INSERT")
}
- m.copy(matchedActions = alignedMatchedActions, notMatchedActions = alignedNotMatchedActions)
+ val alignedMerge = m.copy(
+ matchedActions = alignedMatchedActions,
+ notMatchedActions = alignedNotMatchedActions)
+
+ if (!alignedMerge.aligned) {
Review Comment:
I wonder whether we should also cover 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.
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
[GitHub] [iceberg] aokolnychyi commented on a diff in pull request #4848: Spark: Fix Alignment of Merge Commands with Mixed Case
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on code in PR #4848:
URL: https://github.com/apache/iceberg/pull/4848#discussion_r879962532
##########
spark/v3.2/spark-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/AlignRowLevelCommandAssignments.scala:
##########
@@ -84,7 +84,16 @@ object AlignRowLevelCommandAssignments
throw new AnalysisException("Not matched actions can only contain INSERT")
}
- m.copy(matchedActions = alignedMatchedActions, notMatchedActions = alignedNotMatchedActions)
+ val alignedMerge = m.copy(
+ matchedActions = alignedMatchedActions,
+ notMatchedActions = alignedNotMatchedActions)
+
+ if (!alignedMerge.aligned) {
Review Comment:
An alternative way of solving this is to provide a check like `MergeIntoIcebergTableResolutionCheck`. It will be run after all resolution rules and we will know for sure that we failed to align the assignments. If we fail here, we won't give Spark any chances to fix the alignments using other rules.
##########
spark/v3.2/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMerge.java:
##########
@@ -1186,6 +1186,32 @@ public void testMergeAlignsUpdateAndInsertActions() {
sql("SELECT * FROM %s ORDER BY id", tableName));
}
+ @Test
+ public void testMergeMixedCaseAlignsUpdateAndInsertActions() {
+ createAndInitTable("id INT, a INT, b STRING", "{ \"id\": 1, \"a\": 2, \"b\": \"str\" }");
+ createOrReplaceView(
+ "source",
+ "{ \"id\": 1, \"c1\": -2, \"c2\": \"new_str_1\" }\n" +
+ "{ \"id\": 2, \"c1\": -20, \"c2\": \"new_str_2\" }");
Review Comment:
nit: the rest of this file aligns the json records slightly differently (no extra indentation for 2nd record)
##########
spark/v3.2/spark-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/AlignRowLevelCommandAssignments.scala:
##########
@@ -84,7 +84,16 @@ object AlignRowLevelCommandAssignments
throw new AnalysisException("Not matched actions can only contain INSERT")
}
- m.copy(matchedActions = alignedMatchedActions, notMatchedActions = alignedNotMatchedActions)
+ val alignedMerge = m.copy(
+ matchedActions = alignedMatchedActions,
+ notMatchedActions = alignedNotMatchedActions)
+
+ if (!alignedMerge.aligned) {
Review Comment:
I wonder whether we should also cover UPDATEs too?
##########
spark/v3.2/spark-extensions/src/main/scala/org/apache/spark/sql/catalyst/expressions/AssignmentUtils.scala:
##########
@@ -40,7 +40,15 @@ object AssignmentUtils extends SQLConfHelper {
sameSize && table.output.zip(assignments).forall { case (attr, assignment) =>
val key = assignment.key
val value = assignment.value
- toAssignmentRef(attr) == toAssignmentRef(key) &&
+ val refsEqual = if (conf.caseSensitiveAnalysis) {
Review Comment:
Seems like we can use `conf.resolver` that would abstract this away? Then we will have only one case to handle.
##########
spark/v3.2/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMerge.java:
##########
@@ -1186,6 +1186,32 @@ public void testMergeAlignsUpdateAndInsertActions() {
sql("SELECT * FROM %s ORDER BY id", tableName));
}
+ @Test
+ public void testMergeMixedCaseAlignsUpdateAndInsertActions() {
+ createAndInitTable("id INT, a INT, b STRING", "{ \"id\": 1, \"a\": 2, \"b\": \"str\" }");
+ createOrReplaceView(
+ "source",
+ "{ \"id\": 1, \"c1\": -2, \"c2\": \"new_str_1\" }\n" +
+ "{ \"id\": 2, \"c1\": -20, \"c2\": \"new_str_2\" }");
+
+ sql("MERGE INTO %s t USING source " +
+ "ON t.iD == source.Id " +
Review Comment:
nit: same for MERGE statements
--
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
[GitHub] [iceberg] aokolnychyi commented on a diff in pull request #4848: Spark: Fix Alignment of Merge Commands with Mixed Case
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on code in PR #4848:
URL: https://github.com/apache/iceberg/pull/4848#discussion_r881937391
##########
spark/v3.2/spark-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/AlignedRowLevelIcebergCommandCheck.scala:
##########
@@ -0,0 +1,38 @@
+/*
+ * 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.plans.logical.LogicalPlan
+import org.apache.spark.sql.catalyst.plans.logical.MergeIntoIcebergTable
+import org.apache.spark.sql.catalyst.plans.logical.UpdateIcebergTable
+
+object AlignedRowLevelIcebergCommandCheck extends (LogicalPlan => Unit) {
+
+ override def apply(plan: LogicalPlan): Unit = {
+ plan foreach {
+ case m: MergeIntoIcebergTable if !m.aligned =>
+ throw new AnalysisException(s"Could not align Iceberg MERGE INT: $m")
+ case u: UpdateIcebergTable if !u.aligned =>
+ throw new AnalysisException(s"Could not align Iceberg UPDATE was never aligned: $u")
Review Comment:
nit: do we need "was never aligned" part?
--
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
[GitHub] [iceberg] RussellSpitzer commented on pull request #4848: Spark: Fix Alignment of Merge Commands with Mixed Case
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on PR #4848:
URL: https://github.com/apache/iceberg/pull/4848#issuecomment-1137617957
I blame all typos on the fact that I don't have my external monitor
--
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
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #4848: Spark: Fix Alignment of Merge Commands with Mixed Case
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on code in PR #4848:
URL: https://github.com/apache/iceberg/pull/4848#discussion_r879977099
##########
spark/v3.2/spark-extensions/src/main/scala/org/apache/spark/sql/catalyst/expressions/AssignmentUtils.scala:
##########
@@ -40,7 +40,15 @@ object AssignmentUtils extends SQLConfHelper {
sameSize && table.output.zip(assignments).forall { case (attr, assignment) =>
val key = assignment.key
val value = assignment.value
- toAssignmentRef(attr) == toAssignmentRef(key) &&
+ val refsEqual = if (conf.caseSensitiveAnalysis) {
Review Comment:
That is a great idea
--
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
[GitHub] [iceberg] aokolnychyi commented on a diff in pull request #4848: Spark: Fix Alignment of Merge Commands with Mixed Case
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on code in PR #4848:
URL: https://github.com/apache/iceberg/pull/4848#discussion_r881937103
##########
spark/v3.2/spark-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/AlignedRowLevelIcebergCommandCheck.scala:
##########
@@ -0,0 +1,38 @@
+/*
+ * 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.plans.logical.LogicalPlan
+import org.apache.spark.sql.catalyst.plans.logical.MergeIntoIcebergTable
+import org.apache.spark.sql.catalyst.plans.logical.UpdateIcebergTable
+
+object AlignedRowLevelIcebergCommandCheck extends (LogicalPlan => Unit) {
+
+ override def apply(plan: LogicalPlan): Unit = {
+ plan foreach {
+ case m: MergeIntoIcebergTable if !m.aligned =>
+ throw new AnalysisException(s"Could not align Iceberg MERGE INT: $m")
Review Comment:
nit: looks like a typo at the end?
--
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
[GitHub] [iceberg] RussellSpitzer merged pull request #4848: Spark: Fix Alignment of Merge Commands with Mixed Case
Posted by GitBox <gi...@apache.org>.
RussellSpitzer merged PR #4848:
URL: https://github.com/apache/iceberg/pull/4848
--
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
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #4848: Spark: Fix Alignment of Merge Commands with Mixed Case
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on code in PR #4848:
URL: https://github.com/apache/iceberg/pull/4848#discussion_r879973234
##########
spark/v3.2/spark-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/AlignRowLevelCommandAssignments.scala:
##########
@@ -84,7 +84,16 @@ object AlignRowLevelCommandAssignments
throw new AnalysisException("Not matched actions can only contain INSERT")
}
- m.copy(matchedActions = alignedMatchedActions, notMatchedActions = alignedNotMatchedActions)
+ val alignedMerge = m.copy(
+ matchedActions = alignedMatchedActions,
+ notMatchedActions = alignedNotMatchedActions)
+
+ if (!alignedMerge.aligned) {
Review Comment:
I did add in a test case for Update. I think that's a good point, I can also just add a case to the Merge Rewrite Rule itself to match on any unaligned merge to an Iceberg table
--
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
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #4848: Spark: Fix Alignment of Merge Commands with Mixed Case
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on code in PR #4848:
URL: https://github.com/apache/iceberg/pull/4848#discussion_r879977304
##########
spark/v3.2/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMerge.java:
##########
@@ -1186,6 +1186,32 @@ public void testMergeAlignsUpdateAndInsertActions() {
sql("SELECT * FROM %s ORDER BY id", tableName));
}
+ @Test
+ public void testMergeMixedCaseAlignsUpdateAndInsertActions() {
+ createAndInitTable("id INT, a INT, b STRING", "{ \"id\": 1, \"a\": 2, \"b\": \"str\" }");
+ createOrReplaceView(
+ "source",
+ "{ \"id\": 1, \"c1\": -2, \"c2\": \"new_str_1\" }\n" +
+ "{ \"id\": 2, \"c1\": -20, \"c2\": \"new_str_2\" }");
Review Comment:
ugh it was the intellij helper annotations, they made it look aligned :)
--
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
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #4848: Spark: Fix Alignment of Merge Commands with Mixed Case
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on code in PR #4848:
URL: https://github.com/apache/iceberg/pull/4848#discussion_r880820536
##########
spark/v3.2/spark-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/AlignRowLevelCommandAssignments.scala:
##########
@@ -84,7 +84,16 @@ object AlignRowLevelCommandAssignments
throw new AnalysisException("Not matched actions can only contain INSERT")
}
- m.copy(matchedActions = alignedMatchedActions, notMatchedActions = alignedNotMatchedActions)
+ val alignedMerge = m.copy(
+ matchedActions = alignedMatchedActions,
+ notMatchedActions = alignedNotMatchedActions)
+
+ if (!alignedMerge.aligned) {
Review Comment:
Add check rule for both Updates and Merges
--
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