You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "amogh-jahagirdar (via GitHub)" <gi...@apache.org> on 2024/01/24 23:33:00 UTC
[PR] Spark 3.4: Fix writing of default values in CoW for rows with NULL columns which are unmatched [iceberg]
amogh-jahagirdar opened a new pull request, #9556:
URL: https://github.com/apache/iceberg/pull/9556
Fixes https://github.com/apache/iceberg/issues/9555
--
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 3.4: Fix writing of default values in CoW for rows with NULL columns which are unmatched [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #9556:
URL: https://github.com/apache/iceberg/pull/9556#discussion_r1469971672
##########
spark/v3.4/spark-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/RewriteMergeIntoTable.scala:
##########
@@ -214,6 +214,8 @@ object RewriteMergeIntoTable extends RewriteRowLevelIcebergCommand with Predicat
val rowFromSourceAttr = resolveAttrRef(ROW_FROM_SOURCE_REF, joinPlan)
val rowFromTargetAttr = resolveAttrRef(ROW_FROM_TARGET_REF, joinPlan)
+ // The output expression should retain read attributes for correctly determining nullability
+ val matchedOutputsWithAttrs = matchedActions.map(matchedActionOutput(_, metadataAttrs) :+ readAttrs)
Review Comment:
Thanks @rdblue appreciate the detailed thread with the reasoning, I followed along and it makes sense. I agree that adding the readAttrs to the notMatchedOutput makes more sense for having a `MergeRows` logical plan which is logically sound compared to the previous approach which more forces the nullability check to pass just by passing the attributes along with the matched output list. I've updated the PR. @aokolnychyi let us know what you think.
--
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 3.4: Fix writing of default values in CoW for rows with NULL columns which are unmatched [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #9556:
URL: https://github.com/apache/iceberg/pull/9556#discussion_r1469971672
##########
spark/v3.4/spark-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/RewriteMergeIntoTable.scala:
##########
@@ -214,6 +214,8 @@ object RewriteMergeIntoTable extends RewriteRowLevelIcebergCommand with Predicat
val rowFromSourceAttr = resolveAttrRef(ROW_FROM_SOURCE_REF, joinPlan)
val rowFromTargetAttr = resolveAttrRef(ROW_FROM_TARGET_REF, joinPlan)
+ // The output expression should retain read attributes for correctly determining nullability
+ val matchedOutputsWithAttrs = matchedActions.map(matchedActionOutput(_, metadataAttrs) :+ readAttrs)
Review Comment:
Thanks @rdblue appreciate the detailed thread with the reasoning, I followed along and it makes sense. I agree that adding the readAttrs to the notMatchedOutput makes more sense for having a `MergeRows` logical plan which is logically sound. I've updated the PR. @aokolnychyi let us know what you think.
--
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 3.4: Fix writing of default values in CoW for rows with NULL columns which are unmatched [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #9556:
URL: https://github.com/apache/iceberg/pull/9556#issuecomment-1912823558
I think you identified the problem correctly, @amogh-jahagirdar. We have to take into account the read attributes when determining the nullability of `MergeRows` output. This matches [what](https://github.com/apache/spark/blob/7ab7509fa12418ff5f93782670b7e939c055703a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/RewriteMergeIntoTable.scala#L186) we did in Spark.
Just to be sure, can you set `spark.sql.planChangeLog.level` to info and execute the failing test to see what rule adds the default value behavior?
--
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 3.4: Fix writing of default values in CoW for rows with NULL columns which are unmatched [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #9556:
URL: https://github.com/apache/iceberg/pull/9556#discussion_r1468233102
##########
spark/v3.4/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMerge.java:
##########
@@ -523,6 +523,78 @@ public void testMergeWithOnlyUpdateClauseAndNullValues() {
sql("SELECT * FROM %s ORDER BY id", selectTarget()));
}
+ @Test
+ public void testMergeWithOnlyUpdateNullUnmatchedValues() {
Review Comment:
What about adding these tests to all Spark version to be safe?
--
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 3.4: Fix writing of default values in CoW for rows with NULL columns which are unmatched [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on PR #9556:
URL: https://github.com/apache/iceberg/pull/9556#issuecomment-1915239004
> Just to be sure, can you set spark.sql.planChangeLog.level to info and execute the failing test to see what rule adds the default value behavior? It is still not clear how a wrong nullability info in MergeRows led to inserting default values.
Sure, I've set that for before/after the fix (specifically the fix @rdblue mentioned here https://github.com/apache/iceberg/pull/9556#discussion_r1468932165).
Logs Before: https://gist.github.com/amogh-jahagirdar/98ffeaa21e203423c7da0c98e132e9bd
Logs After: https://gist.github.com/amogh-jahagirdar/6209e373b37e2d7a134df447ec49f8b5
I don't see anything differentiating in terms of the rules and their outputs. I can still explain why there's a default value behavior though based on what I saw the debugger.
The Iceberg schema used for performing the write that gets built here SparkWriteBuilder#validateOrMergeWriteSchema: https://github.com/apache/iceberg/blob/54756b6f5c653be2ab271bfbe262e77109bf9608/spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/source/SparkWriteBuilder.java#L120
That schema makes the fields *required* instead of optional, due to the nullability passed through in the Spark Writer API being incorrect. Then when the write is actually performed, we end up writing default values.
I've updated this PR to follow the approach @rdblue https://github.com/apache/iceberg/pull/9556#discussion_r1468932165 which after going through the thread I believe does make sense, and is a better solution compared to the previous approach which just forces the matched output list to have the attributes for making the nullability check behave as expected.
--
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 3.4: Fix writing of default values in CoW for rows with NULL columns which are unmatched [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue merged PR #9556:
URL: https://github.com/apache/iceberg/pull/9556
--
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 3.4: Fix writing of default values in CoW for rows with NULL columns which are unmatched [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #9556:
URL: https://github.com/apache/iceberg/pull/9556#discussion_r1468928409
##########
spark/v3.4/spark-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/RewriteMergeIntoTable.scala:
##########
@@ -214,6 +214,8 @@ object RewriteMergeIntoTable extends RewriteRowLevelIcebergCommand with Predicat
val rowFromSourceAttr = resolveAttrRef(ROW_FROM_SOURCE_REF, joinPlan)
val rowFromTargetAttr = resolveAttrRef(ROW_FROM_TARGET_REF, joinPlan)
+ // The output expression should retain read attributes for correctly determining nullability
+ val matchedOutputsWithAttrs = matchedActions.map(matchedActionOutput(_, metadataAttrs) :+ readAttrs)
Review Comment:
Okay, I think I understand why the inputs would be aligned with the outputs. This is the copy-on-write case, where each matched clause should produce an output for every column in order. If that's the case, then the indexing on each output should work. That is, each output should correspond to an input attr because both are based on the target table.
If that's the case, then I guess I can see what is happening here. The input and output correspond, so the input type and name should be used. But the output does depend on the nullability of the output expression.
If that's right, then the short-term fix is what I pasted above. If the table column is optional, then this should produce an optional output in case there is a null value.
That will work, but it doesn't explain why the output nullability is incorrect.
--
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 3.4: Fix writing of default values in CoW for rows with NULL columns which are unmatched [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #9556:
URL: https://github.com/apache/iceberg/pull/9556#discussion_r1469080603
##########
spark/v3.4/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMerge.java:
##########
@@ -523,6 +523,78 @@ public void testMergeWithOnlyUpdateClauseAndNullValues() {
sql("SELECT * FROM %s ORDER BY id", selectTarget()));
}
+ @Test
+ public void testMergeWithOnlyUpdateNullUnmatchedValues() {
Review Comment:
Great point, yes we should. 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 3.4: Fix writing of default values in CoW for rows with NULL columns which are unmatched [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #9556:
URL: https://github.com/apache/iceberg/pull/9556#discussion_r1467001797
##########
spark/v3.4/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMerge.java:
##########
@@ -523,6 +523,58 @@ public void testMergeWithOnlyUpdateClauseAndNullValues() {
sql("SELECT * FROM %s ORDER BY id", selectTarget()));
}
+ @Test
+ public void testMergeWithOnlyUpdateNullUnmatchedValues() {
+ createAndInitTable(
+ "id INT, value INT", "{ \"id\": 1, \"value\": 2 }\n" + "{ \"id\": 6, \"value\": null }");
+
+ createOrReplaceView("source", "id INT NOT NULL, value INT", "{ \"id\": 1, \"value\": 100 }\n");
+ sql(
+ "MERGE INTO %s t USING source s "
+ + "ON t.id == s.id "
+ + "WHEN MATCHED THEN "
+ + " UPDATE SET id=123, value=456",
+ commitTarget());
+
+ sql("SELECT * FROM %s", commitTarget());
+
+ ImmutableList<Object[]> expectedRows =
+ ImmutableList.of(
+ row(6, null), // kept
+ row(123, 456)); // updated
+
+ assertEquals(
+ "Should have expected rows",
+ expectedRows,
+ sql("SELECT * FROM %s ORDER BY id", selectTarget()));
+ }
+
+ @Test
+ public void testMergeWithOnlyUpdateSingleFieldNullUnmatchedValues() {
Review Comment:
This test would pass before this change, I added it for additional coverage and verification.
--
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 3.4: Fix writing of default values in CoW for rows with NULL columns which are unmatched [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #9556:
URL: https://github.com/apache/iceberg/pull/9556#discussion_r1468932165
##########
spark/v3.4/spark-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/RewriteMergeIntoTable.scala:
##########
@@ -214,6 +214,8 @@ object RewriteMergeIntoTable extends RewriteRowLevelIcebergCommand with Predicat
val rowFromSourceAttr = resolveAttrRef(ROW_FROM_SOURCE_REF, joinPlan)
val rowFromTargetAttr = resolveAttrRef(ROW_FROM_TARGET_REF, joinPlan)
+ // The output expression should retain read attributes for correctly determining nullability
+ val matchedOutputsWithAttrs = matchedActions.map(matchedActionOutput(_, metadataAttrs) :+ readAttrs)
Review Comment:
Looks like the new tests pass locally with just the change to `buildMergingOutput`.
I also think that I can explain why the nullability is incorrect. That's because in the test case, the only expressions are constants (non-null) and there is no explicit case that represents the existing rows.
That means there are two possible solutions. First, the one I just tested where `buildMergingOutput` carries through the nullability of the input attrs. Assuming that the input attrs are nullable if and only if the table fields are nullable, that's probably the safest option. Second, the copy-on-write cases could pass an additional seq of the input attrs to represent the unmodified rows in the _unmatched_ set:
```scala
// line 230
output = buildMergeRowsOutput(matchedOutputs, notMatchedOutputs :+ readAttrs, readAttrs)
```
I think that I prefer the second option because the output of the `MergeRows` node is more accurate. If an output is always present then it will not be nullable. That's probably also safer.
I also validated that this passes tests.
--
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 3.4: Fix writing of default values in CoW for rows with NULL columns which are unmatched [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #9556:
URL: https://github.com/apache/iceberg/pull/9556#discussion_r1468232951
##########
spark/v3.4/spark-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/RewriteMergeIntoTable.scala:
##########
@@ -214,6 +214,8 @@ object RewriteMergeIntoTable extends RewriteRowLevelIcebergCommand with Predicat
val rowFromSourceAttr = resolveAttrRef(ROW_FROM_SOURCE_REF, joinPlan)
val rowFromTargetAttr = resolveAttrRef(ROW_FROM_TARGET_REF, joinPlan)
+ // The output expression should retain read attributes for correctly determining nullability
+ val matchedOutputsWithAttrs = matchedActions.map(matchedActionOutput(_, metadataAttrs) :+ readAttrs)
Review Comment:
Can we reuse `matchedOutputs` above in this statement?
--
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 3.4: Fix writing of default values in CoW for rows with NULL columns which are unmatched [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #9556:
URL: https://github.com/apache/iceberg/pull/9556#issuecomment-1912790467
Let me see.
--
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 3.4: Fix writing of default values in CoW for rows with NULL columns which are unmatched [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on PR #9556:
URL: https://github.com/apache/iceberg/pull/9556#issuecomment-1917527435
Merging this. Thanks @amogh-jahagirdar!
> It is still not clear how a wrong nullability info in MergeRows led to inserting default values.
@aokolnychyi, I think what's happening is that the nullability flag is not checked when copying the row from the `MergeRows` output. The "default" values are null values coming from looking at unset values in `UnsafeRow`.
--
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 3.4: Fix writing of default values in CoW for rows with NULL columns which are unmatched [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on PR #9556:
URL: https://github.com/apache/iceberg/pull/9556#issuecomment-1909402282
There's another approach that I'm thinking about, I'm not sure yet how reasonable it is but in the Spark plan, if there's no "when not matched" case could we implicitly just add an "insert *" to the not matched condition? This issue is specific to when there is no "not matched" clause.
--
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 3.4: Fix writing of default values in CoW for rows with NULL columns which are unmatched [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #9556:
URL: https://github.com/apache/iceberg/pull/9556#discussion_r1468925515
##########
spark/v3.4/spark-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/RewriteMergeIntoTable.scala:
##########
@@ -214,6 +214,8 @@ object RewriteMergeIntoTable extends RewriteRowLevelIcebergCommand with Predicat
val rowFromSourceAttr = resolveAttrRef(ROW_FROM_SOURCE_REF, joinPlan)
val rowFromTargetAttr = resolveAttrRef(ROW_FROM_TARGET_REF, joinPlan)
+ // The output expression should retain read attributes for correctly determining nullability
+ val matchedOutputsWithAttrs = matchedActions.map(matchedActionOutput(_, metadataAttrs) :+ readAttrs)
Review Comment:
This doesn't make sense to me. This does have the intended effect to correctly set the nullability in `buildMergeRowsOutput`, it seems to me like there is a better way than creating a different (fake) version of `matchedOutputs`. I think this works because it is including the original attributes as fake outputs (_for every action output list_). A more direct way of doing the same thing is to make an attribute nullable if it is nullable OR if an output is nullable.
In other words, I think the actual problem is in `RewriteRowLevelIcebergCommand.buildMergingOutput`:
```scala
protected def buildMergingOutput(
outputs: Seq[Seq[Expression]],
attrs: Seq[Attribute]): Seq[Attribute] = {
// build a correct nullability map for output attributes
// an attribute is nullable if at least one output may produce null
val nullabilityMap = attrs.indices.map { index =>
index -> outputs.exists(output => output(index).nullable)
}.toMap
attrs.zipWithIndex.map { case (attr, index) =>
AttributeReference(attr.name, attr.dataType, nullabilityMap(index))()
}
}
```
The change in the current PR updates the `outputs` arg to that method to include a copy of the original `attrs` for every matched expression. What ends up happening is that `outputs.exists` checks whether the corresponding original attr is nullable. You could do the same without this PR's change like this:
```scala
protected def buildMergingOutput(
outputs: Seq[Seq[Expression]],
attrs: Seq[Attribute]): Seq[Attribute] = {
// build a correct nullability map for output attributes
// an attribute is nullable if at least one output may produce null
val nullabilityMap = attrs.indices.map { index =>
index -> outputs.exists(output => output(index).nullable)
}.toMap
attrs.zipWithIndex.map { case (attr, index) =>
AttributeReference(attr.name, attr.dataType, attr.nullable || nullabilityMap(index))()
}
}
```
Looking at this code also raises a larger question: what is `buildMergingOutput` trying to do? It basically copies the input attrs to be the `MergeRows` output attrs, but then bases nullability on whether unrelated expressions are nullable. The problem is the expression coming from an output comes from an `Assignment` and has nothing to do with the position in the input attrs.
@aokolnychyi do you know what's happening here? Why would the output of assignment expressions affect the nullability of the incoming data?
--
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 3.4: Fix writing of default values in CoW for rows with NULL columns which are unmatched [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #9556:
URL: https://github.com/apache/iceberg/pull/9556#discussion_r1465683526
##########
spark/v3.4/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMerge.java:
##########
@@ -2144,6 +2144,26 @@ public void testMergeWithTableWithNonNullableColumn() {
sql("SELECT * FROM %s ORDER BY id", selectTarget()));
}
+ @Test
+ public void testMergeIntoNullUnmatchedValues() {
+ createAndInitTable(
+ "id INT, ts TIMESTAMP",
+ "{ \"id\": 1, \"ts\": \"2000-01-01 00:00:00\" }\n" + "{ \"id\": 6, \"ts\": null }");
+
+ createOrReplaceView(
+ "source",
+ "id INT NOT NULL, ts TIMESTAMP",
+ "{ \"id\": 1, \"ts\": \"2000-01-01 00:00:00\" }\n");
+ sql(
+ "MERGE INTO %s t USING source s "
+ + "ON t.id == s.id "
+ + "WHEN MATCHED THEN "
+ + " UPDATE SET id=123, ts=current_timestamp()",
+ commitTarget());
+
+ sql("SELECT * FROM %s", commitTarget());
Review Comment:
i'll need to add an assertion but did verify this outputs the right values. I'll also add more tests once CI verifies everything else still passes.
--
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