[core] Support manifest sort feature when commit

[discivigour]

Purpose

• add manifest-sort.enabled to enable manifest sort

Tests

• ManifestFileMetaTest.testManifestSortWithOverlappingPartitions()

[leaves12138]

Why not use CodeGenUtils.newRecordComparator?

[discivigour]

Nice point.

[leaves12138]

Do not use boolean addedToExisting.

Just
List earliestRun = runs.pool();
if (earliestRun == null) {
do something
} else if (compare(xxx) > 0) {
do something
} else {
do something
}

It makes this more pretty

[discivigour]

changed.

[leaves12138]

There is overlap in one run if "equals".

[discivigour]

I designed it this way to ensure that the minimum number of Sorted runs is built to reduce the burden of sorting.

[leaves12138]

Thanks for the update. I took another pass over the latest revision, and I think there are still a few issues that should be fixed before merging.

1. The boundary condition for interval overlap still looks wrong.

   In ManifestFileSorter.buildLevelSortedRuns, a file is appended to an existing run when file.min >= last.max. In splitIntoSections, a new section is also started when file.min >= sectionMaxBound.

   However, partition stats represent closed intervals. For example, [1, 3] and [3, 5] still overlap at partition value 3. A sorted run is documented as containing non-overlapping intervals, so this case should not be placed into the same run. Similarly, sections are used as overlap-connected rewrite units, so this case should not be split into different sections either. I think both checks should use > 0, not >= 0, and we should add a test for the max == min boundary case.

2. manifest-sort.enabled currently bypasses the original manifest compaction trigger/gate.

   ManifestFileMerger.merge directly enters ManifestFileSorter.trySortRewrite and returns from that path when manifest sort is enabled. This means the original manifest.full-compaction-threshold-size and manifest.merge-min-count behavior is no longer applied in the same way. Inside classifyManifests, files are classified only by fileSize < targetSize or delete-range overlap, so small manifests / delete manifests can trigger sort rewrite more aggressively than the existing merge logic.

   If this is intentional, I think the new semantics should be documented very clearly. Otherwise, the sort path should preserve the existing full/minor compaction gates, especially manifest.merge-min-count for minor manifest merging.

3. The partial rewrite path for manifest-sort.max-rewrite-size can break the output order.

   When the first section exceeds the rewrite budget, rewriteSections splits it into rewriteFiles and remainingFiles, rewrites the first part, and appends the remaining section to the end of the sections list. If there are later sections with larger key ranges, the remaining part of the current section will be emitted after them, which can produce an order like 0..10, 20..30, 10..20.

   To keep the manifest list sorted, I think we should either skip the whole section once the budget is exceeded, or keep the remaining section at the current position/order instead of appending it to the tail. This also needs a regression test.

4. Test coverage is still missing some important edge cases.

   The new tests cover large overlapping ranges and delete elimination, but I do not see coverage for boundary-touching intervals (max == min), manifest.merge-min-count / full threshold behavior under manifest-sort.enabled, manifest-sort.max-rewrite-size preserving global output order, or null partition values. The switch to RecordComparator is a good improvement, but a null partition test would make this safer.

[discivigour]

Thanks for the update. I took another pass over the latest revision, and I think there are still a few issues that should be fixed before merging.

1. The boundary condition for interval overlap still looks wrong.
   In ManifestFileSorter.buildLevelSortedRuns, a file is appended to an existing run when file.min >= last.max. In splitIntoSections, a new section is also started when file.min >= sectionMaxBound.
   However, partition stats represent closed intervals. For example, [1, 3] and [3, 5] still overlap at partition value 3. A sorted run is documented as containing non-overlapping intervals, so this case should not be placed into the same run. Similarly, sections are used as overlap-connected rewrite units, so this case should not be split into different sections either. I think both checks should use > 0, not >= 0, and we should add a test for the max == min boundary case.
2. manifest-sort.enabled currently bypasses the original manifest compaction trigger/gate.
   ManifestFileMerger.merge directly enters ManifestFileSorter.trySortRewrite and returns from that path when manifest sort is enabled. This means the original manifest.full-compaction-threshold-size and manifest.merge-min-count behavior is no longer applied in the same way. Inside classifyManifests, files are classified only by fileSize < targetSize or delete-range overlap, so small manifests / delete manifests can trigger sort rewrite more aggressively than the existing merge logic.
   If this is intentional, I think the new semantics should be documented very clearly. Otherwise, the sort path should preserve the existing full/minor compaction gates, especially manifest.merge-min-count for minor manifest merging.
3. The partial rewrite path for manifest-sort.max-rewrite-size can break the output order.
   When the first section exceeds the rewrite budget, rewriteSections splits it into rewriteFiles and remainingFiles, rewrites the first part, and appends the remaining section to the end of the sections list. If there are later sections with larger key ranges, the remaining part of the current section will be emitted after them, which can produce an order like 0..10, 20..30, 10..20.
   To keep the manifest list sorted, I think we should either skip the whole section once the budget is exceeded, or keep the remaining section at the current position/order instead of appending it to the tail. This also needs a regression test.
4. Test coverage is still missing some important edge cases.
   The new tests cover large overlapping ranges and delete elimination, but I do not see coverage for boundary-touching intervals (max == min), manifest.merge-min-count / full threshold behavior under manifest-sort.enabled, manifest-sort.max-rewrite-size preserving global output order, or null partition values. The switch to RecordComparator is a good improvement, but a null partition test would make this safer.

Thanks for your comment.

1. I designed it this way to ensure that the minimum number of Sorted runs is built to reduce the burden of sorting.
2. I have introduced manifestFullCompactionThresholdSize to to reduce the phenomenon of "one delete entry causing large-scale file rewriting.
3. I don‘t think it is a problem.
4. I will add more tests.

[leaves12138]

LGTM.
Please check the two questions:
1、If sorted run has files overlap, is it correct?
2、If delete comes, how to deal with it.
Nothing else to me.

[discivigour]

LGTM. Please check the two questions: 1、If sorted run has files overlap, is it correct? 2、If delete comes, how to deal with it. Nothing else to me.

1. If the file endpoints in SortedRun overlap, it does not affect the subsequent reconstruction of the LSM Tree. In addition, when users filter through partitions, they will only read 1-2 more files.
2. If a delete entry comes, it will first determine whether achieve manifestFullCompactionThresholdSize.If achieved, will eliminate all the delete; If not reach, the meta related to the delete entry partition will be retained and not participate in the sorting to prevent the order of add and delete from being disrupted

[leaves12138]

Rename to triggerFullCompact

[leaves12138]

Why read delete every time? If not full compaction, we still need to read all the deletes?

[leaves12138]

Split full compaction and minor compaction, don't make it mixed. Refer to ManifestFileMerge.merge

[discivigour]

Thanks, I will split full compaction and minor compaction.

[JingsongLi]

Review: [core] Support manifest sort feature when commit

Overall this is a substantial feature that models manifest files like an LSM tree and applies sorted compaction by partition field. The design is well-structured with clear separation of concerns (ManifestFileSorter, ManifestPickStrategy, ManifestAdjacentSortedRun). Below are findings that I believe warrant discussion.

---

Correctness Concerns

1. Modifying sections list during iteration (ManifestFileSorter.rewriteSections)

for (int i = 0; i < sections.size(); i++) {
    ...
    if (!remainingFiles.isEmpty()) {
        Section remainingSection = new Section(remainingFiles, remainingSize, remainingHasDefault);
        sections.add(remainingSection);  // <-- adds to the list being iterated
    }
    reachedLimit = true;
}

This works because sections.size() is re-evaluated each iteration, but it is fragile and difficult to reason about. If a future change introduces another path that adds to sections, this could loop indefinitely. Consider collecting remaining sections separately and processing them after the main loop.

2. Boundary equality semantics inconsistency

In buildLevelSortedRuns, min == max (boundary equality) means files are non-overlapping and placed in the same SortedRun:

// >= 0 means non-overlapping
if (fieldComparator.compare(file.min, earliestRun.last.max) >= 0) { ... }

But in splitIntoSections, min == sectionMaxBound causes a new section to be created:

// >= 0 means separate sections
if (fieldComparator.compare(file.min, sectionMaxBound) >= 0) { ... }

The two algorithms disagree on what "boundary equality" means. This inconsistency could cause a single SortedRun's files to be split across multiple sections. While this may not cause data loss, it could reduce compaction effectiveness and merits a clear comment explaining the deliberate divergence.

3. ManifestAdjacentSortedRun mutable level field used in equals/hashCode

setLevel() mutates the object after construction, and level participates in equals()/hashCode(). These objects are placed into a HashSet<ManifestAdjacentSortedRun> pickedSet. If setLevel() were ever called after insertion into the set, lookups would silently break. Current code assigns levels before building the set, so it works today, but this is a latent bug waiting to happen. Consider making level part of a builder/factory pattern or removing it from equals/hashCode.

---

Design Observations

4. Reusing maxSizeAmplificationPercent and sortedRunSizeRatio from data-file compaction

These options (num-sorted-run.size-ratio, sort-spill.threshold) were designed for data-file universal compaction. Reusing them for manifest file LSM compaction couples two unrelated subsystems. The optimal ratio for data files (e.g., default 200%) is likely not appropriate for manifest files which have very different I/O characteristics. Consider introducing dedicated options (e.g., manifest-sort.size-amplification-percent, manifest-sort.size-ratio) with appropriate defaults.

5. ManifestFileMerger.merge() API change

The signature change from explicit parameters to CoreOptions makes the API less explicit and harder to unit-test in isolation. The workaround in compactManifestOnce():

Options compactOptions = Options.fromMap(options.toMap());
compactOptions.set(CoreOptions.MANIFEST_MERGE_MIN_COUNT, 1);
compactOptions.set(CoreOptions.MANIFEST_FULL_COMPACTION_FILE_SIZE, MemorySize.ofBytes(1));

copies the entire options map just to override two values, which is heavier than the previous approach of passing parameters directly. An overload that preserves the old signature (calling into the new one) would avoid this.

6. Memory pressure with large manifest sets

sortAndRewriteFull and sortAndRewriteMinor read all entries from all picked files into memory for sorting. While maxRewriteSize bounds total file size, manifest files can contain a very large number of entries. For tables with millions of data files, this could cause significant GC pressure or OOM. Consider a streaming merge-sort or spill-to-disk approach for very large sections, or at minimum document the memory implication in the option description.

---

Minor Issues

7. resolveSortField will throw IndexOutOfBoundsException if called with an empty partitionType and null sortPartitionField. The caller guards against this (partitionType.getFieldCount() > 0), but the method itself is package-private and could be called from tests or future code without that guard. A defensive check would be safer.

8. ManifestPickStrategy.MAX_LEVEL = 4 is hardcoded. The number of levels directly affects compaction aggressiveness. Consider making this configurable or at least documenting why 4 is the right constant.

9. Visibility changes in ManifestFileMerger (computeDeletePartitions, FullCompactionReadResult from private to package-private) should be documented as intentional cross-class sharing rather than left implicit.

---

Test Coverage

The tests cover the key scenarios (overlapping partitions, DELETE elimination, multi-field partitions, schema validation). However, I'd suggest adding:

• A test that verifies behavior when maxRewriteSize is hit mid-section (the partial rewrite path).
• A test for the minor compaction path where ADD+DELETE pairs span different sections to verify no data loss.
• A test with a single-file input to verify no unnecessary rewrites.

---

