Skip to content

[1.10] Backport always merge DVs on commit path (#15006) to 1.10.x#15654

Merged
amogh-jahagirdar merged 2 commits into
apache:1.10.xfrom
amogh-jahagirdar:backport-dv-merging-1.10
Mar 17, 2026
Merged

[1.10] Backport always merge DVs on commit path (#15006) to 1.10.x#15654
amogh-jahagirdar merged 2 commits into
apache:1.10.xfrom
amogh-jahagirdar:backport-dv-merging-1.10

Conversation

@amogh-jahagirdar

@amogh-jahagirdar amogh-jahagirdar commented Mar 16, 2026

Copy link
Copy Markdown
Contributor

Backports always merge DVs on commit path (#15006) to 1.10.x

Note:
This was not a completely clean backport because 1.10 doesn't have spark 4.1 and in the original PR some 4.1 tests had to be updated. This does include the test updates for the spark versions that were part of the 1.10 release.

Fixes #15599

@amogh-jahagirdar

amogh-jahagirdar commented Mar 16, 2026

Copy link
Copy Markdown
Contributor Author

OK core tests in 1.10 with this because:

TestRowDelta > testCannotMergeDVsMismatchedSequenceNumbers() > formatVersion = 3, branch = main FAILED
    java.lang.IllegalArgumentException: Content offset is required for DV
        at org.apache.iceberg.relocated.com.google.common.base.Preconditions.checkArgument(Preconditions.java:141)
        at org.apache.iceberg.FileMetadata$Builder.build(FileMetadata.java:259)
        at org.apache.iceberg.TestRowDelta.testCannotMergeDVsMismatchedSequenceNumbers(TestRowDelta.java:2201)

that's a new test I added as part of the original change but the test relies on making sure DV fields are properly copied over in the FileMetadata builder copy(DeleteFile) api, and that was fixed in a PR #14351 . I think there's a good argument that we should backport #14351 first because that's simply a correctness problem in that API without that fix (I would expect copy to always preserve those fields as is) but I'm not sure if this will cause behavior changes in the backport release, will check that out further.

If not, then we just update this test in this 1.10 branch to just explicitly pass through the value in the builder

@RussellSpitzer

Copy link
Copy Markdown
Member

I would probably just modify/ remove the failing test to avoid the behavior change unless you are 100% confident we would really be always fixing a bug and not break anyone with slightly different behavior. We can always have folks upgrade to 11 if they need the check to work.

@amogh-jahagirdar

amogh-jahagirdar commented Mar 16, 2026

Copy link
Copy Markdown
Contributor Author

Yeah I agree @RussellSpitzer , I took another look and while we could cherry pick the specific parts of #14351 to address this bug, it's probably not worth it for a patch release where it's unclear if someone relies on the fields being nulled out or something after a copy. It's better to have that change be in 1.11. I updated the tests to work around the issue.

@singhpk234 singhpk234 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, thanks @amogh-jahagirdar !

@amogh-jahagirdar

Copy link
Copy Markdown
Contributor Author

Thanks all, merging!

@amogh-jahagirdar amogh-jahagirdar merged commit 08342d1 into apache:1.10.x Mar 17, 2026
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants