[xiangfu0]

Motivation

Servers hosting tens of thousands of sealed segments retain per-segment schemas, column maps, and statistics on heap before queries run. In a metadata-only scale probe that repeated 16 sampled metadata/index-map pairs to model 50,000 segments, post-GC heap fell from 2,561.23 MiB with eager metadata to 126.55 MiB with mapped metadata (95.1%). Moving stable per-column statistics into a local, file-backed sidecar leaves more heap available for query execution while metadata.properties remains canonical. The probe did not measure full-server or query behavior.

Change

Sealed segments currently retain a Schema, a sorted column map, and per-column metadata on heap. When enabled, the serving loader opens a versioned, read-only segment_metadata.bin sidecar after preprocessing. Fixed-width column statistics are read from the sidecar buffer, and column metadata views are created when requested; the normal serving loader requests every physical column while building index containers. metadata.properties remains canonical. Missing, stale, or corrupt sidecars rebuild locally; if rebuild or sidecar open fails, serving keeps the eager metadata so the segment remains available. For first-boot generation, the final serving loader captures the canonical metadata.properties and index_map bytes used for eager parsing. It reuses that parsed view only while both source fingerprints and the IndexService identity still match; otherwise it reparses canonical metadata. The transient snapshot is discarded after mapped open, reload, or close. Realtime metadata and callers requesting a full schema or column map retain their existing APIs.

On-disk layout and serving

segment_metadata.bin is a disposable local cache in initial big-endian format v1, limited to 2 GiB:

Region	Contents
64-byte header	Magic and version, total length, length/CRC32C fingerprints of metadata.properties and optional index_map plus registered index IDs, full-file CRC32C (with its own field zeroed), segment-properties offset/length, column count, and table/blob offsets.
180 bytes per column	One fixed record for each sorted, unique column: flags, encoding, document/cardinality/multivalue statistics, typed min/max values, sizes, and absolute offset/length pairs for variable fields.
Blob area	UTF-8 column names, FieldSpec JSON, variable-length bounds, partition/sparse metadata, compression and index sizes, and counted segment-property key/value data.

The fixed record stride and validated absolute offsets make the file directly addressable: it has no Java object pointers to relocate or object graph to deserialize. PinotDataBuffer.mapFile opens it read-only in mmap mode; HEAP read mode loads the sidecar into a direct buffer without an additional mapping. A sorted name array resolves a requested column to a record ordinal, and the column wrapper reads fixed-width values from the backing record. Variable values are decoded on demand. This is how the sidecar serves per-column statistics during segment use, rather than merely storing a copy that must be expanded into one heap object per column. Names, eligible shared FieldSpecs, segment-level fields, and any requested wrappers still use heap; mmap mode uses file-backed bytes that may use OS page cache/RSS; HEAP mode uses direct memory.

The writer derives the sidecar from canonical metadata and replaces it via a temporary file and rename. Normal writes delete their own temporary file in a finally block, and directory copies exclude these files. An orphan from a terminated process can remain in an active segment directory; sweeping there requires writer/cleaner coordination so another active writer file is never deleted. Open validates the version, source fingerprints, CRC, bounds, column order, and typed values; after opening it verifies the buffer bytes and source fingerprints again before publishing the view. A same-version file can be opened by newer code after validation. A different version is treated as a cache miss and rebuilt from metadata.properties and the optional index_map, not decoded as an unknown binary layout. Future format or derived-value parser changes require a version bump. Numeric FieldSpec defaults and TIMESTAMP default epochs are encoded as text to preserve their exact values. A checksum-valid sidecar with unreadable FieldSpec JSON is rebuilt once from canonical metadata. The segment directory closes the buffer after readers retire. Complex FieldSpecs stay segment-local because their child map is mutable. The validation path temporarily reads the full sidecar into a heap byte[], so peak startup allocation is distinct from retained heap.

Sidecar loading is opt-in for serving loads with an instance config; unconfigured reader and tool loads stay eager. Set pinot.server.instance.segment.metadata.mmap.enabled=true on a server instance to enable it. The default remains eager until representative full-server startup and Linux VMA headroom have been measured. Peer-download and deep-store-upload archives exclude the root sidecar and its in-progress temporary file, so the cache remains local to each server.

Disk-size reporting includes this local sidecar for v1/v2 segments, which measure the segment root. Normal v3 segments measure their v3/ subdirectory, so they do not count the root sidecar. The effect on size-based rebalance decisions has not been separately measured.

A standalone caller owns and closes its mapping:

try (SegmentMetadataImpl metadata = SegmentMetadataImpl.loadMapped(segmentDir)) {
  ColumnMetadata column = metadata.getColumnMetadataFor("myColumn");
}

Heap benchmark and validation

Three separate JDK 25 JVMs per mode loaded the same 128 synthetic v1 segments with 1,000 INT columns each. With -Xms2g -Xmx2g, the probe retained all segments, performed three GCs, and measured whole-JVM used heap. The primary before/after comparison uses eager loading at the PR merge-base 2877723da384 and mapped loading at commit 387ec7358907; mapped runs reused valid sidecars (251,241 bytes per segment). A same-commit eager control measured 35.669 MiB, within 0.005 MiB of the merge-base result. Those later fixes were not part of the older measurement; a separate current-head synthetic full-segment probe appears below.

Retained graph	Eager median	Mapped median	Reduction
Segment metadata; one column lookup per segment (merge-base vs benchmarked commit)	35.673 MiB	13.910 MiB	21.763 MiB (61.0%)
Serving-style segment wrappers and all column entries (same benchmarked commit)	49.209 MiB	32.160 MiB	17.049 MiB (34.6%)

These are synthetic post-GC retained-heap results, not full-server measurements. The serving-style probe builds ImmutableSegmentImpl and column containers but opens no real index readers or queries. The current-head full-segment synthetic probe below adds index readers and in-process queries; production server readiness and representative query latency remain unmeasured. The separate metadata-only probe below reports local JVM load peaks, process RSS, and post-load mapped-file footprint.

An earlier Unit attempt reported a PinotSinkIntegrationTest metadata 404, and the next attempt passed that class. The original cause remains unattributed; a bounded path audit found no concrete coupling to this feature.

Review fixes preserve TIMESTAMP default epochs, cache column names and compact index sizes, restore the protected upsert Set field, return detached mapped metadata from the instance APIs, and reuse parsed index sizes when building the sidecar. Earlier local validation covered 57 focused tests across segment metadata, upsert, and server. The final v1 format and detached-copy changes passed all 15 SegmentMetadataImplTest cases and affected Spotless, Checkstyle, and license checks. Commit 47e02405cd adds guarded reuse of the final eager parse; 18 SegmentMetadataImplTest cases and LoaderTest#testLoad passed with fresh compilation and the affected Spotless, Checkstyle, and license checks. Hosted CI is tracked on the current head in Checks; maintainer review remains pending. The prior StarTree S3 test run is older-head evidence.

Production-metadata-only scale probe

Sixteen completed v3 metadata/index-map pairs from one production table were selected from a small S3-key sample and copied into separate local directories, weighted and interleaved to model 1,000, 10,000, and 50,000 segments. This is not a statistical sample of the table. The fixture contains metadata.properties, index_map, and creation.meta; no index data or queries were loaded. Valid sidecars were built before the timed loads. The per-segment column counts of these 16 pairs are not reported; the 1,000-column width above applies only to the synthetic v1 probe. Merge-base eager and benchmarked-commit mapped used the same fixture and JDK 25 settings (-Xms1g -Xmx8g). The 1,000-segment values are medians of three fresh JVMs per mode; 10,000 and 50,000 are one fresh JVM per mode. Benchmarked-commit eager controls at 1,000 and 10,000 were close to merge-base eager.

