Skip to content

Allocate ColumnMetadataImpl index sizes lazily and skip the index_map lookup without an index dir - #19480

Merged
xiangfu0 merged 2 commits into
masterfrom
xiangfu0/data-3221-1-lazy-index-sizes
Sep 11, 2026
Merged

xiangfu0 merged 2 commits into
masterfrom
xiangfu0/data-3221-1-lazy-index-sizes

Conversation

@xiangfu0

@xiangfu0 xiangfu0 commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

PR flow

Lazy allocation of index sizes and skipping index_map lookup for stream-loaded metadata.

flowchart TD
  N0["#95;indexTypeSizes field starts null #40;F1#41;"]:::stAdded
  N1["addIndexSize allocates LongArrayList if null then adds #40;F1#41;"]:::stModified
  N2["getIndexSizeFor returns UNAVAILABLE when list null #40;F1#41;"]:::stModified
  N3["getIndexSizeMap returns empty map when list null #40;F1#41;"]:::stModified
  N4["getNumIndexes returns 0 when list null #40;F1#41;"]:::stModified
  N5["getIndexType#47;getIndexSize bounds#45;check then read from list #40;F1#41;"]:::stModified
  N6["SegmentMetadataImpl#46;init checks #95;indexDir for v3 index#95;map #40;F2#41;"]:::stModified
  N7["Stream#45;loaded metadata #40;no indexDir#41; skips index#95;map #40;F2#44; F3#41;"]:::stModified
  N8["Directory#45;loaded metadata loads index#95;map #8594; addIndexSize #40;F2#44; F3#41;"]:::stModified
  N6 -->|"#95;indexDir #61;#61; null #8594; skip index#95;map"| N7
  N6 -->|"#95;indexDir #33;#61; null #8594; load index#95;map"| N8
  N7 -->|"getIndexSizeFor sees null list"| N2
  N7 -->|"getIndexSizeMap sees null list"| N3
  N7 -->|"getNumIndexes sees null list"| N4
  N7 -->|"getIndexType#47;getIndexSize bounds#45;check on empty list"| N5
  N8 -->|"loading invokes addIndexSize"| N1
  N1 -->|"getIndexSizeFor reads from list"| N2
  N1 -->|"getIndexSizeMap builds from list"| N3
  N1 -->|"getNumIndexes returns list size"| N4
  N1 -->|"getIndexType#47;getIndexSize accesses list"| N5
  classDef stAdded fill:#dafbe1,stroke:#1a7f37,color:#1f2328,stroke-width:2px
  classDef stModified fill:#fff8c5,stroke:#9a6700,color:#1f2328,stroke-width:2px
  classDef stRemoved fill:#ffebe9,stroke:#cf222e,color:#1f2328,stroke-width:2px
  classDef stUnchanged fill:#f6f8fa,stroke:#656d76,color:#1f2328,stroke-width:1px
Loading

AI-generated · Green: added · Yellow: modified · Red: removed · Gray: existing

Diff evidence
  • F1: pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImpl.java — before · after
  • F2: pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/SegmentMetadataImpl.java — before · after
  • F3: pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/SegmentMetadataImplTest.java — before · after
  • Regenerate PR flow

Summary

Allocate index-size storage only on the first valid addIndexSize call. Columns without recorded sizes retain no list or backing array; populated columns use LongArrayList(2) for amortized appends instead of copying the entire array on every append. Positional reads preserve bounds errors, and lookups use primitive indexed access.

Skip the local v3/index_map lookup when metadata is loaded from streams without an index directory. Empty indexSizeMap values still serialize as {}; index-size packing and public signatures are unchanged.

The absent-size case avoids the original list plus two-element array (approximately 56 bytes per column with conventional HotSpot compressed references). Populated columns retain the original list-based storage overhead. This is an object-layout estimate for this PR, not an attribution of the full DATA-3221 stack's heap reduction.

Validation

  • 31 tests passed across ColumnMetadataImplTest, ColumnMetadataTest, and SegmentMetadataImplTest.

  • Focused metadata tests cover empty and populated states, growth/order, 48-bit sizes and signed index IDs, bounds errors, equality/hash/string behavior, and rejected appends preserving existing entries.

  • Segment-loading coverage compares a local v3 directory with the stream constructor, including index-size lookup and JSON compatibility.

  • Spotless, Checkstyle, license formatting, and license checks passed for pinot-segment-spi and pinot-segment-local.

  • The additional strict compiler-warning run passed the SPI tests with no warnings on changed lines, then stopped at the existing missing JetBrains NotNull annotation dependency in unchanged ZstandardDecompressor.java:51.

