Core: Fix TestReplacePartitions using wrong table for validation#15798
Merged
singhpk234 merged 1 commit intoapache:mainfrom Mar 28, 2026
Merged
Core: Fix TestReplacePartitions using wrong table for validation#15798singhpk234 merged 1 commit intoapache:mainfrom
singhpk234 merged 1 commit intoapache:mainfrom
Conversation
Three test methods in TestReplacePartitions operate on separate unpartitioned or all-void tables but were passing the wrong table instance to commit() and validation helpers: - testReplaceWithUnpartitionedTable used commit(table, ...) instead of commit(unpartitioned, ...) for operations on the unpartitioned table - testReplaceAndMergeWithUnpartitionedTable had the same issue - validateSnapshot and validateManifestEntries were called without specifying which table to use for reading manifests, defaulting to the partitioned test table instead of the actual target table The commit(table, ...) bug was introduced in apache#6650 when the test was refactored from direct toBranch().commit() calls to use the commit() helper. The validation calls for the all-void unpartitioned table (added in apache#14186) also used the default overloads that read manifests via the wrong table's specs. This adds Table-accepting overloads for validateSnapshot and validateManifestEntries in TestBase and updates the affected tests to pass the correct table instance.
singhpk234
approved these changes
Mar 27, 2026
Contributor
singhpk234
left a comment
singhpk234
left a comment
There was a problem hiding this comment.
LGTM, thank you so much for fixing it @RussellSpitzer !
huaxingao
approved these changes
Mar 27, 2026
Contributor
huaxingao
left a comment
huaxingao
left a comment
There was a problem hiding this comment.
LGTM. Thanks @RussellSpitzer
Contributor
|
Thanks for the change @RussellSpitzer ! thank for the review @huaxingao |
manuzhang
pushed a commit
to manuzhang/iceberg
that referenced
this pull request
Mar 30, 2026
…che#15798) Three test methods in TestReplacePartitions operate on separate unpartitioned or all-void tables but were passing the wrong table instance to commit() and validation helpers: - testReplaceWithUnpartitionedTable used commit(table, ...) instead of commit(unpartitioned, ...) for operations on the unpartitioned table - testReplaceAndMergeWithUnpartitionedTable had the same issue - validateSnapshot and validateManifestEntries were called without specifying which table to use for reading manifests, defaulting to the partitioned test table instead of the actual target table The commit(table, ...) bug was introduced in apache#6650 when the test was refactored from direct toBranch().commit() calls to use the commit() helper. The validation calls for the all-void unpartitioned table (added in apache#14186) also used the default overloads that read manifests via the wrong table's specs. This adds Table-accepting overloads for validateSnapshot and validateManifestEntries in TestBase and updates the affected tests to pass the correct table instance.
ldudas-marx
pushed a commit
to ldudas-marx/iceberg
that referenced
this pull request
Mar 31, 2026
…che#15798) Three test methods in TestReplacePartitions operate on separate unpartitioned or all-void tables but were passing the wrong table instance to commit() and validation helpers: - testReplaceWithUnpartitionedTable used commit(table, ...) instead of commit(unpartitioned, ...) for operations on the unpartitioned table - testReplaceAndMergeWithUnpartitionedTable had the same issue - validateSnapshot and validateManifestEntries were called without specifying which table to use for reading manifests, defaulting to the partitioned test table instead of the actual target table The commit(table, ...) bug was introduced in apache#6650 when the test was refactored from direct toBranch().commit() calls to use the commit() helper. The validation calls for the all-void unpartitioned table (added in apache#14186) also used the default overloads that read manifests via the wrong table's specs. This adds Table-accepting overloads for validateSnapshot and validateManifestEntries in TestBase and updates the affected tests to pass the correct table instance.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Discovered when fixing - #15634
Previously this wasn't an issue because the validation table was close enough to the tables being tested that everything worked out. Parquet manifests changed the structure for partitioned vs unpartitioned tables, the validation table was partitioned so validation began to fail.
Three test methods in TestReplacePartitions operate on separate unpartitioned or all-void tables but were passing the wrong table instance to commit() and validation helpers:
The commit(table, ...) bug was introduced in #6650 when the test was refactored from direct toBranch().commit() calls to use the commit() helper. The validation calls for the all-void unpartitioned table (added in #14186) also used the default overloads that read manifests via the wrong table's specs.
This adds Table-accepting overloads for validateSnapshot and validateManifestEntries in TestBase and updates the affected tests to pass the correct table instance.
Cursor + Claude-4.6-opus-high