Hold segment column metadata in sorted arrays and derive the map on demand - #19481
Open
xiangfu0 wants to merge 2 commits into
Open
Hold segment column metadata in sorted arrays and derive the map on demand#19481xiangfu0 wants to merge 2 commits into
xiangfu0 wants to merge 2 commits into
Conversation
This was referenced Sep 6, 2026
Open
Open
xiangfu0
added
performance
… the map on demand A server retains one SegmentMetadataImpl per loaded segment for the segment's lifetime, and it held its columns in a TreeMap. On a 1000-column external-table segment that is a red-black-tree node per column: 40.0 B/column measured, pure bookkeeping on top of the ColumnMetadata the node points at. Hold the columns as two parallel arrays instead -- the names in natural order and their metadata at the same index -- which costs 8.1 B/column, so ~32 B/column less. Lookups binary-search the name array and getAllColumns() is an unmodifiable view of it (SortedStringArraySet, a NavigableSet over a sorted String[] range). getColumnMetadataMap() keeps working and keeps returning a TreeMap, but it is now derived from the arrays on the first call and cached until the columns change, exactly as the per-segment Schema already was. Nothing on the load or query path asks for it any more: SegmentMetadata gains getNumColumns(), getAllColumnMetadata(), forEachColumn(BiConsumer) and addColumnMetadata(String, ColumnMetadata) as additive default methods, and the OSS callers are migrated onto those plus the existing getAllColumns()/getColumnMetadataFor(). LoaderTest asserts the map view is still unbuilt after ImmutableSegmentLoader.load, next to the same assertion for the schema, so a stray caller is caught in CI. Two behaviour notes: - getAllColumns() is now an unmodifiable snapshot rather than the TreeMap's live navigableKeySet. TablesResource#getSegmentMetadata was retaining that live view as its running column intersection and calling retainAll on it, i.e. removing columns from the first segment's own metadata; it now intersects a copy. - getColumnMetadataMap() still returns null for a CONSUMING segment, and writes to the returned map no longer reach the metadata (use addColumnMetadata/removeColumn). Mock-based tests that stubbed only getColumnMetadataMap() now stub the accessors the production code reads. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review follow-up on the sorted-array column metadata store.
- The names and the metadata were two independent volatile fields replaced one
after the other, so a reader could see the names of one version beside the
metadata of another. Both now live in one immutable Columns holder behind a
single volatile field, so every publication is atomic, and init() fills the
arrays before publishing them instead of publishing them empty and filling
them afterwards (the index_map loop looks its column up in the local array).
- addColumnMetadata() replaced the metadata of an existing column in place,
which a Collection returned earlier by getAllColumnMetadata() -- documented as
a snapshot -- observed. It is copy-on-write now, like the insert branch.
- removeColumn() on a CONSUMING segment fell through to invalidateDerivedViews()
and nulled the caller-supplied schema, leaving a metadata with neither a
schema nor column metadata (the next getSchema() then NPE'd). It rejects that
segment, as addColumnMetadata() already did, and returns early -- without
dropping the derived views -- when the column is not there.
- The two mutators and the two derived-view builders now share the instance
monitor, so a schema or map built from columns that have already been replaced
can no longer be cached.
- SortedStringArraySet's range views did not enforce their own bounds, so
headSet("f").tailSet("h") returned an empty set where a TreeSet range view
throws. They carry their bounds and reject an out-of-range argument the way
TreeSet does.
- Documented that a segment holding no column metadata (CONSUMING) reports its
schema's columns from getAllColumns()/getNumColumns() while
getAllColumnMetadata() is empty and forEachColumn() visits nothing, so the two
families must not be paired; PhysicalColumnNames was pairing them, and now
counts its size from the same column metadata it iterates.
Heap: no new per-column term. Per segment the Columns holder costs ~20 B (one
object of 24 B replacing one of the two reference fields) and the retained
SortedStringArraySet grows 32 -> 40 B for its bounds, i.e. ~+28 B/segment, which
at 1000 columns is +0.03 B/column against the measured 166 B/column.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
xiangfu0
force-pushed
the
xiangfu0/data-3221-9-column-arrays
branch
from
September 9, 2026 01:56
a2313ef to
f0a20da
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## xiangfu0/data-3221-8-primitive-minmax #19481 +/- ##
===========================================================================
+ Coverage 57.76% 67.76% +9.99%
- Complexity 7 1430 +1423
===========================================================================
Files 2691 3493 +802
Lines 164589 225070 +60481
Branches 26741 35550 +8809
===========================================================================
+ Hits 95074 152515 +57441
+ Misses 61475 60517 -958
- Partials 8040 12038 +3998
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
SegmentMetadataImplkept its columns in aTreeMap, about 40 bytes of red-black-tree node per column. They now live in sorted parallel arrays with binary-search lookup, published as one immutable holder behind a single volatile reference so a reader can never pair new names with old metadata, and replaced copy-on-write.getColumnMetadataMap()still works: the map view is derived on demand and cached, and the load and query paths use the array accessors instead, so a normal segment load never materializes it. A test-visible counter plus a loader assertion keep it that way. Additive accessors were added for the callers that only need a count, a lookup or an iteration, and virtual columns are added through an explicitaddColumnMetadatarather than by mutating the returned map.Column order,
getAllColumns(),getPhysicalColumnNames()andtoJsonare unchanged.Tests
SegmentMetadataImplTest: the derived map equals the previous one and stays unbuilt afterImmutableSegmentLoader.load; adding and removing columns keeps the arrays sorted and consistent; consuming segments behave consistently across all accessors. NewSortedStringArraySetTestcovers the sorted-set view including range bounds.Why
A server keeps one metadata object graph per (segment, column) for as long as the segment is loaded, so on wide tables the per-column footprint decides how many segments a server can hold. Measured end to end on a 1000-column segment, this series takes the heap retained at load from 4.08 MB to 0.175 MB per segment (4,080 to 174 bytes per column), with a fully compacting collector on both sides. No on-disk format change, the
/tables/{table}/segments/{segment}/metadataJSON stays byte-identical, and every public and SPI signature keeps working.Stack
Part 9 of 9, based on #19479. Review only this part's own commits; the earlier parts account for the rest of the diff.