Skip to content

Core: Add max-file-group-input-files to valid rewrite options #17544

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rahulsmahadev wants to merge 1 commit into
base: main
Choose a base branch
from
+61 −2
Open

Core: Add max-file-group-input-files to valid rewrite options #17544

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion core/src/main/java/org/apache/iceberg/actions/SizeBasedFileRewritePlanner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,8 @@ public Set<String> validOptions() {
MAX_FILE_SIZE_BYTES,
MIN_INPUT_FILES,
REWRITE_ALL,
MAX_FILE_GROUP_SIZE_BYTES);
MAX_FILE_GROUP_SIZE_BYTES,
MAX_FILE_GROUP_INPUT_FILES);
}

@Override
Expand Down
36 changes: 36 additions & 0 deletions core/src/test/java/org/apache/iceberg/actions/TestBinPackRewriteFilePlanner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,41 @@ void testMaxGroupSize() {
assertThat(plan.groupsInPartition(FILE_6.partition())).isEqualTo(1);
}

@Test

@rahulsmahadev rahulsmahadev Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't know if there is a better way to run this 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);

@laskoviymishka laskoviymishka Aug 7, 2026

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.

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.

assertThat(constrainedPlan.groupsInPartition(FILE_1.partition()))
.isGreaterThan(baselineGroupsInPartition0)
.isEqualTo(2);
assertThat(constrainedPlan.groupsInPartition(FILE_4.partition())).isEqualTo(1);
assertThat(constrainedPlan.groupsInPartition(FILE_6.partition())).isEqualTo(1);
}

@Test
void testFilter() {
addFiles();
Expand Down Expand Up @@ -289,6 +324,7 @@ void testValidOptions() {
BinPackRewriteFilePlanner.MIN_INPUT_FILES,
BinPackRewriteFilePlanner.REWRITE_ALL,
BinPackRewriteFilePlanner.MAX_FILE_GROUP_SIZE_BYTES,
BinPackRewriteFilePlanner.MAX_FILE_GROUP_INPUT_FILES,
BinPackRewriteFilePlanner.DELETE_FILE_THRESHOLD,
BinPackRewriteFilePlanner.DELETE_RATIO_THRESHOLD,
RewriteDataFiles.REWRITE_JOB_ORDER,
Expand Down
1 change: 1 addition & 0 deletions core/src/test/java/org/apache/iceberg/actions/TestBinPackRewritePositionDeletePlanner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,7 @@ void testValidOptions() {
BinPackRewritePositionDeletePlanner.MIN_INPUT_FILES,
BinPackRewritePositionDeletePlanner.REWRITE_ALL,
BinPackRewritePositionDeletePlanner.MAX_FILE_GROUP_SIZE_BYTES,
BinPackRewritePositionDeletePlanner.MAX_FILE_GROUP_INPUT_FILES,
RewriteDataFiles.REWRITE_JOB_ORDER));
}

Expand Down
3 changes: 2 additions & 1 deletion core/src/test/java/org/apache/iceberg/actions/TestSizeBasedFileRewritePlanner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));

@laskoviymishka laskoviymishka Aug 7, 2026

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.

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.

}

@Test
Expand Down
20 changes: 20 additions & 0 deletions ...v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteDataFilesAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();

@laskoviymishka laskoviymishka Aug 7, 2026

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.

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?

assertThat(result.rewrittenBytesCount()).isEqualTo(dataSizeBefore);

table.refresh();
assertEquals("Rows must match", originalData, currentData());
}

@TestTemplate
public void testSortMultipleGroups() {
Table table = createTable(20);
Expand Down
Loading