Staged segments	Eager post-GC heap	Mapped post-GC heap	Heap reduction	Eager → mapped sampled peak RSS	Eager → mapped mapped-file regions
1,000	62.07 MiB	14.67 MiB	76.4%	801.39 → 610.98 MiB	7 → 1,007
10,000	525.51 MiB	40.26 MiB	92.3%	1,095.91 → 1,442.84 MiB	8 → 10,007
50,000	2,561.23 MiB	126.55 MiB	95.1%	3,718.33 → 1,380.86 MiB	8 → 50,007

At 50,000, sampled peak heap was 4,662.66 → 711.42 MiB, post-load macOS vmmap physical footprint was 5.0 GiB → 875.4 MiB, and metadata-only load time was 528.10 → 78.52 seconds. The 50,000 sidecars used 3.40 GiB on disk. The single 10,000-segment run had higher sampled peak RSS in mapped mode despite lower post-load footprint, so peak RSS needs repeated measurement. These copied samples are not 50,000 independent production segments or full-server startup/query results. The timed probe does not include serving-loader preprocessing or index initialization; sampled peaks may miss shorter spikes.

Each segment in mmap mode adds roughly one metadata mapping. HEAP mode uses a direct buffer for the sidecar without an additional mapping. This macOS probe cannot establish Linux VMA headroom: before a 50,000-segment-per-server rollout, verify /proc/sys/vm/max_map_count and /proc/<pid>/maps on the target server, including existing index/cache mappings and reload overlap.

Preprocessing and serving-load performance gaps

Preprocessing uses eager SegmentMetadataImpl and updates canonical metadata.properties before the mapped serving open. Some index handlers call SegmentMetadataUtils.updateMetadataProperties for individual columns; each call rereads and rewrites the entire properties file and eagerly reloads metadata for every column. If 100 column updates each call this helper, they can cause about 100 full-segment reloads plus the preprocessor's phase reloads. This pre-existing cost was excluded from the reported probes. Batching must preserve the forward-index metadata reload before dependent inverted, range, and FST index handlers run. The binary sidecar is opened or rebuilt only after preprocessing, rather than after each property update.

On a missing or stale sidecar, generation reuses the final loader's parsed eager metadata/configuration when its captured source fingerprints and index registry still match; otherwise it reparses canonical metadata. It serializes records and blobs in temporary heap arrays, computes a CRC, writes and renames a temporary file, then rereads, validates, and opens it. A reused sidecar still fingerprints the canonical metadata.properties and optional index_map, reads and validates the complete binary file into a temporary heap array, compares it byte for byte with the opened buffer, rechecks the source fingerprints, and parses FieldSpecs. Mapped getSchema() and getColumnMetadataMap() also materialize compatibility objects on each call. Parallel preload workers can overlap these allocations and I/O; concurrent loaders of one segment may redundantly generate the same cache. Sidecar opening occurs after the preprocessing throttle, so that throttle does not limit this overlap.

The earlier metadata-only probes did not measure binary generation time or realistic cold/reused serving startup. The current-head synthetic full-segment probe below covers these paths in-process at bounded scale. Before broad enablement, compare actual server startup and readiness time, peak heap, GC, RSS, and I/O under representative preprocessing and preload concurrency; compare production short queries hitting the same segments because scalar metadata reads synchronize on each mapped store; and confirm retained heap after compression-summary/index-size access and min/max pruning warm the lazy caches. The synthetic and metadata-only numbers do not establish production serving costs.

Current-head synthetic full-segment probe

At commit 47e0240 on Temurin JDK 25/macOS, three fresh JVMs per cell loaded 32 copies of one complete v3 segment with 1,000 INT columns and 100 rows. This fixture is TogetherAI-like in width only; it is not a production segment. The in-process loader opened real index readers. Ready measures load submission through all segment opens and one successful min/max-pruned query, not server/pod readiness. Mapped cold had no sidecars; mapped reused used sidecars from a prior run. The OS page cache was not reset. Load-phase heap was sampled every 5 ms, RSS about every 100 ms; brief spikes may be missed. Values below are medians of three independent JVMs.

Load workers	Mode	Ready (s)	Sampled peak heap (MiB)	Sampled peak RSS (MiB)	Load GC count / ms	Warm retained heap increase (MiB)
1	Eager	3.617	220.9	441.3	18 / 44	30.62
1	Mapped cold	3.852	213.0	433.4	20 / 51	26.66
1	Mapped reused	3.696	209.4	454.2	21 / 48	26.69
4	Eager	2.110	379.1	682.9	14 / 51	30.57
4	Mapped cold	2.224	404.1	737.0	15 / 55	26.65
4	Mapped reused	2.100	404.5	710.3	14 / 54	26.57

The retained-heap increase follows actual SegmentCompressionStatsReader.read calls with both summary modes, then all column bounds and index sizes, and three explicit GCs. Mapped retained about 4 MiB less heap across 32 copies (about 13% of the eager increase). Cold sidecar creation increased median in-process readiness by about 5–7%; four-worker mapped loads had higher sampled peak heap/RSS. Reused-sidecar readiness ranges overlapped eager.

One complete segment also ran filtered COUNT, filtered SUM, and min/max-pruned COUNT through in-process plans, with 100 queries per worker and three JVMs per mode. Median per-run p95 latency (microseconds) was:

Query workers	Eager	Mapped cold	Mapped reused
1	213	221	232
4	163	158	165
8	174	186	184
16	309	253	266

This tiny synthetic query mix showed no clear latency regression. The 32 load segments were identical copies; the probe does not cover actual Pinot server/pod readiness, production data/index distribution, representative queries, 50,000-segment load, Linux VMA headroom, or cold OS page cache. Those remain rollout gates; the feature stays opt-in. The harness and raw logs are archived outside the Git repository for follow-up.

[xiangfu0]

