szehon-ho commented on code in PR #17581:
URL: https://github.com/apache/iceberg/pull/17581#discussion_r3788177470
##########
core/src/main/java/org/apache/iceberg/ManifestsTable.java:
##########
@@ -120,7 +120,7 @@ static List<StaticDataTask.Row> partitionSummariesToRows(
return null;
}
- List<StaticDataTask.Row> rows = Lists.newArrayList();
+ List<StaticDataTask.Row> rows =
Lists.newArrayListWithCapacity(summaries.size());
Review Comment:
This one is correct but hard to motivate: `summaries` has one entry per
partition field, so it is almost always well under `ArrayList`'s default
capacity of 10 and no growth happens. Could you say what prompted the change (a
profile, a benchmark, a static-analysis rule)? If there's no measurement behind
it, I'd lean toward leaving this site alone and keeping the PR to the
`ManifestFiles` cleanup.
##########
core/src/main/java/org/apache/iceberg/ManifestFiles.java:
##########
@@ -632,7 +632,7 @@ static <F> List<ManifestFile> writeParallel(
List<List<F>> groups = divide(files, parallelism);
// Pair each group with its index so results can be reassembled in input
order.
- List<Pair<Integer, List<F>>> groupsWithIndex = Lists.newArrayList();
+ List<Pair<Integer, List<F>>> groupsWithIndex =
Lists.newArrayListWithCapacity(groups.size());
Review Comment:
Rather than pre-sizing `groupsWithIndex`, consider removing it.
`Tasks.range(int)` already covers this case:
```java
Tasks.range(groups.size())
.stopOnFailure()
.throwFailureWhenFinished()
.executeWith(writePool)
.run(index -> results.set(index, writeFunc.apply(groups.get(index))));
```
That drops the list, the loop, one `Pair` allocation per group, and the
comment that exists only to explain the pairing, and it makes the input-order
guarantee visible directly in the `run` body.
It would also make this method consistent with the rest of the project.
`Tasks.range(n)` with the lambda indexing into the source list is the idiom
everywhere else, including the two classes in this package that do the same job
of fanning out over groups of manifests and writing results back positionally
to preserve order: `ManifestMergeManager` (`Tasks.range(bins.size())` then
`bins.get(index)`), `ManifestFilterManager`, `SnapshotProducer`, and `DVUtil`;
also `SparkPlanningUtil` and `FlinkSplitPlanner` in the engine modules.
Searching `src/main` for an index-paired list fed to `Tasks` turns up exactly
one occurrence, which is these four lines, so pre-sizing the list entrenches
the outlier instead of resolving it.
The `AtomicReferenceArray` for results is fine to keep either way.
--
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]