This PR targets master and is the first metadata-compaction layer in DATA-3221; #19473 remains based on this branch.

@codecov-commenter

codecov-commenter commented Sep 6, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 55.00000% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.70%. Comparing base (c611609) to head (22186c0).
⚠️ Report is 9 commits behind head on master.

Files with missing lines Patch % Lines
...segment/spi/index/metadata/ColumnMetadataImpl.java 57.89% 7 Missing and 1 partial ⚠️
...egment/spi/index/metadata/SegmentMetadataImpl.java 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #19480      +/-   ##
============================================
- Coverage     67.74%   67.70%   -0.04%     
- Complexity     1424     1450      +26     
============================================
  Files          3489     3490       +1     
  Lines        224672   225001     +329     
  Branches      35468    35521      +53     
============================================
+ Hits         152210   152343     +133     
- Misses        60445    60628     +183     
- Partials      12017    12030      +13     
Flag Coverage Δ
integration 100.00% <ø> (+100.00%) ⬆️
integration1 100.00% <ø> (?)
integration2 0.00% <ø> (ø)
java-25 67.70% <55.00%> (-0.04%) ⬇️
lane-a 100.00% <ø> (+100.00%) ⬆️
lane-b 0.00% <ø> (ø)
temurin 67.70% <55.00%> (-0.04%) ⬇️
unittests 67.70% <55.00%> (-0.05%) ⬇️
unittests1 57.80% <55.00%> (+<0.01%) ⬆️
unittests2 39.45% <0.00%> (-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
xiangfu0 requested review from Jackie-Jiang and a lite review from Copilot September 6, 2026 07:14
@xiangfu0 xiangfu0 added performance Related to performance optimization memory Related to memory usage or optimization labels Sep 6, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The changes are well-scoped, preserve external behavior/serialization, and are covered by new focused tests (with only a minor non-blocking load-time allocation consideration noted).

Pull request overview

This PR reduces per-column retained heap in segment metadata by making index-size storage lazy in ColumnMetadataImpl, and avoids unnecessary filesystem probing when SegmentMetadataImpl is constructed without an index directory (e.g., stream/tiered-storage paths). It keeps the REST/JSON indexSizeMap shape stable ({} when empty) and adds targeted unit tests for the new behavior.

Changes:

  • Replace eager LongArrayList index-size storage with a lazily-initialized packed long[] in ColumnMetadataImpl.
  • Guard v3 index_map loading in SegmentMetadataImpl to skip the lookup when _indexDir is null.
  • Add/extend unit tests covering empty/default index sizes, packing round-trip, invalid-size rejection, and stream-vs-dir loading behavior.
File summaries
File Description
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImpl.java Lazily stores packed index sizes in a long[] and updates equality/hash/JSON serialization accordingly.
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/SegmentMetadataImpl.java Skips v3/index_map probe when there is no index directory (stream constructor path).
pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImplTest.java Adds tests for default-empty sizes, packing round-trip, value-object participation, and invalid-size rejection.
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/SegmentMetadataImplTest.java Adds regression test ensuring index sizes are only present when loading via an index directory (v3 index_map).
Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

… lookup without an index dir

Every ColumnMetadataImpl eagerly allocated a LongArrayList plus its long[2] for index sizes, although sizes are only ever populated by SegmentMetadataImpl from a local v3 index_map. Metadata loaded from streams (tiered storage), v1/v2 segments and the built-in virtual columns never have any, yet paid ~56 bytes per column for the segment's lifetime; on a wide external-table segment (1000+ columns, tens of thousands of segments per server) that is a measurable share of the per-column heap. The sizes now live in a long[] that stays null until the first addIndexSize. SegmentMetadataImpl skips the v3/index_map probe when it has no index directory: the stream constructor used to stat a cwd-relative v3/index_map once per segment and would have thrown on _indexDir.getPath() had that file existed. No public signature, on-disk or REST JSON change: indexSizeMap still serializes as {} when empty.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@xiangfu0
xiangfu0 force-pushed the xiangfu0/data-3221-1-lazy-index-sizes branch from 89ea2d2 to fb925d7 Compare September 9, 2026 01:56
@xiangfu0
xiangfu0 merged commit b65be1d into master Sep 11, 2026
14 checks passed
@xiangfu0
xiangfu0 deleted the xiangfu0/data-3221-1-lazy-index-sizes branch September 11, 2026 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

memory Related to memory usage or optimization performance Related to performance optimization

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants