kbendick commented on a change in pull request #1991:
URL: https://github.com/apache/iceberg/pull/1991#discussion_r549226737
##########
File path: core/src/test/java/org/apache/iceberg/TestMergeAppend.java
##########
@@ -239,8 +239,10 @@ public void testMergeWithExistingManifest() {
public void testManifestMergeMinCount() throws IOException {
Assert.assertEquals("Table should start empty", 0,
listManifestFiles().size());
table.updateProperties().set(TableProperties.MANIFEST_MIN_MERGE_COUNT, "2")
- // each manifest file is 5227 bytes, so 12000 bytes limit will give us
2 bins with 3 manifest/data files.
- .set(TableProperties.MANIFEST_TARGET_SIZE_BYTES, "12000")
+ // Each initial v1/v2 ManifestFile is 5661/6397 bytes respectively.
Merging two of the given
+ // manifests make one v1/v2 ManifestFile of 5672/6408 bytes
respectively, so 12850 bytes
+ // limit will give us two bins with three manifest/data files.
+ .set(TableProperties.MANIFEST_TARGET_SIZE_BYTES, "12850")
Review comment:
Technically this limit needs to be at least 12817 bytes.
That's one byte larger than the number of bins used in the tests * the size
of the largest manifest file in either test run (v1 or v2, v2 is slightly
larger) for the set of operations defined in the test to meet their assertions,
but I chose a value slightly larger / rounded value to match somewhat how the
test was previously set up.
Feel free to suggest a different value.
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
