Skip to content

Core: Simplify list handling in manifest read and write paths - #17581

Merged
szehon-ho merged 2 commits into
apache:mainfrom
uros-b:core-presize-manifest-row-lists
Aug 17, 2026
Merged

Core: Simplify list handling in manifest read and write paths#17581
szehon-ho merged 2 commits into
apache:mainfrom
uros-b:core-presize-manifest-row-lists

Conversation

@uros-b

@uros-b uros-b commented Aug 9, 2026

Copy link
Copy Markdown
Member

Two small, behavior-preserving cleanups to how manifest code builds result lists:

  • ManifestsTable#partitionSummariesToRows — the rows list is filled with exactly one row per entry in summaries, a size known up front, so it is allocated with Lists.newArrayListWithCapacity(summaries.size()) instead of the default-capacity newArrayList(), avoiding intermediate resizes.

  • ManifestFiles#writeParallel — replaced the intermediate List<Pair<Integer, List<F>>> (built only to carry each group's index) with Tasks.range(groups.size()) indexing directly into groups. This drops the list, its build loop, one Pair allocation per group, the now-unused Pair import, and the comment that existed only to explain the pairing, and it makes the input-order guarantee visible in the run body. It also brings the method in line with the Tasks.range(n) idiom used by every other parallel fan-out in the codebase (ManifestMergeManager, ManifestFilterManager, SnapshotProducer, DVUtil, and the Spark/Flink planners); writeParallel was the sole outlier, having diverged when it was extracted from SnapshotProducer in Core: extract parallel manifest write logic out of SnapshotProducer #16730. The AtomicReferenceArray results assembly is unchanged.

The ManifestsTable pre-size is intentionally limited to this one spot rather than a codebase-wide sweep of every under-sized list, to keep the change small and easy to review.

Use Lists.newArrayListWithCapacity in ManifestsTable.partitionSummariesToRows and ManifestFiles to avoid intermediate ArrayList resizes when the target size is already known.
@github-actions github-actions Bot added the core label Aug 9, 2026

@ebyhr ebyhr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me, but there are still same patterns in other places.

@uros-b uros-b left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thank you @ebyhr. Yeah, probably there are other places too, but I happened to come across this one. Otherwise, the PR could explode quite a bit (like this one for example: #17534).

@anoopj anoopj left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@ebyhr

ebyhr commented Aug 10, 2026

Copy link
Copy Markdown
Member

Limiting the scope is perfectly acceptable, but I recommend providing an explanation of the intention in the PR description. This way, it becomes clear whether the limitation was intentional or not.

@uros-b

uros-b commented Aug 13, 2026

Copy link
Copy Markdown
Member Author

cc @szehon-ho PTAL

@szehon-ho szehon-ho left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Both changes are correct and behavior-preserving, and newArrayListWithCapacity is the right helper here since the sizes are exact rather than estimates.

For ManifestFiles.writeParallel there's a better version of the change: the list being pre-sized doesn't need to exist at all, and removing it would also bring the method in line with every other parallel fan-out in the codebase. Details inline.

Comment thread core/src/main/java/org/apache/iceberg/ManifestFiles.java Outdated
@uros-b uros-b changed the title Core: Pre-size row lists built from known-size collections Core: Simplify list handling in manifest read and write paths Aug 16, 2026
@uros-b
uros-b requested a review from szehon-ho August 16, 2026 16:50
@szehon-ho
szehon-ho merged commit 2da21c5 into apache:main Aug 17, 2026
39 checks passed
@szehon-ho

Copy link
Copy Markdown
Member

Merged, thanks @uros-b and @anoopj @ebyhr for additional review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants