laskoviymishka commented on code in PR #17544:
URL: https://github.com/apache/iceberg/pull/17544#discussion_r3734348831
##########
core/src/test/java/org/apache/iceberg/actions/TestBinPackRewriteFilePlanner.java:
##########
@@ -175,6 +175,41 @@ void testMaxGroupSize() {
assertThat(plan.groupsInPartition(FILE_6.partition())).isEqualTo(1);
}
+ @Test
+ void testMaxFileGroupInputFiles() {
+ addFiles();
+ // First, establish baseline without the constraint
+ BinPackRewriteFilePlanner baselinePlanner = new
BinPackRewriteFilePlanner(table);
+ baselinePlanner.init(REWRITE_ALL);
+ FileRewritePlan<FileGroupInfo, FileScanTask, DataFile, RewriteFileGroup>
baselinePlan =
+ baselinePlanner.plan();
+ int baselineGroupCount = baselinePlan.totalGroupCount();
+ int baselineGroupsInPartition0 =
baselinePlan.groupsInPartition(FILE_1.partition());
+
+ // Now test with max-file-group-input-files set to 2, which limits each
group to 2 files
+ // Partition 0 has 3 files (FILE_1, FILE_2, FILE_3), so it should be split
into 2 groups
+ // Partition 1 has 2 files (FILE_4, FILE_5), so it should be 1 group
+ // Partition 2 has 1 file (FILE_6), so it should be 1 group
+ BinPackRewriteFilePlanner constrainedPlanner = new
BinPackRewriteFilePlanner(table);
+ constrainedPlanner.init(
+ ImmutableMap.of(
+ BinPackRewriteFilePlanner.REWRITE_ALL,
+ "true",
+ BinPackRewriteFilePlanner.MAX_FILE_GROUP_INPUT_FILES,
+ "2"));
+
+ FileRewritePlan<FileGroupInfo, FileScanTask, DataFile, RewriteFileGroup>
constrainedPlan =
+ constrainedPlanner.plan();
+
+ // Verify the constraint is honored: should have MORE groups when input
files are limited
+
assertThat(constrainedPlan.totalGroupCount()).isGreaterThan(baselineGroupCount).isEqualTo(4);
Review Comment:
the `.isGreaterThan(baselineGroupCount)` is redundant once you've got
`.isEqualTo(4)` — the absolute assertion is strictly stronger, and the whole
baseline planner above (the `baselinePlanner` block) exists only to feed those
relative checks.
`testMaxGroupSize` right above just asserts the absolute counts with no
baseline. I'd drop the baseline planner and keep the plain
`isEqualTo(4)`/`isEqualTo(2)` to match it.
##########
spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteDataFilesAction.java:
##########
@@ -1486,6 +1486,26 @@ public void testInvalidOptions() {
.hasMessageContaining("requires enabling Iceberg Spark session
extensions");
}
+ @TestTemplate
+ public void testMaxFileGroupInputFilesOption() {
+ Table table = createTable(4);
+ shouldHaveFiles(table, 4);
+
+ List<Object[]> originalData = currentData();
+ long dataSizeBefore = testDataSize(table);
+
+ RewriteDataFiles.Result result =
+ basicRewrite(table)
+ .option(SizeBasedFileRewritePlanner.MAX_FILE_GROUP_INPUT_FILES,
"2")
+ .execute();
+
+ assertThat(result.rewriteResults()).as("Action should rewrite file
groups").isNotEmpty();
Review Comment:
this asserts the option is accepted, but not that it does anything — 4 files
landing in a single group is also non-empty, so a regression where
`max-file-group-input-files` is accepted but silently ignored would still pass
here.
With 4 unpartitioned files and a limit of 2 the grouping is deterministic (2
groups), so I'd assert `result.rewriteResults()` has size 2 and add
`shouldHaveFiles(table, 2)` after the rewrite — then the test actually pins the
constraint rather than just proving it's no longer rejected. wdyt?
##########
core/src/test/java/org/apache/iceberg/actions/TestSizeBasedFileRewritePlanner.java:
##########
@@ -103,7 +103,8 @@ void testValidOptions() {
BinPackRewriteFilePlanner.MAX_FILE_SIZE_BYTES,
BinPackRewriteFilePlanner.MIN_INPUT_FILES,
BinPackRewriteFilePlanner.REWRITE_ALL,
- BinPackRewriteFilePlanner.MAX_FILE_GROUP_SIZE_BYTES));
+ BinPackRewriteFilePlanner.MAX_FILE_GROUP_SIZE_BYTES,
+ BinPackRewriteFilePlanner.MAX_FILE_GROUP_INPUT_FILES));
Review Comment:
while we're adding this to the valid set — `testInvalidOption` covers the `>
0` check for `MAX_FILE_GROUP_SIZE_BYTES` but not for
`MAX_FILE_GROUP_INPUT_FILES`, even though `maxGroupCount()` carries the same
precondition. Worth adding a `MAX_FILE_GROUP_INPUT_FILES, "0"` case there
expecting the "must be > 0" message.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
