feat(pk-index): maintain BTree indexes during compaction

[wangyong9999]

Purpose

Linked issue: N/A (follow-up to #194)

This completes the Java-compatible source-backed primary-key BTree index lifecycle for fixed-bucket primary-key tables. #194 added the read path; this PR adds the missing maintenance path:

• validate the Java-equivalent table/index prerequisites and BTree options;
• restore committed source files and primary-key source-backed payload metadata into each bucket writer, without mixing Java data-evolution (DEIX) payloads;
• read physical source rows without deletion-vector filtering, sort (value, group row id) with the existing bounded/spill-capable buffers, and build one payload per indexed field and positive data level;
• isolate synchronous build failures to one field and level, retain successful payloads, leave only failed levels uncovered for normal scan fallback, and rebuild them on a later maintenance attempt;
• reconcile missing, stale, duplicate, replaced, removed-definition, and empty-level payloads during compaction, and commit matching index ADD/DELETE entries in the same snapshot as the data changes;
• retain files referenced by current-branch live tags during snapshot expiration, reject expiration while another branch exists, and preserve the retry anchor when an external payload deletion fails; and
• document the synchronous C++ maintenance model and its current boundaries.

The implementation reuses the source metadata, BTree payload format, index manifests, commit messages, sort buffers, path factories, and reader/writer abstractions already in the repository. It adds no storage protocol, index family, background scheduler, retry policy, or manual action.

Java's asynchronous scheduling, retry/fairness policy, and manual rebuild actions are not ported. Realtime and postpone-bucket writers do not automatically build source-backed payloads. Removed-definition cleanup is emitted when a bucket is subsequently opened by write or compaction.

Tests

• Added coverage for schema validation, deterministic payload construction, field/level build-failure fallback and repair, writer restore and compaction reconciliation, removed definitions, data-evolution payload separation, current-branch tag retention, branch-safe expiration, external-delete retry, orphan cleanup, and Parquet/ORC lifecycle behavior. The ORC case forces dictionary encoding and lazy decoding.
• GCC 8.3 Debug paimon-core-test target built with the project's -Wall -Werror configuration.
• paimon-core-test: 1,777/1,777 passed.
• paimon-primary-key-sorted-index-inte-test: 12/12 passed.
• pre-commit run --all-files: all hooks passed.
• Apache RAT 0.16.1: 0 unknown licenses after excluding the linked-worktree .git pointer.
• git diff --check: passed.

API and Format

• No new supported table option or query/build API. Options::PK_CLUSTERING_OVERRIDE centralizes the existing Java-compatible key, which remains rejected by the unsupported C++ commit path.
• No new storage format or protocol. Payloads continue to use the existing BTree/global-index files, GlobalIndexMeta, primary-key source metadata v1, index manifests, and commit-message format.
• The internal restore contract explicitly requests primary-key source-backed payload scanning and excludes Java data-evolution source metadata.
• OrphanFilesCleaner supports primary-key tables and retains reachable index manifests, index payloads, and data-file extra files. It still does not enumerate global-index.external-path; snapshot expiration deletes retired external payloads by exact committed path and retains metadata for retry on failure.

Documentation

Updated docs/source/user_guide/primary_key_global_index.rst with automatic build and compaction maintenance, build-failure fallback, cleanup ownership, tag/branch expiration boundaries, spill requirements, and the synchronous/realtime/postpone limitations.

Generative AI tooling

Generated-by: Codex (GPT-5)

[wangyong9999]

skipping_sets only covers the current retained snapshot. Tags (and branches sharing the table root) can still reference an older index manifest, so this can delete a live payload; a tagged DV read then fails or loses its deletion semantics. Add all live tag/branch index manifests and payloads to the retention set, or keep expiration rejected until that traversal exists.

[wangyong9999]

Fixed for the supported scope. Expiration now uses current-branch live tags to retain their data files, manifests, and index payloads, matching Java. Cross-branch traversal remains unchanged.

[wangyong9999]

createBranch(branch, tag) copies the tag into the branch, but deleting the source-branch tag does not check branch ownership. The branch snapshot and copied tag still reference the same table-root payload, and main-branch expiration can delete it. Please traverse branch metadata or reject expiration while another branch exists.

[wangyong9999]

Fixed. Snapshot expiration now rejects tables while another branch exists, until cross-branch retention is supported.

[wangyong9999]

These batches do not bound writer memory: BTreeGlobalIndexWriter retains every row id for the current key until the key changes. A valid low-cardinality field such as BOOL can therefore hold a whole level across all spill batches, and Flush() allocates/copies another encoded buffer, so prepare-commit can OOM even with spill enabled. Spill/chunk each posting list, or enforce a controlled per-key limit.

[wangyong9999]

Confirmed: spill does not bound one posting list. Java uses the same one-key/one-posting-list BTree format; chunking or a limit would change the shared format or behavior, so this parity PR keeps it unchanged.

[wangyong9999]

The shared format does not require accepting process OOM. A hard memory-budget check can return Status before current_row_ids_ and the encoding buffer allocate; the new maintainer fallback then leaves the level uncovered. Please add that bound even if chunking is deferred.

[wangyong9999]

Confirmed, but neither implementation defines a posting-list memory budget. Reusing the sort-buffer limit would add C++-only rejection semantics, so this parity PR leaves it unchanged.

[wangyong9999]

The payload delete result is ignored, then the only manifest that records it is removed. Orphan cleanup does not enumerate the global-index external path, so a transient delete failure there becomes permanent and repeated expirations can exhaust external storage. Keep the manifest/retry record until every external payload is deleted or confirmed absent.

[wangyong9999]

This matches Java expiration: payload deletion is best-effort, followed by index-manifest cleanup. Retaining the manifest as a retry journal would change that lifecycle contract, so this remains unchanged.

[wangyong9999]

Java parity does not restore a retry path here: this PR explicitly keeps global-index external paths out of orphan cleanup. Once this manifest is deleted, a transient payload-delete failure is no longer discoverable. Keep a durable retry record or retain the manifest until external deletes succeed or are confirmed absent.

[wangyong9999]

Fixed. If an external payload delete fails and the file still exists, expiration returns an error and keeps the manifest/snapshot retry anchor; confirmed absence is accepted. Added retry coverage.

[wangyong9999]

Returning here discards increment — which writer->PrepareCommit() already drained out of the writer — and also every CommitMessage collected for the buckets processed earlier in this loop. A retry gets empty increments from those writers, so data files that were written and would have been committed silently drop out of the commit and stay on disk as orphans.

That window now holds a heavyweight, failure-prone step: a full re-read of a data level plus an external sort, which hard-fails when the sort quota is exhausted (pk_sorted_index_builder.cpp, "external-sort quota is exhausted"). A table with a level larger than write-buffer-size and no spill directory fails every commit.

The read path is already built to tolerate this — PkSortedBucketIndexState documents that levels without a valid group stay uncovered and are scanned normally, and Java builds asynchronously for the same reason. Suggest degrading a maintainer failure to "this level has no payload" (log, delete the payloads built in this attempt, keep committing), or at least running the build before the writer increment is drained.

[wangyong9999]

Fixed. BTree build failures now keep the drained data transition, clean payloads created by that attempt, and leave the level uncovered for fallback and later rebuild. Structural increment errors still propagate. Added a forced-failure regression test.

[wangyong9999]

ColumnarRowRef's constructor takes std::shared_ptr<ColumnarBatchContext> by value, so each comparison copies two shared pointers — about 2·n·log₂n atomic refcount round-trips, ~40M for a one-million-row level, none of which does any comparing.

Build the row refs once and index into them:

std::vector<ColumnarRowRef> rows;
rows.reserve(indices.size());
for (uint64_t i = 0; i < indices.size(); ++i) rows.emplace_back(value_ctx_, i);
std::stable_sort(indices.begin(), indices.end(), [&](uint64_t left, uint64_t right) {
    return sort_comparator_->CompareTo(rows[left], rows[right]) < 0;
});

[wangyong9999]

The refcount traffic is real, but prebuilding owning row refs adds O(n) retained row-view memory to the spill path. This is a performance tradeoff rather than a correctness issue, so it remains unchanged here.

[wangyong9999]

positions never reaches a consumer that uses it. The loop above already asserts physical_position == rows_read + index for every row, and physical_row_count == file->row_count, bitmap.Cardinality() == length, and the final rows_read == file->row_count pin the same property from three other directions — so the vector is always exactly [rows_read, rows_read + length). The one consumer (PkSortedIndexBuilder::Build) only compares its size() and derives the ordinal from its own rows_buffered + index.

So this materializes an int64 vector per batch and widens BatchConsumer to two parameters to carry a value nobody reads. Either use it for the ordinal (file_base + positions[i], which is what the group-row-id contract suggests) or drop the parameter and keep the contiguity assertion local to the reader.

[wangyong9999]

Fixed. The reader keeps the contiguous-position validation locally, and the unused position vector and callback parameter are removed.

[wangyong9999]

This column carries the same number the sort buffer already assigns. With last_sequence_number = -1 and rows written in group order, KeyValue.sequence_number is the group row id — that is exactly what BuildFromSortedReader reads back from record_batch->column(0).

The cost of the duplicate: an extra int64 per row, which doubles the sort-buffer footprint for a 4-byte indexed type on the one path that hard-fails when the buffer quota is exhausted, plus three comparators (comparator, sequence_comparator, in_memory_comparator) where one would do.

Dropping it should work directly: stable_sort on the value alone keeps insertion order for ties, and SortMergeReaderWithMinHeap already falls back to sequence_number when the user-defined sequence comparator is null.

[wangyong9999]

Kept to match the Java (value, rowId) sort contract and preserve row-id quota accounting. Removing it is an optimization rather than a correctness fix.

[wangyong9999]

Unreachable: PrimaryKeyIndexDefinitions::Create builds BTREE definitions by iterating schema.Fields() and testing membership in the btree column list, so any column that passed the "must reference an existing column" check a few lines above necessarily has a definition here.

Also, kPkClusteringOverride duplicates the same literal already defined in file_store_commit_impl.cpp:98.

[wangyong9999]

Fixed. The unreachable definition lookup is removed, resolved BTree definitions are validated directly, and pk-clustering-override is centralized under Options.

[wangyong9999]

Defaulting to nullptr leaves the main write path on the divergence this parameter exists to fix: WriteBuffer sorts its in-memory runs with Arrow SortIndices, while ExternalSortBuffer merges the spilled runs with FieldsComparator. The two orderings must agree or a spilled flush produces a run that is not globally sorted.

The CompareFloatingPoint change in this PR makes them definitively opposite for NaN — Arrow places NaN with nulls under NullPlacement::AtStart, FieldsComparator now places NaN last — so a float/double sort field containing NaN merges incorrectly once spilling kicks in.

Separately, that change also makes -0.0 < +0.0 where the old comparator returned 0, which changes key equality for every existing table with a float/double key or sequence field. Worth splitting out, or at least calling out in the description.

[wangyong9999]

Fixed. Java floating-point ordering is now scoped to the PK BTree in-memory and spill/merge comparators. The generic merge-tree comparator keeps its previous behavior; tests cover NaN and signed zero.

[wangyong9999]

Third copy of this literal — BtreeDefs::kIdentifier and kBTreeIndexType (primary_key_index_definitions.cpp:38) already exist, and the value has to stay in sync with PrimaryKeyIndexDefinition::IndexType() or the restore scan silently returns nothing and every level gets rebuilt on each commit. A generic WriteRestore hard-coding one index family is also the wrong layer; the index type could come from the maintainer factory, which already supplies the IndexFileHandler.

While here: scan_primary_key_indexes = false is a default argument on a virtual function (declared on both the pure virtual in write_restore.h and this override). Defaults bind statically, so a future override with a different default silently changes behavior depending on the static type. Better to make it required and update the two call sites.

[wangyong9999]

Fixed. Restore now scans source-backed entries by source_meta instead of a hard-coded index type, virtual defaults are removed, and BtreeDefs::kIdentifier is canonical.

[wangyong9999]

Two writers based on the same snapshot can each publish a different payload for the same uncovered level. The second commit does not compare source metadata, and GlobalFileNameCombiner keeps both filenames; the next restore rejects the whole level because it has two candidates and falls back to a full scan. Deduplicate or conflict-check by partition/bucket/type/field/source set during commit retry.

[wangyong9999]

Confirmed as temporary coverage loss, not result corruption: duplicate candidates invalidate that level and queries fall back to normal scanning. Java has the same source-backed behavior. Commit-time semantic arbitration would add a new conflict policy, so this parity PR leaves it unchanged.

[wangyong9999]

active_payloads_ can contain payloads for a field removed from pk-btree.index.columns, but only current fields_ populate deleted_identities, so this loop retains the old payload forever; removing the last definition skips creating the maintainer entirely. Either reject that schema change, or emit DELETEs for restored BTree payloads with no current owner, including the zero-definition case.

[wangyong9999]

Fixed. Restored PK BTree payloads with no current field owner are deleted when the bucket is next opened, including the zero-definition case. Added schema-evolution coverage.

[wangyong9999]

The break a few lines above leaves the field loop on the first failure, and this branch then deletes every payload built in this round — including the ones that already succeeded for earlier fields and for earlier levels of the failing field.

With a persistent failure that turns into permanent per-commit amplification. The likely persistent failure is exactly the one this path exists for: PkSortedIndexBuilder::Build returns Invalid when the sort quota is exhausted, so a level that does not fit the write buffer fails every time. Each commit then re-reads and re-sorts the levels that can build, writes their payloads, deletes them again, and the table ends up with no index on any level.

Make the failure granularity (field, level): keep the payloads that built plus the rejected payloads they replace, and leave only the failing level uncovered. That is also what the new class comment promises — "the affected level remains uncovered", not "every level stays uncovered".

[wangyong9999]

Fixed. Build failures are now isolated to one field and level; successful payloads and matching deletions are retained, and only the failed level remains uncovered.

[wangyong9999]

entry.index_file is dereferenced here without a null check, and so are expire_snapshots.cpp:357 and :533 — while orphan_files_cleaner_impl.cpp:304, added in the same PR, treats a null index_file as an error worth failing the scan for.

Both can't be right. If a deserialized IndexManifestEntry can carry a null index_file, these three new sites segfault on a manifest the cleaner would have rejected; if it can't, that check is dead weight. Worth picking one and applying it consistently.

[wangyong9999]

index_file is non-null by the index-manifest serializer/deserializer contract. Removed the inconsistent dead guard.

[wangyong9999]

Scoping the Java order to the index build is the right call, but note what the false branch is: compare(NaN, x) and compare(x, NaN) both return 1, so it is not a strict weak ordering, and every std::sort / std::stable_sort / heap comparator that consumes a default FieldsComparator is UB the moment a NaN shows up.

Leaving that behavior alone in this PR is fine, but the TODO that documented it ("nan cannot be compared") was deleted in the previous revision, so the branch now reads as deliberate and correct. Worth restoring a note here.

[wangyong9999]

Restored the note. PK BTree construction continues to opt into the Java floating-point order.

[wangyong9999]

Two notes on this skip:

• tagged_data_files is only recomputed when the tag changes, so one failed read is sticky for every remaining snapshot under the same tag, not just this one. Java retries tryReadDataFiles per expiring snapshot and only caches successes.
• The skip covers data-file cleanup only. The second loop still deletes this snapshot's manifests and the snapshot file itself, so the data files that were not cleaned end up unreachable — no snapshot references them and no delta manifest lists them any more. They are recoverable only through orphan cleanup.

The conservative direction is right (better an orphan than a deleted tagged file); worth saying the second part in the warning so the leak is diagnosable.

[wangyong9999]

Current Java also builds one skipper per tag per expiration, so the failure is sticky there as well. Updated the warning to state that metadata expiration continues and skipped files may remain for orphan cleanup.

[wangyong9999]

FileStoreCommitImpl::ValidateCommitOptions already rejects pk-clustering-override for every table (file_store_commit.cpp:129 runs it on every commit path), so this branch only fires for a schema whose first commit would fail anyway, and the message reads as if the option works when BTree indexes are off. Either drop it or point it at the global restriction.

[wangyong9999]

Updated the message to point to the global C++ commit-path restriction while retaining early schema validation.

[wangyong9999]

cc @lxy-9602 @SteNicholas could you please take a look when convenient? Thanks~