Review verdict: no blocking issues — looks good to merge. (Posting as a COMMENT review rather than an explicit approval because this session's token is the PR author, and GitHub does not allow self-approval.)

Really nice piece of work — I traced the format versioning, the mmap lifecycle, and the backward-compat behavior across broker/server/controller, and it holds together well. A few highlights from the review:

• Format versioning is sound. The sidecar is a purely local, non-shipped derived cache: magic + VERSION + length + source fingerprints + payload CRC32C are all checked, and any mismatch (including an old/newer version) rebuilds from the canonical metadata.properties rather than mis-parsing. Bumping VERSION rebuilds rather than migrates, and the tests cover the v2→rebuild case.
• FieldSpec JSON round-trip is correct, including the TimeFieldSpec discriminator omission (top-level and nested under ComplexFieldSpec), and ComplexFieldSpec is correctly excluded from interning.
• mmap lifecycle is safe. The metadata mapping is released exactly once on every unload path and is protected by the same segment reference-count invariant that already guards the column-index mmaps — destroy() only runs at refcount 0, so no query can read a freed mapping. The 3-attempt open() loop has no buffer leak.
• Untrusted-parse path is well-bounded — no OOB reads, negative array sizes, or infinite loops; float/double min-max round-trip through raw bits is exact (NaN/-0.0 preserved).

I only found a handful of minor, non-blocking items (inline). None of them block merge. Approving — thanks for the thorough tests and the clear header documentation.

[xiangfu0]

(Duplicate of the review above — submitted twice by accident. See the full review and inline comments in the companion review on this PR.)

[codecov-commenter]

Codecov Report

❌ Patch coverage is 54.94186% with 465 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.21%. Comparing base (c91d0e8) to head (fdafe1c).
⚠️ Report is 11 commits behind head on master.

Files with missing lines	Patch %	Lines
...spi/index/metadata/MappedSegmentMetadataStore.java	60.58%	122 Missing and 79 partials ⚠️
...gment/spi/index/metadata/MappedColumnMetadata.java	14.18%	117 Missing and 4 partials ⚠️
...egment/spi/index/metadata/SegmentMetadataImpl.java	46.92%	84 Missing and 11 partials ⚠️
...segment/spi/index/metadata/ColumnMetadataImpl.java	0.00%	13 Missing ⚠️
...cal/upsert/BasePartitionUpsertMetadataManager.java	81.81%	3 Missing and 3 partials ⚠️
...server/starter/helix/HelixInstanceDataManager.java	0.00%	6 Missing ⚠️
...indexsegment/immutable/ImmutableSegmentLoader.java	72.22%	3 Missing and 2 partials ⚠️
...a/org/apache/pinot/segment/spi/ColumnMetadata.java	0.00%	5 Missing ⚠️
...ocal/indexsegment/immutable/EmptyIndexSegment.java	40.00%	3 Missing ⚠️
.../local/loader/TierBasedSegmentDirectoryLoader.java	0.00%	2 Missing and 1 partial ⚠️
... and 4 more

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #19998      +/-   ##
============================================
+ Coverage     70.14%   70.21%   +0.06%     
  Complexity     1522     1522              
============================================
  Files          3560     3564       +4     
  Lines        236798   237875    +1077     
  Branches      38054    38247     +193     
============================================
+ Hits         166098   167013     +915     
- Misses        57996    58078      +82     
- Partials      12704    12784      +80     

Flag	Coverage Δ
integration	100.00% <ø> (ø)
integration1	100.00% <ø> (ø)
integration2	0.00% <ø> (ø)
java-25	70.21% <54.94%> (+0.06%)	⬆️
lane-a	100.00% <ø> (ø)
lane-b	0.00% <ø> (ø)
temurin	70.21% <54.94%> (+0.06%)	⬆️
unittests	70.20% <54.94%> (+0.06%)	⬆️
unittests1	59.32% <49.80%> (-0.01%)	⬇️
unittests2	41.89% <13.95%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[xiangfu0]

Verdict: LGTM / would approve (posting as a comment because GitHub blocks the authenticated account from formally approving its own PR in this environment).

Reviewed the updated head (eae234d). I focused on the areas most at risk: the new binary sidecar format, backward/forward compatibility of the SPI changes, mixed-version behavior, the mmap lifecycle, and the lock-free caches.

The design holds up well:

• Format: header (64B) and record (180B) offset layouts are self-consistent between build() and validate()/open(), with no field overlaps. Fixed-width stat reads (int/float bit tricks, min/max tags) decode correctly, and validate() runs before any view is published so the raw readString/copyTo paths are always bounds-guarded. The payload CRC plus the exact structural arithmetic (table + count*RECORD_SIZE == blobStart) make a header bit-flip effectively non-exploitable.
• Compatibility: the sidecar is a per-server local derived cache; metadata.properties stays canonical, and a wrong/newer VERSION or stale fingerprint triggers a local rebuild rather than a migration. So mixed-version servers can't corrupt each other. The new ColumnMetadata methods are default-implemented, so plugin impls stay source/binary compatible.
• mmap lifecycle: unmap happens in SegmentDirectory.close() → SegmentMetadataImpl.close(), reached only via destroy() after the segment's reference count hits 0. Queries pin the segment through the SegmentDataManager for their whole lifetime, so pruner/execution reads of a MappedColumnMetadata view can't overlap the unmap. No use-after-unmap in the serving path.
• Concurrency: the AtomicReferenceArray CAS memoization and the volatile lock-free caches in MappedColumnMetadata are correct (idempotent recompute, safe publication).

No correctness, compatibility, or lifecycle defects found. I left a few minor hardening/perf nits inline — none blocking. Approving.

[xiangfu0]

Reviewed the mapped sidecar end to end (encode/decode fidelity, mmap lifecycle, concurrency, perf, back-compat). The design is solid and the round-trip test coverage is strong. One thing I'd like to resolve before merge is the always-on load behavior (inline), plus a couple of minor notes. Nice work on keeping metadata.properties canonical and making the sidecar a rebuildable cache.

[xiangfu0]

Reviewed the current HEAD focusing on the mapped-sidecar format, concurrency/lifecycle, and null/numeric correctness. Really nice work — the format is versioned and self-describing, stale/corrupt sidecars rebuild from canonical metadata.properties, and the min/max round-trip preserves exact numeric fidelity (incl. NaN/-0.0). Lifecycle looks safe: load is single-threaded and the segment is published through the ConcurrentHashMap handoff, and the mapped buffer is unmapped on the same refcount-gated destroy() path that already protects the column data buffers.

One thing worth addressing before merge (MAJOR): the sidecar is written into the live serving segment directory, and the peer-download / segment-upload paths in TablesResource re-tar that directory recursively with no filename filter (createCompressedTarFile at ~L622 and ~L1112, via TarCompressionUtils.addFileToCompressedTar). So segment_metadata.bin gets baked into the deep-store copy and shipped on every peer download. It's not a correctness bug — CRC comes from creation.meta and the receiver re-validates/rebuilds the sidecar against its own metadata.properties, so it self-heals — but it violates the "purely local cache" invariant the class javadoc states and pollutes deep store / wastes transfer bandwidth. Suggest excluding MappedSegmentMetadataStore.FILE_NAME at those two tar sites (and ideally in CrcUtils.collectFiles alongside creation.meta/*.crc for future-proofing). A couple of smaller notes inline. Did not approve since there's a MAJOR item; CI status couldn't be confirmed from here.

[xiangfu0]

Reviewed the current head (643f9fb), focusing on what changed since the earlier reviewed commits (the new createSegmentTransferTarFile exclusion and the segment.metadata.mmap.enabled instance opt-out), plus a fresh correctness/lifecycle/compat pass over the mapped sidecar core.

The design holds up well. The sidecar validation is solid: CRC32C is checked on the heap copy before any offset is trusted, every blob offset/length is bounds-checked with long arithmetic, and matchesMapped re-verifies the mapping byte-for-byte against the validated bytes so those bounds transfer to the mmap. The mmap lifecycle is correct too — it rides the same reference-count-gated teardown as the existing index buffers, close() is idempotent, and every failure path (validation, empty segment, disabled opt-out) falls back to eager metadata. The transfer-tar exclusion correctly keeps the sidecar local while always shipping metadata.properties, so mixed-version peers rebuild fine.

No CRITICAL or MAJOR issues — only the minor notes inline, none blocking. (Posting as a comment rather than an approval since GitHub won't let me approve a PR under this account; from a review standpoint this looks good to merge.)

[xiangfu0]

Reviewed at 6b6ff51. Thorough, well-engineered change — the mmap sidecar lifecycle, backward compatibility, and failure handling are all handled carefully and backed by strong tests. Assessment: approve (posting as a comment since the review tooling is authenticated as the PR author and GitHub blocks self-approval).

What I checked and found sound:

• Lifecycle / mmap cleanup: the metadata mapping follows the same reference-counted destroy model as the existing index buffers. SegmentLocalFSDirectory.close() closes the mapping in a finally after readers retire, and ImmutableSegmentImpl.destroy() → _segmentDirectory.close() reaches it. EmptyIndexSegment.destroy() closes the metadata on the no-directory path too.
• Compatibility: metadata.properties stays canonical; the sidecar is a disposable, fingerprinted, versioned local cache. Stale / corrupt / wrong-version files rebuild rather than decode as unknown bytes, and every failure path falls back to eager metadata so the segment stays available.
• Distributed state: peer-download and deep-store-upload archives exclude the root sidecar and its in-progress temp file (scoped to the segment root, so a similarly-named nested index file is untouched), keeping the cache server-local.
• Concurrency: the TOCTOU window between validate and mapFile is re-checked against the exact mapped bytes and source fingerprints before publishing; the lazy per-column caches use correct double-checked locking; the load-time overlays are mutated only before the segment is published.

Test coverage is excellent: eager-vs-mapped round-trip equivalence (including toJson, custom map, stats, OPEN_STRUCT, legacy TimeFieldSpec), corruption/version rebuild, source-change invalidation, zero-sized index placeholder handling, mmap buffer-count leak assertions, and the transfer filter.

One forward-looking note left inline on the live-mapping delegation. No blocking issues.

[xiangfu0]

Automated review — intended decision: APPROVE (submitted as a comment because the only available credentials belong to the PR author, and GitHub blocks self-approval).

Reviewed at HEAD 1d05946a9a584018311fd92fc0433fba2c327e3d across compatibility, concurrency/lifecycle, correctness, and resource handling.

Decision: Approve. The mixed-version and sidecar-isolation design is sound and I found no CRITICAL or MAJOR defects. segment_metadata.bin is a disposable local cache: it is excluded from both live-directory tar paths in TablesResource (peer download + deep-store upload), so it never reaches deep store or another node, and any older/other reader rebuilds it from the canonical metadata.properties (or ignores the unknown file). The versioned header + CRC/fingerprint validation correctly treats a stale/old-version/corrupt sidecar as a cache miss and rebuilds it, and segment.metadata.mmap.enabled cleanly reverts to eager metadata.

On lifecycle: the mapped buffer is closed on every teardown path (ImmutableSegmentImpl.destroy → SegmentLocalFSDirectory.close → SegmentMetadataImpl.close, the empty-segment-with/without-directory paths, and the open/constructor failure paths) with no double-close or leak. Reads of mapped metadata on the query and REST paths are protected by the existing SegmentDataManager reference count, so close() (which unmaps the buffer) only runs after all readers release the segment. The lazy double-checked fields in MappedColumnMetadata are correctly published via volatile, and the multi-loader race in open() is handled (full-byte re-compare + fingerprint recheck before publishing, buffer closed on every non-success path).

A few MINOR / defensive notes are left as inline comments; none block merge.

[xiangfu0]

Reviewed the delta since the last reviewed SHA (d79866b..66339cd). The lazy per-column caching, the mapped-metadata fallback/opt-out, the OPEN_STRUCT logical-column handling in getColumnNames(), and the tar sidecar exclusion all look sound and are well covered. I built and ran the focused suites under JDK 25: TarCompressionUtilsTest, SegmentMetadataImplTest, ImmutableSegmentImplTest, IndexLoadingConfigTest, LoaderTest, MultipleTreesBuilderCloseTest, and SegmentCompressionStatsReaderTest all pass. One blocking item: TablesResourceTest.testDownloadSegments fails at HEAD. A few minor notes inline too.

[xiangfu0]

LGTM — approving in spirit (GitHub blocks approving one's own PR, so posting as a comment). This is a carefully built and thoroughly tested change: the header/CRC fingerprinting, the TOCTOU re-validation after mapFile, treating a version mismatch as a cache miss rather than decoding an unknown layout, and the resource lifecycle (fallback to eager on any failure, close-after-readers-retire) all look solid. The mapped overlays (_virtualColumnMetadata/_removedColumns) are mutated only during load before publication, so the plain HashMap/HashSet are fine. Only minor/nit notes inline — nothing that should hold up the merge.

Heads up: CI was still in progress at the reviewed SHA (linter green; unit/integration/compat suites running) — worth a final glance before merge.

[xiangfu0]

Independent re-review at head fed91a9, focused on backward compatibility, the memory-mapped read/write paths, mmap resource lifecycle, and concurrency.

Verdict: no CRITICAL or MAJOR findings — this looks ready to merge. (Posting as a comment rather than a formal approval because this account authored the PR and GitHub won't let an author approve their own PR.)

What I verified:

• Backward / mixed-version compatibility — The sidecar is a disposable local cache: it is excluded from peer-download and deep-store-upload tars (root-level segment_metadata.bin plus the .segment_metadata.*.tmp temp files), and a receiving server rebuilds it from the canonical metadata.properties. A version mismatch or fingerprint mismatch is treated as a cache miss and rebuilt, never decoded as an unknown layout. So there is no wire/on-disk format compatibility risk across mixed broker/server/controller versions.
• Correctness of the mapped read/write paths — Traced header/record/blob offset arithmetic. validate() (on a heap byte[]) establishes table == HEADER_SIZE, count >= 0, and table + count*RECORD_SIZE == blobStart <= length with long arithmetic, so every record/field read stays in-bounds; validateBlob forces each blob field to be exactly (-1,0) or a valid in-bounds blob, which matches blob()'s runtime contract, so no OOB reads, negative allocations, or integer overflow are reachable from a validating file. value()'s DECIMAL/STRING/BYTES paths can't NPE because validateValueBlob guarantees a present blob for those tags.
• mmap lifecycle — open()'s keepBuffer flag closes the mapping on every non-success exit (including the retry continue), so no buffer leaks; the post-map matchesMapped() + dual fingerprint recheck closes the validate→mapFile TOCTOU. Teardown is gated by the segment manager's reference count: destroy() → SegmentDirectory.close() → SegmentMetadataImpl.close() unmaps only after readers retire. Empty-segment and fallback paths close their metadata too.
• Concurrency — The lazy caches in MappedColumnMetadata (_bounds, _indexes, and the _partitionFunction/_partitions value+*Loaded flag pairs) use correct double-checked locking over volatile fields. The per-ordinal MappedColumnMetadata caching in SegmentMetadataImpl uses an AtomicReferenceArray with CAS. The mutable overlays (_virtualColumnMetadata, _removedColumns) are confined to single-threaded load before publication.
• Polymorphism safety — Moving isMaterializedChild()/getParentColumn()/sparse accessors onto the ColumnMetadata interface means every serving-path caller works against the interface; the only remaining ColumnMetadataImpl cast (SegmentPreProcessor) runs on eager metadata before loadMappedMetadata() swaps in the mapped view.

Ran locally on fed91a9 with JDK 25: SegmentMetadataImplTest (9), IndexLoadingConfigTest (7), ImmutableSegmentImplTest (7), EmptyIndexSegmentTest (4), TarCompressionUtilsTest (11), SegmentCompressionStatsReaderTest (7) — all pass.

One residual, informational only (already discussed on the value() thread): a hand-crafted sidecar with a recomputed CRC could embed a non-numeric DECIMAL (or unknown compression) blob that passes validate() and throws lazily at read time. Out of the stated threat model (local, integrity-checked, rebuilt cache), so not worth changing.

[xiangfu0]

Independent re-review at head fed91a9, focused on backward compatibility, the memory-mapped read/write paths, mmap resource lifecycle, and concurrency.

Verdict: no CRITICAL or MAJOR findings — this looks ready to merge. (Posting as a comment rather than a formal approval because this account authored the PR and GitHub won't let an author approve their own PR.)

What I verified:

• Backward / mixed-version compatibility — The sidecar is a disposable local cache: it is excluded from peer-download and deep-store-upload tars (root-level segment_metadata.bin plus the .segment_metadata.*.tmp temp files), and a receiving server rebuilds it from the canonical metadata.properties. A version mismatch or fingerprint mismatch is treated as a cache miss and rebuilt, never decoded as an unknown layout. So there is no wire/on-disk format compatibility risk across mixed broker/server/controller versions.
• Correctness of the mapped read/write paths — Traced header/record/blob offset arithmetic. validate() (on a heap byte[]) establishes table == HEADER_SIZE, count >= 0, and table + count*RECORD_SIZE == blobStart <= length with long arithmetic, so every record/field read stays in-bounds; validateBlob forces each blob field to be exactly (-1,0) or a valid in-bounds blob, which matches blob()'s runtime contract, so no OOB reads, negative allocations, or integer overflow are reachable from a validating file. value()'s DECIMAL/STRING/BYTES paths can't NPE because validateValueBlob guarantees a present blob for those tags.
• mmap lifecycle — open()'s keepBuffer flag closes the mapping on every non-success exit (including the retry continue), so no buffer leaks; the post-map matchesMapped() + dual fingerprint recheck closes the validate→mapFile TOCTOU. Teardown is gated by the segment manager's reference count: destroy() → SegmentDirectory.close() → SegmentMetadataImpl.close() unmaps only after readers retire. Empty-segment and fallback paths close their metadata too.
• Concurrency — The lazy caches in MappedColumnMetadata (_bounds, _indexes, and the _partitionFunction/_partitions value+*Loaded flag pairs) use correct double-checked locking over volatile fields. The per-ordinal MappedColumnMetadata caching in SegmentMetadataImpl uses an AtomicReferenceArray with CAS. The mutable overlays (_virtualColumnMetadata, _removedColumns) are confined to single-threaded load before publication.
• Polymorphism safety — Moving isMaterializedChild()/getParentColumn()/sparse accessors onto the ColumnMetadata interface means every serving-path caller works against the interface; the only remaining ColumnMetadataImpl cast (SegmentPreProcessor) runs on eager metadata before loadMappedMetadata() swaps in the mapped view.

Ran locally on fed91a9 with JDK 25: SegmentMetadataImplTest (9), IndexLoadingConfigTest (7), ImmutableSegmentImplTest (7), EmptyIndexSegmentTest (4), TarCompressionUtilsTest (11), SegmentCompressionStatsReaderTest (7) — all pass.

One residual, informational only (already discussed on the value() thread): a hand-crafted sidecar with a recomputed CRC could embed a non-numeric DECIMAL (or unknown compression) blob that passes validate() and throws lazily at read time. Out of the stated threat model (local, integrity-checked, rebuilt cache), so not worth changing.

[xiangfu0]

Took a fresh pass over the current head, focusing on the on-disk format, mixed-version read/write, mmap lifecycle, and the upsert/dedup concurrency. The compat story looks solid: the v4 version bump plus rebuild-on-mismatch means old sidecars are never misread, the sidecar is a disposable local cache excluded from peer-download and deep-store transfers, and the mapping is unmapped through the segment read-lock on destroy so readers can't touch freed buffers. Nice test coverage on the round-trip, checksum-valid-but-unparseable, legacy TIME specs, and v3->v4 rebuild paths. A couple of non-blocking notes inline; nothing I'd call a blocker. (Note: I'm posting as the PR author's token, so I can't formally approve my own PR here.)

[xiangfu0]

Reviewed the current HEAD fresh, focusing on on-disk format/serialization safety, mixed-version/compat, concurrency, and the use-after-close lifecycle of the mmap'd metadata. (Posting as a comment review since GitHub won't let the token approve its own PR — but this is an approve-equivalent: no blocking findings.)

The design holds up well. The sidecar is a disposable, server-local cache (correctly excluded from the peer-download/deep-store tar in createSegmentTransferTarFile, and a version or fingerprint mismatch just triggers a local rebuild rather than a cross-version decode), so there's no wire/format compat exposure. On the format itself, every blob reference reachable at serve time is bounds-validated before any mmap copyTo/getInt runs, the full payload is covered by a CRC32C (with the CRC field zeroed), and matchesMapped() + the re-checked source fingerprints close the TOCTOU window between validate and mapFile. The new ColumnMetadata default methods and @Nullable getColumnMetadataFor keep existing implementers binary-compatible. I ran SegmentMetadataImplTest, ImmutableSegmentImplTest, LoaderTest, BasePartitionUpsertMetadataManagerTest, and ConcurrentMapPartitionDedupMetadataManagerWithTTLTest on JDK 25 — all green.

No blocking issues. Two small non-blocking notes inline. Nice work, and thanks for the thorough PR writeup and benchmark context.

[xiangfu0]

Fresh pass at 6004060, focused on eager-vs-mapped fidelity and on what the sidecar's cache key covers. One correctness issue: BIG_DECIMAL default null values change through the FieldSpec JSON round trip (reproduced with jshell). Plus three smaller points, all inline.

[xiangfu0]

Summary

Reviewed the current head (221d8cb) of "Store sealed segment metadata in a mapped sidecar" — the full diff across pinot-segment-spi, pinot-segment-local, pinot-common, and pinot-server (37 files, +2331/-117).

Overall this is a careful, well-tested change and I didn't find any correctness or compatibility blockers. A few things I specifically checked and liked:

• Mixed-version compatibility: the sidecar is a purely local, versioned artifact. metadata.properties stays canonical, old versions are rebuilt rather than migrated (VERSION bump invalidates), and createSegmentTransferTarFile excludes the sidecar from server-to-server tarballs so a receiving node always rebuilds from canonical metadata. No sidecar ever crosses a node boundary.
• Resource/mmap lifecycle: the mapped buffer is closed on destroy()/directory close, and reads that could race destruction (the upsert TTL watermark path) are now guarded by the segment read lock; getWatermarkToPersist forces the reload path when it can't acquire. getDataCrc/getCrc used in recordSnapshotPass read heap fields, so they stay safe after close.
• Format internals: header offsets, fixed 180-byte records, blob refs, and the CRC-over-everything-but-its-own-slot checksum are all internally consistent between build/validate/open, and open re-verifies the mapped bytes against the heap copy to handle an atomic replace mid-open.
• Concurrency: MappedColumnMetadata lazy caches use volatile + synchronized double-checked init; the per-column AtomicReferenceArray CAS is correct. Overlays are load-time only.
• Feature is opt-in and off by default (segment.metadata.mmap.enabled, instance-level), and the tests cover round-trip fidelity, corrupt/stale rebuild, index-service version changes, decimal precision, and buffer-leak counts.

Only one minor documentation nit (inline). From my side this is good to merge once that's addressed; flagging as a comment rather than an approval because I'm the PR author and GitHub won't let me self-approve.

Finding counts: CRITICAL 0, MAJOR 0, MINOR 1 (doc nit).

[Jackie-Jiang]

Re-reviewed ce7d6c4, including all six files changed since fcab777 and their serving/reload callers. One new P2 correctness issue remains in the detached metadata fix; details are inline.

Verified fixes: TIMESTAMP defaults preserve epoch values, including nested fields and timezone/DST coverage; column names are cached rather than rebuilt per partial upsert; the protected upsert Set field descriptor is restored; index-size caches use packed primitive arrays; the sidecar writer reuses parsed index entries; and the redundant boolean constructors are removed.

Remaining performance evidence is still incomplete: concurrent short-query behavior under the per-segment scalar-read monitor, complete cold/reused serving startup, and retained heap after warming column-name/bounds/index-size caches. These are unmeasured risks, not demonstrated latency regressions. The PR now documents them and keeps serving opt-in.

The broader SegmentMetadataImpl design concern remains: it combines eager, mapped, and consuming representations with mode-specific accessors and overlays. The check/rebuild/open flow already exists in MappedSegmentMetadataStore; isolating the backing strategy would make that flow and its ownership contracts easier to maintain. This does not make all integration changes out of scope. Version 6 is explained by invalidating development caches, including v5's timestamp encoding; it is not itself a compatibility defect.

Validation: all 13 distinct hosted checks are green for this head. This rereview inspected the changed tests and traced the reload interleaving; no repository tests/builds were run and no source changes were made.

[xiangfu0]

Risk: HIGH — persisted/serialized on-disk metadata format (segment_metadata.bin) plus memory-mapped buffer ownership/lifecycle and concurrency on segment metadata reads. Each is independently a high-risk characteristic, so this needs a human owner's sign-off regardless of finding severity.

Reviewed SHA: 578a04a — full merge diff vs merge-base 2877723 (39 files, +2677/−125). The branch was squashed to a single commit since my last pass (fcab777); I re-reviewed the complete current diff, not just the delta. Net-new surface since fcab777 is 7 files, all of which strengthen the design.

What changed since the last head, and it's a solid direction:

• Detached metadata snapshots (the main hardening). HelixInstanceDataManager.getSegmentMetadata()/getAllSegmentsMetadata() now return SegmentMetadataImpl.detachedCopy() for mapped segments — an eager, heap-backed copy built via ColumnMetadataImpl.detachedCopyOf() while the segment is still referenced, before releaseSegment(). The returned copy holds no reference to the mmap store, so callers that keep it past the ref-count window are safe. This closes the "caller caches metadata beyond the reference window" fragility I flagged earlier. Covered by testDetachedMappedMetadataPreservesServingColumnsAfterClose (verifies virtual/removed columns, index sizes, ZK timestamps survive, and isMapped()==false).
• API-compat fix for subclasses. The protected _updatedSegmentsSinceLastSnapshot field is restored to its original Set<ImmutableSegmentImpl> type (the prior head had changed it to a ConcurrentMap, which would have broken extensions). It's now a keySet(TTL_WATERMARK_NOT_SET) view over a private value-carrying map, so .add() inserts the correct marker and the watermark semantics are preserved.
• Index sizes sourced from the eager ColumnMetadataImpl (getNumIndexes/getIndexType/getIndexSize) instead of re-parsing index_map. I verified this produces identical on-disk content: the eager path only populates index sizes for v3 + totalDocs>0, and uses EmptyColumnMetadata (0 indexes) otherwise — matching the old readIndexSizes special-case exactly. On-disk blob is still string-keyed (type id + long); the numeric id is resolved at read time, so nothing version-fragile is persisted.
• VERSION reset 5→1 for the first merged format — safe because the sidecar is a disposable local cache that rebuilds on any version mismatch (feature is opt-in and unreleased).
• TIMESTAMP defaultNullValue now encoded as text alongside numerics, preventing a precision/format drift for timestamp defaults (covered by testMappedTimestampDefaultNullValuesPreserveInstantAcrossTimeZones).

Validation: All 14 hosted checks are green at this head — Unit Test Set 1/2, Integration Test Set 1/2, Compatibility Regression (master + release-1.5.0), MSQE compatibility, Quickstart, Linter, Binary-compat. (At my prior head these were still in progress; they have now completed successfully.)

Decision: no agent approval. I found no CRITICAL or MAJOR correctness, compatibility, or concurrency defect at this SHA, and CI is complete and green. But this is a HIGH-risk change (on-disk format + mmap lifecycle + concurrency), so per policy it requires a human maintainer's review rather than an automated approval — and the reviewing token is the PR author's account, which cannot self-approve in any case. Requesting a human owner review before merge. Minor, non-blocking note inline.

[KKcorps]

This closes the last order the model found, where a segment destroyed before the watermark cap read its max made the round persist no watermark. Nothing new turned up.

I replayed the model's 52 known bad orders and none changed. An hour of search over 8 table shapes found nothing that breaks only with this change, and neither did a second search started next to a marked segment that gets destroyed.

Each segment's comparison max is now cached before any thread can mark the segment, and markSegmentUpdated publishes it with the marker. So outside tests getWatermarkToPersist never meets an unset marker, and a destroyed segment caps with the same max today's code reads from its metadata. That also closes a Helix thread marking a segment already removed out of TTL, which the model cannot build.

The model covers one replica and one partition, and not the mapped metadata sidecar itself.

[xiangfu0]

Focused deep-dive: design + preload behavior (follow-up to my review at this same SHA, 578a04a; this pass traced the upsert preload flow end to end against the pre-PR base).

What I verified:

• Preload invariant holds on every path. doPreloadSegment now reads and caches the comparison-column max before the empty-snapshot and out-of-TTL early returns, and preloadSegment tracks the segment afterwards — so by the time trackSegmentForSnapshot/markSegmentUpdated runs, the weak cache is always populated. Preload uses the same serving IndexLoadingConfig, so preloaded segments carry mapped metadata and their maxima are cached while the mapping is open.
• The watermark-capping linchpin is intact. Segments whose snapshot persist fails (lock contention, replaced mid-pass, exception) stay in the updated map and keep capping the persisted watermark via their cached maxima — readable even after the segment is destroyed and its mapping closed. retainAll only drops untracked segments, whose stale snapshots are gone or superseded by a tracked replacement that re-caps. The stale-snapshot-skip-on-restart protection survives the rework.
• Skip-path semantics preserved: an out-of-TTL preloaded segment uses its snapshot as-is, gets tracked, caps the watermark until its next fresh snapshot round — same as base, now with the destroyed-segment read hazard removed.
• Dedup doesn't need the same treatment: BasePartitionDedupMetadataManager only reads segment metadata in lifecycle-ordered add/remove ops while the segment is alive; it has no background pass over tracked segments, so the upsert-only caching is the right scope.
• Serving-path cost model is as designed: preprocessing still requires the eager parse, so the win is retained heap, not load time, on the serving path — consistent with the PR's stated motivation. Two minor notes inline, one of them an actionable first-boot cost saving.

No new CRITICAL/MAJOR findings. Risk classification unchanged (HIGH: persisted format + mmap lifecycle + concurrency) — human maintainer review still required; no agent approval (reviewing token is also the author account).

[Jackie-Jiang]

Re-reviewed 578a04a. No new correctness or compatibility defect found in this iteration.

The previous detached-copy race is fixed: the implementation copies the acquired segment's in-memory fields and materializes its mapped columns without reopening the canonical directory. I checked the segment and column fields, including bounds, partitioning, OPEN_STRUCT metadata, compression/index sizes, CRCs, timestamps, and serving overlays. The regression test now moves the directory before detachment and reads the detached metadata after closing its owner.

The initial format is now version 1, with an unsupported-version rebuild test. The earlier timestamp encoding, column-name allocation, protected Set descriptor, compact index-size cache, and parser-reuse fixes remain intact. One minor exception-signature cleanup is inline.

The existing design and performance threads remain open. SegmentMetadataImpl still combines eager, mapped, and consuming representations; separating its backing strategy would simplify ownership and mode-specific accessors. Most integration changes are related to the heap/lifetime goal, so I do not classify them as unrelated scope. Concurrent short-query contention, complete cold/reused serving startup, and heap retained after warming the lazy caches remain unmeasured; the PR explicitly documents these limits and keeps serving opt-in. These are outstanding validation/design concerns, not newly demonstrated regressions.

Validation: compared all four files changed since ce7d6c4 and rechecked relevant format/serving/lifecycle paths. All 39 PR file snapshots were verified against their diff blob hashes, and all 13 distinct hosted checks are green for this head. No local builds/tests or repository source changes were made during this rereview.

[xiangfu0]

Re-review at new head 249e39d (one commit on top of my previously reviewed 578a04a: "Remove obsolete exceptions from detached metadata copy").

Delta: 2 files, +2/−9 — pure dead-code removal. detachedCopy() never throws the declared IOException/ConfigurationException (its body is field copies plus unchecked-wrapping getters), so dropping the throws clause and the now-unreachable catch in HelixInstanceDataManager is correct; javac itself forces the catch removal once the clause is gone. Only two callers exist (both in this PR), so no API impact. No behavior change.

Validation: all 13 checks are pending at this head (just pushed). The delta is signature-only on top of a head that was fully green (14/14), so no new runtime risk is introduced — but the gate still needs these runs to complete.

Risk: HIGH (unchanged — persisted on-disk format + mmap lifecycle + concurrency across the full PR). No CRITICAL/MAJOR findings at this head. Human maintainer review still required; no agent approval (reviewing token is also the author account). My two earlier non-blocking inline notes (preload-time max read visibility, double parse in build()) remain open as optional follow-ups.

[Jackie-Jiang]

Consolidated review, updated for 249e39d.

The previously reported correctness and compatibility issues are addressed. I found no remaining confirmed correctness blocker in the reviewed implementation.

Verified fixes:

• TIMESTAMP defaults preserve their epoch values, including nested fields and DST/timezone cases.
• Column names are cached instead of rebuilding a tree for every partial upsert.
• The protected upsert field retains its original Set binary descriptor.
• Metadata-returning APIs detach their results before releasing the segment; detachment now copies the acquired state instead of reopening a directory that reload can rename or replace.
• Index-size caches use compact primitive arrays, and sidecar generation reuses parsed index entries.
• The initial format starts at version 1, and redundant boolean constructors are removed.
• The latest commit removes the obsolete checked exceptions from detachedCopy() and the unnecessary caller catch block.

Remaining concerns:

1. Load-time efficiency. Rebuilding constructs another eager metadata instance and separately reparses the properties even though the serving loader already has parsed metadata. Please reuse that input. A valid-cache open still fingerprints canonical inputs, reads and validates the complete binary into a heap array, compares the opened buffer, and fingerprints inputs again. Normally there are zero or one rebuilds, but retries permit three per open, and independent concurrent callers can duplicate generation. Consider coordinating generation for the same directory. Preserve source-change and concurrent-replacement protection when simplifying validation. These are concrete additional operations; their end-to-end cost remains unmeasured.

2. Design clarity. The check/rebuild/open lifecycle is reasonable and already centralized in MappedSegmentMetadataStore. SegmentMetadataImpl still combines eager, mapped, and consuming representations with mode-specific getters, overlays, and ownership rules. Separating mapped backing from legacy parsing would simplify maintenance. The integration changes serve the heap/lifetime goal, so I would not classify all of them as unrelated scope.

3. Performance evidence. Keep serving opt-in until representative cold/reused serving startup, concurrent short-query performance under the per-segment scalar-read monitor, and retained heap after warming column-name, bounds, and index-size caches have been measured. Existing retained-heap probes do not establish these serving costs. These remain validation gaps, not demonstrated latency regressions.

4. Minor file hygiene. The writer cleans its own temporary file in finally, but a hard crash can leave .segment_metadata.*.tmp files indefinitely, including through copy-based reloads. Please add safe abandoned-temp cleanup without deleting another active writer's file.

Validation: the substantive review covered 578a04a; I additionally verified the two-file exception-cleanup delta to 249e39d. At publication, 12 hosted checks have passed and Unit Test Set 2 is queued for the latest head. All 13 distinct checks passed on the preceding head. No local builds/tests or repository source changes were made during this review.

[xiangfu0]

Thanks for the consolidated review. I pushed 92e17aa: sidecar generation now uses one parsed PropertiesConfiguration for the eager metadata and encoding, and copy-based reload skips root .segment_metadata.*.tmp files while retaining the valid sidecar. The focused tests passed (15 SegmentMetadataImplTest and 5 SegmentLocalFSDirectoryTest cases), along with affected Spotless, Checkstyle, license, and test compilation checks.

This removes one rebuild parse, but does not yet reuse the loader's existing eager object: that object has no parse-time source fingerprint, so it could be stale relative to the fingerprint taken during rebuild. Safe cleanup of orphan temp files in the live source directory still needs writer ownership or locking; deleting by name or age could remove another active writer's file. I am leaving those points, the representation discussion, and the serving-performance measurements open. Hosted CI has restarted on the new head.

[xiangfu0]

Re-review at new head 92e17aa (one commit on top of 249e39d: "Reduce mapped metadata rebuild parsing and skip temp copies"; 4 files, +44/−7).

Delta review — both changes verified correct:

• Rebuild parse reduction: build() now parses metadata.properties once and threads the config into a package-private eager constructor, halving the rebuild parse cost flagged in my earlier note. I verified the eager init path is strictly read-only over the shared config, so aliasing with encodeSegmentConfiguration/encodePartitionFunction is safe — and a single parse actually tightens correctness (one consistent view of the file for both column data and the encoded segment config, matching the fingerprint taken in open()).
• Temp-file copy exclusion: the reload directory copy now filters abandoned .segment_metadata.*.tmp files (root-level only; directories and nested paths pass through, so commons-io filter semantics are safe) while deliberately keeping the valid segment_metadata.bin so an unchanged segment reuses its cache after reload. This also means crash-orphaned temp files stop propagating through backup/restore cycles and get cleaned up naturally. Consistent with the tar-transfer path, which strips both (cache stays local). Covered by the new testCopyExcludesMappedMetadataTemporaryFile.

Validation: all 13 checks pending at this head (just pushed). The delta is small and behavior-preserving on the serving path; prior head lineage was fully green, but the gate still needs these runs to complete.

Risk: HIGH (unchanged for the full PR — persisted on-disk format + mmap lifecycle + concurrency). No CRITICAL/MAJOR findings at this head. Human maintainer review still required; no agent approval (reviewing token is also the author account).

[xiangfu0]

Re-review at new head 4a3d0cb — a merge of upstream master into the branch ("Merge master to include broker test fixture fix"). For a merge commit the review question changes, so I verified accordingly:

• The merge is provably clean: the merge commit's tree is byte-identical to git's automatic merge of its two parents (git merge-tree --write-tree 92e17aae c91d0e83 = 4a3d0cbb^{tree}). No manual conflict resolution, no hand edits beyond the two already-reviewed inputs.
• PR content vs the new base is unchanged from what I reviewed at 92e17aa, with two benign deltas: DimensionTableDataBytesPrimaryKeyTest and DimensionTableDataManagerTest dropped out of the diff because the PR's temp-dir hygiene fix landed upstream independently (PR head and new base are byte-identical there), and the PR diff is now 38 files.
• No production-file intersection with master drift. Only two test files were touched on both sides (TarCompressionUtilsTest, LoaderTest); the interleave is clean and the PR-side additions (testFilteredDirectory, the sidecar-aware v3 directory assertions) are intact at the merge head.

Validation: 2/13 checks green, 11 pending at this head (merge just pushed). The merge brings ~378 files of upstream change, so the full suite needs to complete on the merged tree before this head counts as validated.

Risk: HIGH (unchanged for the full PR — persisted on-disk format + mmap lifecycle + concurrency). No new findings at this head. Human maintainer review still required; no agent approval (reviewing token is also the author account).

[Jackie-Jiang]

Re-reviewed 4a3d0cb. No new confirmed correctness or compatibility findings.

The latest changes address part of the remaining efficiency and file-hygiene concerns:

• Sidecar generation now parses metadata.properties once and passes the same PropertiesConfiguration to the eager metadata constructor and binary encoder. The constructor and column/star-tree/text metadata parsers do not mutate or retain that configuration, and source-fingerprint checks remain intact. This removes the duplicate parse inside build().
• Reload copies skip top-level .segment_metadata.*.tmp files while retaining segment_metadata.bin, including when copying from the backup directory. The new focused test covers an abandoned temporary file.
• The earlier detached-copy, TIMESTAMP, protected Set descriptor, cached column names, compact index sizes, and exception cleanup remain intact.

Two limits to those improvements: build() still constructs another eager metadata object instead of reusing the serving loader's existing metadata; and excluding temporary files from copies does not sweep existing crash leftovers from the source directory. Those leftovers remain until directory cleanup.

The existing broader concerns remain open: representative full serving startup, concurrent short-query behavior under the per-segment scalar-read monitor, and retained heap after warming caches are still unmeasured. SegmentMetadataImpl still combines eager, mapped, and consuming representations. The new package-private constructor is a narrow, useful reuse improvement, but it does not address that larger design concern. Keep serving opt-in while the documented performance validation remains outstanding.

Validation: reviewed the latest feature changes and their interaction with the merged master changes, verified all 38 PR file snapshots against their diff blob hashes, and checked current CI. All 13 hosted checks passed for this head. No local builds/tests or repository source edits were performed.

[Jackie-Jiang]

Remaining items on 4a3d0cb:

1. Reuse the loader's already parsed metadata. The duplicate properties parse inside build() is fixed, but generation still constructs another eager SegmentMetadataImpl after the serving loader has parsed the same metadata. Reuse the post-preprocessing metadata/configuration where its identity can be verified against the canonical source, preserving the existing stale-source checks.

2. Complete serving-performance validation. Measure cold and reused-sidecar startup/readiness, peak heap/GC/RSS under realistic load concurrency, concurrent short-query behavior under the per-segment scalar-read monitor, and retained heap after warming column names, bounds, and index-size caches. Keep the feature opt-in until these results are available; current retained-heap probes do not establish these serving costs.

3. Simplify SegmentMetadataImpl's representation and ownership model. Separate mapped backing from eager/consuming metadata to reduce mode-specific accessors, nullable backing fields, and overlay/lifetime logic. The check/rebuild/open flow is already centralized; the remaining concern is the surrounding class design.

4. Safely clean abandoned sidecar temporary files. Reload copies now exclude .segment_metadata.*.tmp files, but crash leftovers remain in the source directory until directory cleanup. Add cleanup at a lifecycle point where another active writer's file cannot be deleted.

These are the remaining efficiency, validation, maintainability, and hygiene items. No confirmed correctness or compatibility blocker remains in the reviewed head.

[xiangfu0]

Re-review at new head 47e0240 (one commit on top of 4a3d0cb: "Reuse verified eager metadata for mapped sidecar rebuilds"; 8 files, +221/−19).

This completes the rebuild-parse optimization thread from my earlier reviews, and it takes exactly the path I had cautioned needed fingerprint coupling — and does the coupling right. I reviewed it as the highest-risk change of the recent follow-ups, since an unverified metadata reuse could silently publish a sidecar whose content doesn't match its recorded fingerprints.

Verified:

• Staleness hazard closed by construction. captureEagerSnapshot reads the canonical bytes once, fingerprints those bytes, and parses the eager view from those same bytes — so the parsed object provably corresponds to the recorded fingerprints. build() only reuses the snapshot when its fingerprints equal the ones open() just computed from the live files AND the IndexService instance is unchanged (registry drift is also re-checked at capture time via _reusable). Anything changed on disk → fresh parse, exactly as before.
• Parse parity. fromInputStream(bytes) and the incumbent fromFile(file) configure identically (default delimiter, DefaultIOFactory, same FileHandler load), so a snapshot-parsed view is equivalent to a file-parsed view of the same bytes. The versioned-header variant (getSegmentMetadataFromFile) is not on either path.
• Lifecycle: the snapshot is dropped on reloadMetadata() (correct — preprocessing rewrote the file), consumed and nulled at loadMappedMetadata(), and nulled at close(). Its metadata doubles as the directory's _segmentMetadata, so no duplicate eager object is retained. The withMappedMetadataEnabled() flag is set only on the final serving load after preprocessing; the needPreprocess probe uses the unflagged context; externally-created directories simply skip the optimization.
• Tests are well-constructed: the stale-snapshot tests mutate content with same-length replacements so only the CRC differs — precisely exercising the fingerprint check — for both metadata.properties and index_map, plus a reload-drops-snapshot test.

Net effect: a sidecar rebuild now parses metadata.properties exactly once per segment (down from three parses two commits ago), with correctness preserved.

Validation: 2/13 checks green, 11 pending at this head. Risk: HIGH (unchanged for the full PR). No CRITICAL/MAJOR findings. Human maintainer review still required; no agent approval (reviewing token is also the author account).

[Jackie-Jiang]

Reviewed 47e0240 for format correctness, compatibility, lifecycle, performance, and maintainability. No confirmed correctness or compatibility blocker remains. The guarded eager snapshot and its cleanup look sound, and the new ColumnMetadata methods have compatible defaults. Approving with one nonblocking loader-path follow-up below.

The reported full-segment synthetic measurements improve the evidence: about 13% lower warmed retained heap, with roughly 5–7% slower cold loads and higher sampled peak heap/RSS at four load workers. Representative server startup, production query behavior, and Linux mapping headroom remain rollout gates while the feature stays opt-in. Separating eager/mapped representations and safely cleaning crash-orphaned temporary files can follow separately.

All current-head CI checks pass. This re-review was based on code inspection and the reported measurements; I did not run local tests.

[xiangfu0]

Re-review at new head fdafe1c (one commit on top of 47e0240: "Reuse eager metadata for existing server segments"; 3 files, +58/−2).

Delta: extends the snapshot-reuse flag to BaseTableDataManager.initSegmentDirectory, covering the existing-segment startup and reload paths that previously skipped the optimization. I reviewed this against the staleness question from the last round, and the reasoning holds:

• The flag is a performance hint, not a correctness decision. Since the reuse guard lives in build() (snapshot fingerprints + IndexService identity must match open()'s fresh reads of the live files), no placement of withMappedMetadataEnabled() can cause a stale snapshot to be reused — the worst case anywhere is a wasted capture. That's what makes extending it to more call sites safe, and the loosened javadoc ("may become the mapped serving view") correctly reflects the guarded contract.
• Preprocessing on these paths stays correct: when a reload/preprocess modifies metadata.properties, reloadMetadata() drops the snapshot first thing; even a hypothetical path that modified the file without calling it would be caught by the fingerprint mismatch.
• Plugin loaders are unaffected: a third-party SegmentDirectoryLoader that ignores the new context flag just loses the optimization, not correctness.
• The test is the right proof: it drives the real table-manager reload flow (local-copy reuse, createBackup never invoked), modifies metadata.properties before reload, asserts the loader observed the flag, and verifies the reloaded mapped view reflects the fresh content — the exact leak a stale snapshot would cause.

Validation: 2/13 checks green, 11 pending at this head (just pushed). Risk: HIGH (unchanged for the full PR — persisted on-disk format + mmap lifecycle + concurrency). No CRITICAL/MAJOR findings. Human maintainer review still required; no agent approval (reviewing token is also the author account).

[xiangfu0]

Documentation for this change has been added: pinot-contrib/pinot-docs#1159

It documents the new server instance config pinot.server.instance.segment.metadata.mmap.enabled (disabled by default) in the Server configuration reference.