JingsongLi commented on code in PR #9257:
URL: https://github.com/apache/paimon/pull/9257#discussion_r3793796883
##########
paimon-core/src/main/java/org/apache/paimon/operation/ManifestEntryRunMerge.java:
##########
@@ -234,11 +232,6 @@ private static ManifestEntryRunMergePlan discoverRuns(
ManifestFileMeta meta = section.get(manifestIndex);
Discovery.DiscoveredManifest manifest =
discovered.get(manifestIndex);
if (manifest.fragmented) {
- long entryCount = meta.numAddedFiles() +
meta.numDeletedFiles();
- inMemoryEntries += entryCount;
- if (inMemoryEntries > MAX_IN_MEMORY_FRAGMENTED_ENTRIES) {
- return null;
- }
sources.add(new
ManifestEntryRunMergePlan.Source.FragmentedManifestSpec(meta));
Review Comment:
**[P1] Keep a bounded fallback for fragmented manifests**
This now accepts every fragmented manifest based only on cursor count.
`mergeToManifest` opens all sources before emitting output, and each
`InMemoryManifestCursor` copies every full row plus a sort key and sorts the
entire manifest in heap. `MAX_STREAM_CURSORS` therefore limits file handles,
not retained bytes; `manifest.target-file-size` is only a configurable rolling
target. A large single fragmented manifest, or several in one section, can turn
the previous spillable external-sort fallback into input-sized heap retention
and OOM the commit or compaction process.
Please retain a configurable byte or entry budget and return `null` to
external sort when it is exceeded, or make the fragmented cursor spillable. Add
a production-heap stress test covering multiple fragmented manifests.
##########
paimon-core/src/test/java/org/apache/paimon/manifest/ManifestFileMetaTest.java:
##########
@@ -1922,7 +1922,7 @@ public void
testDataEvolutionManifestRunMergeFragmentedSmallManifests() {
}
@Test
- public void
testDataEvolutionManifestRunMergeFallsBackForLargeFragmentedManifest() {
+ public void
testDataEvolutionManifestRunMergeSortsLargeFragmentedManifest() {
Review Comment:
**[P2] Assert that this case actually uses run merge**
Only the test name changed; the assertions are identical to the old fallback
test. This still passes when `sortAndWriteFullEntries` returns `null` and the
caller uses external sort, so it does not protect the behavior introduced by
this PR. Please add an observable assertion for the selected path, or test plan
construction directly, and cover inputs just below and above the intended
memory budget.
--
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]
