You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "rahulsmahadev (via GitHub)" <gi...@apache.org> on 2026/08/07 02:24:20 UTC
[PR] Core: Add max-file-group-input-files to valid rewrite options [iceberg]
rahulsmahadev opened a new pull request, #17544:
URL: https://github.com/apache/iceberg/pull/17544
## Summary
`max-file-group-input-files` is a `rewrite_data_files` option that is read and validated by `SizeBasedFileRewritePlanner` but is missing from `validOptions()`, so passing it is rejected.
`SizeBasedFileRewritePlanner.maxGroupCount()` reads and validates the option, but neither `SizeBasedFileRewritePlanner.validOptions()` nor `BinPackRewriteFilePlanner.validOptions()` lists it. `RewriteDataFilesSparkAction.validateAndInitOptions()` fails on any key outside the union of valid options, so `CALL system.rewrite_data_files(table => 't', options => map('max-file-group-input-files', '100'))` throws even though the option is implemented.
This also makes the option behave differently across engines: Flink exposes it through `RewriteDataFiles.maxFileGroupInputFiles()` where it works, while Spark rejects it. The option was added in #14837 and `validOptions()` was not updated at the time.
## Test plan
- Add `MAX_FILE_GROUP_INPUT_FILES` to `SizeBasedFileRewritePlanner.validOptions()`
- Update the exact-set assertions in `TestSizeBasedFileRewritePlanner`, `TestBinPackRewriteFilePlanner`, and `TestBinPackRewritePositionDeletePlanner`
- `./gradlew :iceberg-core:test --tests "org.apache.iceberg.actions.*"` passes (53 tests), plus spotless and checkstyle
--
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] Core: Add max-file-group-input-files to valid rewrite options [iceberg]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka merged PR #17544:
URL: https://github.com/apache/iceberg/pull/17544
--
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] Core: Add max-file-group-input-files to valid rewrite options [iceberg]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
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: 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] Core: Add max-file-group-input-files to valid rewrite options [iceberg]
Posted by "rahulsmahadev (via GitHub)" <gi...@apache.org>.
rahulsmahadev commented on code in PR #17544:
URL: https://github.com/apache/iceberg/pull/17544#discussion_r3733120597
##########
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
Review Comment:
I don't know if there is a better way to run this test
--
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] Core: Add max-file-group-input-files to valid rewrite options [iceberg]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on PR #17544:
URL: https://github.com/apache/iceberg/pull/17544#issuecomment-5241144445
@rahulsmahadev - can you fix CI? once CI green we can merge this
--
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