Overall the feature is well-thought-out. The main actionable items are the boundary equality inconsistency (#2), the mutable-hashCode issue (#3), and the option reuse concern (#4). Nice work on the LSM-based approach for manifest organization.

[JingsongLi]

Thanks for the update. I took another pass over the latest revision and found a few issues that I think should be addressed before merging.

1. manifest-sort.max-rewrite-size can be exceeded and can reorder the remaining section.

   In ManifestFileSorter.rewriteSections, when a section is larger than the remaining budget, the code splits it into rewriteFiles and remainingFiles, rewrites rewriteFiles, then appends remainingFiles to the end of sections. If the remaining section contains default-compaction files, it can still be rewritten later through rewriteSubSegments, so the configured max rewrite size is no longer a hard cap. Also, appending the remaining range after later sections can produce an output order such as 0..10, 20..30, 10..20. I think we should either skip the whole over-budget section, or keep the remaining files unchanged in the original position/order. This needs a regression test for the budget-hit path.

2. manifest-sort.enabled changes the existing minor manifest merge trigger semantics.

   Once manifest sort is enabled, ManifestFileMerger.merge bypasses the old full/minor compaction path and always enters ManifestFileSorter.trySortCompaction. The minor sort path classifies files by fileSize < targetSize and delete-range overlap, but it does not preserve the previous manifest.merge-min-count gate in the same way. This can rewrite small manifests more aggressively than users expect from existing manifest merge behavior. If this is intentional, the option documentation should state the changed semantics clearly; otherwise the sort path should preserve the old minor merge trigger.

3. Boundary-touching partition ranges are treated as non-overlapping.

   buildLevelSortedRuns appends a file to a run when file.min >= last.max, and splitIntoSections starts a new section when file.min >= sectionMaxBound. Manifest partition stats are closed intervals, so [1, 3] and [3, 5] still overlap at partition value 3. This conflicts with the ManifestAdjacentSortedRun contract that intervals do not overlap on the sort field. I think these comparisons should use > 0, or the deliberate relaxed invariant should be documented very explicitly and covered by boundary tests.

[discivigour]

Thanks for the update. I took another pass over the latest revision and found a few issues that I think should be addressed before merging.

1. manifest-sort.max-rewrite-size can be exceeded and can reorder the remaining section.
   In ManifestFileSorter.rewriteSections, when a section is larger than the remaining budget, the code splits it into rewriteFiles and remainingFiles, rewrites rewriteFiles, then appends remainingFiles to the end of sections. If the remaining section contains default-compaction files, it can still be rewritten later through rewriteSubSegments, so the configured max rewrite size is no longer a hard cap. Also, appending the remaining range after later sections can produce an output order such as 0..10, 20..30, 10..20. I think we should either skip the whole over-budget section, or keep the remaining files unchanged in the original position/order. This needs a regression test for the budget-hit path.
2. manifest-sort.enabled changes the existing minor manifest merge trigger semantics.
   Once manifest sort is enabled, ManifestFileMerger.merge bypasses the old full/minor compaction path and always enters ManifestFileSorter.trySortCompaction. The minor sort path classifies files by fileSize < targetSize and delete-range overlap, but it does not preserve the previous manifest.merge-min-count gate in the same way. This can rewrite small manifests more aggressively than users expect from existing manifest merge behavior. If this is intentional, the option documentation should state the changed semantics clearly; otherwise the sort path should preserve the old minor merge trigger.
3. Boundary-touching partition ranges are treated as non-overlapping.
   buildLevelSortedRuns appends a file to a run when file.min >= last.max, and splitIntoSections starts a new section when file.min >= sectionMaxBound. Manifest partition stats are closed intervals, so [1, 3] and [3, 5] still overlap at partition value 3. This conflicts with the ManifestAdjacentSortedRun contract that intervals do not overlap on the sort field. I think these comparisons should use > 0, or the deliberate relaxed invariant should be documented very explicitly and covered by boundary tests.

Thanks for your comment.

1. The files within the manifest-sort.max-rewriting-size threshold are sorted, while those outside the threshold are merged into small files and deleted. This is intentional.
2. SortCompact has completely rewritten Manifest Merge, and the related parameters can still be used normally.
3. I designed it this way to ensure that the minimum number of Sorted runs is built to reduce the burden of sorting. The endpoints of intervals between sections do not overlap because there is no need to merge these sections

[JingsongLi]

Thanks for the clarification. I understand these are intentional design choices, but I still think we need to make the contract explicit and add regression coverage, otherwise this will be hard to reason about later.

1. About manifest-sort.max-rewrite-size: if files outside the threshold may still be merged/deleted, then the current option name/description is misleading. It says this is the maximum total size of manifest files to rewrite in a single sort rewrite pass and that sections exceeding the limit are skipped. However, the current code can still process the remaining part later through rewriteSubSegments when hasDefaultCompactMeta is true. So either the code should enforce the cap strictly, or the documentation should say that the cap only limits the sorted rewrite portion and default/minor cleanup may still happen beyond it.

   I am also still worried about ordering. Appending remainingSection to the tail means the remaining key range from the current section is emitted after later sections. If global manifest order by sort key is not a required invariant, please document that explicitly. If it is an invariant, we should keep the remaining files in their original position/order. A test that hits the budget in the middle of a section and verifies the final manifest order would settle this.

2. About manifest.merge-min-count: saying the parameters can still be used normally is not enough for me to verify the behavior. In the old minor merge path, the trailing candidates are kept unchanged when their count is below manifest.merge-min-count. In the sort path, a non-empty defaultCompactionMap can trigger rewrite even when the number of small manifests is below that threshold. If the intended behavior is different from the old manifest merge, that is fine, but then it should be documented as a semantic change and covered by a test. Otherwise we should preserve the old gate.

3. About boundary equality: reducing the number of SortedRuns is a reasonable optimization, but the current class comment says the intervals do not overlap. With closed partition ranges, [1, 3] and [3, 5] overlap at 3; they do not just touch in an open-interval sense. If the intended invariant allows boundary-touching intervals in one run/section, please update the comments to state that explicitly and add a boundary test. Otherwise future readers will assume the stricter non-overlap invariant and may build incorrect logic on top of it.

So my ask is not necessarily to reject the design, but to make the semantics precise and backed by tests for these three edge cases.

[discivigour]

Thanks for the clarification. I understand these are intentional design choices, but I still think we need to make the contract explicit and add regression coverage, otherwise this will be hard to reason about later.

1. About manifest-sort.max-rewrite-size: if files outside the threshold may still be merged/deleted, then the current option name/description is misleading. It says this is the maximum total size of manifest files to rewrite in a single sort rewrite pass and that sections exceeding the limit are skipped. However, the current code can still process the remaining part later through rewriteSubSegments when hasDefaultCompactMeta is true. So either the code should enforce the cap strictly, or the documentation should say that the cap only limits the sorted rewrite portion and default/minor cleanup may still happen beyond it.
   I am also still worried about ordering. Appending remainingSection to the tail means the remaining key range from the current section is emitted after later sections. If global manifest order by sort key is not a required invariant, please document that explicitly. If it is an invariant, we should keep the remaining files in their original position/order. A test that hits the budget in the middle of a section and verifies the final manifest order would settle this.
2. About manifest.merge-min-count: saying the parameters can still be used normally is not enough for me to verify the behavior. In the old minor merge path, the trailing candidates are kept unchanged when their count is below manifest.merge-min-count. In the sort path, a non-empty defaultCompactionMap can trigger rewrite even when the number of small manifests is below that threshold. If the intended behavior is different from the old manifest merge, that is fine, but then it should be documented as a semantic change and covered by a test. Otherwise we should preserve the old gate.
3. About boundary equality: reducing the number of SortedRuns is a reasonable optimization, but the current class comment says the intervals do not overlap. With closed partition ranges, [1, 3] and [3, 5] overlap at 3; they do not just touch in an open-interval sense. If the intended invariant allows boundary-touching intervals in one run/section, please update the comments to state that explicitly and add a boundary test. Otherwise future readers will assume the stricter non-overlap invariant and may build incorrect logic on top of it.

So my ask is not necessarily to reject the design, but to make the semantics precise and backed by tests for these three edge cases.

1. add test and comment
2. It is triggered when the small file is smaller than suggestedMinMetaCount because !ctx.deleteEntries.isEmpty() means full compact, suggestedMinMetaCount only in minor compact when used.
3. add test and comment

[JingsongLi]

Thanks for adding the tests and comments. The clarification for manifest-sort.max-rewrite-size and boundary equality helps.

I still think point 2 is not fully addressed. Your explanation about rewriteSubSegments is correct for the overflow/fallback path, but the normal under-budget path does not go through that tail check. In rewriteSections, when processedSize + section.totalSize <= maxRewriteSize, it directly calls sortAndRewriteSection(section, ...) without checking suggestedMinMetaCount. Since minor sort compaction puts all files with fileSize < suggestedMetaSize into defaultCompactionMap, two small manifest files can be rewritten into one new manifest even when manifest.merge-min-count = 3 and full compaction is not triggered. In the old minor merge path, that trailing candidate group would be kept unchanged.

I verified this locally with a small regression case: two small ADD-only manifests, manifest.target-file-size = 10MB, manifest.merge-min-count = 3, and manifest.full-compaction-threshold-size = Long.MAX_VALUE. With manifest-sort.enabled = true, the result is one newly written manifest instead of the original two manifests.

So either the sort path should preserve the old minor merge gate before rewriting small under-budget sections, or the option documentation/tests should explicitly state this semantic change for manifest.merge-min-count under manifest-sort.enabled.

[discivigour]

Thanks for adding the tests and comments. The clarification for manifest-sort.max-rewrite-size and boundary equality helps.

I still think point 2 is not fully addressed. Your explanation about rewriteSubSegments is correct for the overflow/fallback path, but the normal under-budget path does not go through that tail check. In rewriteSections, when processedSize + section.totalSize <= maxRewriteSize, it directly calls sortAndRewriteSection(section, ...) without checking suggestedMinMetaCount. Since minor sort compaction puts all files with fileSize < suggestedMetaSize into defaultCompactionMap, two small manifest files can be rewritten into one new manifest even when manifest.merge-min-count = 3 and full compaction is not triggered. In the old minor merge path, that trailing candidate group would be kept unchanged.

I verified this locally with a small regression case: two small ADD-only manifests, manifest.target-file-size = 10MB, manifest.merge-min-count = 3, and manifest.full-compaction-threshold-size = Long.MAX_VALUE. With manifest-sort.enabled = true, the result is one newly written manifest instead of the original two manifests.

So either the sort path should preserve the old minor merge gate before rewriting small under-budget sections, or the option documentation/tests should explicitly state this semantic change for manifest.merge-min-count under manifest-sort.enabled.

ok, comments for the params added.