Share FieldSpecs and repeated values across segment metadata loads - #19473
Share FieldSpecs and repeated values across segment metadata loads#19473xiangfu0 wants to merge 5 commits into
Conversation
xiangfu0
added
performance
c566594 to
b61c923
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #19473 +/- ##
============================================
+ Coverage 67.72% 67.74% +0.02%
Complexity 1450 1450
============================================
Files 3490 3490
Lines 225032 225147 +115
Branches 35527 35552 +25
============================================
+ Hits 152393 152530 +137
+ Misses 60612 60585 -27
- Partials 12027 12032 +5
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:
|
b61c923 to
987446c
Compare
987446c to
e08955f
Compare
There was a problem hiding this comment.
🟡 Changes recommended
One or more issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Canonicalizes default-null values and interns repeated metadata strings to reduce per-segment heap usage.
Changes:
- Reuses type-default constants and interns custom literals.
- Interns column, parent, date-time, and complex-child names.
- Adds identity, JSON, and cross-segment sharing tests.
File summaries
| File | Description |
|---|---|
| pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImplTest.java | Updated as part of this pull request. |
| pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/SegmentMetadataImpl.java | Updated as part of this pull request. |
| pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImpl.java | Updated as part of this pull request. |
| pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/SegmentMetadataImplTest.java | Updated as part of this pull request. |
Review details
Suppressed comments (2)
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/SegmentMetadataImplTest.java:67
- These static imports are out of the lexicographic order used by the surrounding tests and the repository's Spotless import-order configuration; formatting/checks will reorder them. Move
assertSamebeforeassertTrue.
import static org.testng.Assert.assertTrue;
import static org.testng.Assert.assertSame;
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImpl.java:575
- The new non-default path claims to share custom literals via
literal.intern(), but the added custom-default tests only use value equality, and the segment-level test covers only type-default constants. Add a focused parse/load pair with distinctStringinstances for a custom STRING/JSON default and assert identity; otherwise removing this interning would still pass the current suite.
return dataType.equals(FieldSpec.getDefaultNullValue(fieldType, dataType, literal), typeDefault) ? null
: literal.intern();
- 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.
Jackie-Jiang
left a comment
Jackie-Jiang
left a comment
There was a problem hiding this comment.
Most segment will have the same FieldSpec, so ideally if we can dedup the FieldSpec, that will provide the biggest saving
|
@Jackie-Jiang, whole- I kept that change as a separate reviewable layer and added an explicit pointer near the top of this PR description. |
|
@Jackie-Jiang Yes, sharing the whole COMPLEX parents are excluded because their equality does not include children; ordinary child specs are shared. The read-only contract for shared segment-derived specs is documented there. This PR handles repeated values inside and alongside the specs, including metadata-map keys and the segment time-column name. I updated the description to call out #19476 explicitly and corrected the stale base-branch description: #19473 now targets |
xiangfu0
changed the title
|
Folded #19476 into this PR in 862528b so FieldSpec deduplication and value canonicalization can be reviewed together. Equal DIMENSION, METRIC, TIME, and DATE_TIME specs now share a weakly interned instance across loaded segments. COMPLEX parents remain separate because their equality does not include children; their eligible children share normally. The metadata APIs document the read-only contract. The 48 focused metadata tests pass, including equal/different definitions, shared specs across segment loads, distinct schema containers, custom STRING/JSON literals across distinct specs, and time-column identity. Spotless, Checkstyle, and license checks pass. The dependent stack was restacked to keep each optimization once; #19477 now follows #19475 and #19476 is superseded. |
…metadata parse time
A server retains one ColumnMetadataImpl + FieldSpec per (segment, column) for as long as the segment is loaded.
For wide segments (1000+ columns, tens of thousands of segments per server) two per-column allocations made
on the metadata.properties parse path add up to ~120 bytes per column that carry no information:
- The segment creator writes `column.<c>.defaultNullValue` for every column, so `extractFieldSpec` handed the
literal to the FieldSpec constructor, which boxed it (`Integer.valueOf("-2147483648")`) and kept the literal
in the transient `_stringDefaultNullValue` - a fresh Integer, String and byte[] per column per segment even
though the value is the type default. `ColumnMetadataImpl.extractFieldSpec` now parses the literal, compares
it with `FieldSpec.getDefaultNullValue(fieldType, dataType, null)` under `DataType.equals` (the predicate
`FieldSpec.equals`/`hashCode` use, byte[]-safe) and passes `null` when they match, so the spec holds the
shared static `FieldSpec.DEFAULT_*` constant and retains no literal. A custom default is kept verbatim (after
the STRING special-character recovery) and interned, so the segments of a table share it; a combination
without a type default (a METRIC BOOLEAN with an explicit default) keeps parsing the literal as before.
- Column names recur in every segment of a table, yet each segment parsed its own copy that was then retained
six times over: the column metadata map key, FieldSpec._name, the Schema map key and dimension/metric list
entry, and the loader's per-column maps. `SegmentMetadataImpl.addPhysicalColumns` now interns the parsed name,
and `extractFieldSpec` interns COLUMN_NAME, PARENT_COLUMN, DATETIME_FORMAT, DATETIME_GRANULARITY and complex
child names, so every String the metadata graph retains is one instance per distinct value per JVM. The JVM
string table holds interned strings weakly, so they live exactly as long as a loaded segment references them
and the table is bounded by the number of distinct column names; no code compares column names by identity.
Compatibility: metadata.properties is read and written exactly as before (writers keep emitting
defaultNullValue). The `/tables/{t}/segments/{s}/metadata` payload is unchanged because it bean-serializes
`FieldSpec.getDefaultNullValue()` (a value that is equal by construction) - pinned by a test for every data
type, including BYTES and UUID. FieldSpec.equals/hashCode ignore `_stringDefaultNullValue` and
getDefaultNullValueString() derives from the value, so Schema equality against the table schema, default-column
comparisons and reload decisions are unaffected. The only observable difference is that
`FieldSpec.toJsonObject()` of a segment-derived BYTES column no longer emits a redundant `defaultNullValue: ""`
(it compares the byte[] against the type default by identity, and the spec now holds the constant); no
endpoint serializes a segment-derived Schema that way. No public signature changes; mixed-version safe
(server-local, in-memory only).
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
862528b to
e265da9
Compare
|
Fixed the shared-mutation correctness issue in a564ed6. Interned specs now reject setters, detach/freeze nested TIME settings and collection aliases, and defensively copy mutable defaults. Ordinary schemas and deserialized Validation: 130 SPI/metadata tests plus 6 actual SQL null-ordering cases passed; Spotless, Checkstyle and license checks passed. Regression tests cover cross-segment mutation, stable interner keys, BYTES/UUID arrays, Jackson updates and editable copies. Independent source review has no remaining findings. The fresh three-way benchmark is complete; detailed methodology and numbers are in the PR description. With G1 dedup ON, the reachable metadata graph is 39.7–42.2% smaller than base and unchanged by freezing in these fixtures. One-thread custom-default parsing is 6.1% slower than base (1.8% slower than pre-fix). GROUP BY mean latency is 2.5–3.4% higher, while SUM is 3.7–6.4% lower, with overlapping fork ranges. Nonempty BYTES/UUID getters have a measured allocation cost: 32 B/read for 16-byte defaults, or 1,040 B/read for 1 KiB defaults. This is a bounded local comparison, not a production latency or whole-server heap claim. Current-head CI is still running; local validation is separate from CI and approval. |
PR flow
Sharing FieldSpecs across segment loads by interning strings, canonicalizing defaults, freezing specs, and storing them in a weak interner.
AI-generated · Green: added · Yellow: modified · Red: removed · Gray: existing
Partial evidence: 0 file patches omitted; 1 truncated.
Diff evidence
What
Each loaded segment retains its own column definitions and repeated metadata values. Metadata parsing now shares equal
FieldSpecinstances and the strings and default values they retain:FieldSpec.equals/hashCode. Equal parsed column definitions can share across segments and tables; different definitions remain distinct, and the interner does not keep unused specs alive.ComplexFieldSpecparents remain separate because their equality does not include their children; their ordinary child specs are shared. Shared segment-derived specs are frozen before interning: setters reject mutation, nested time settings and collection properties are detached and protected, and mutable default values (including BYTES and UUID arrays) are defensively copied on access. This prevents a caller from changing another segment or corrupting the interner key. TheSchemacontainer remains separate for each segment. Ordinary table schemas remain mutable; callers needing an editable field definition can deserializeFieldSpec.toJsonObject().Custom defaults preserve their value semantics, including BIG_DECIMAL scale and negative-zero FLOAT/DOUBLE values. On-disk formats, public signatures, and segment-metadata REST JSON remain unchanged.
Tests
ColumnMetadataImplTest: shared type-default constants where available, value-equivalent UUID defaults, custom STRING/JSON identity across distinct specs, literal round-trips, date-time metadata, and serialized JSON equivalence. Equal specs share identity; changes to names, types, defaults, the single-value setting, max length, or time format/granularity remain distinct. COMPLEX parents stay separate while children share.SegmentMetadataImplTest: shared names, defaults, and FieldSpecs across loads; separate Schema containers; OPEN_STRUCT parents and children; time-column identity across directory/stream loads and metadata-map keys; and null time columns.FieldSpecFreezeTest: all setter guards, retained aliases, recursive mutable defaults, nested TIME state, Jackson update rejection, and mutable JSON/Java serialization copies. Metadata regressions also cover cross-segment mutation rejection and stable interner identity after attempted mutation.Local validation at
a564ed6de4: 130 focused SPI/metadata tests and 6 actual SQL DISTINCT/null-ordering cases (STRING, BIG_DECIMAL, BYTES) pass with JDK 25. Spotless, Checkstyle, and license checks pass for all three affected modules. An independent review across eight domains has no remaining findings.Performance rerun at a564ed6
Three-way JMH comparison: base
5771d6acea, previous heade265da96c8, and the immutability fixa564ed6de4. Each timing case has three fresh JVM forks in balanced order, with 3 × 1-second warmup and 5 × 1-second measurement iterations, on JDK 25 with G1 string deduplication enabled. All variants use the same freshly built dependency runtime and exact source overlays.The eight-thread metadata means are 6–8% higher than base but have substantial fork variation. SQL directions are mixed and fork ranges overlap; these results do not establish a general query speedup or slowdown. SQL uses four native v3 segments, 10,000 rows and 200 INT columns per segment, through real local filter/aggregation/broker reduction code. Independent results are checked before timing and after measurement with the loader active; all background rates are within 1% of 200 parses/s. This excludes network, distributed service, and Parquet/S3 latency.
Frozen getters incur the intended defensive-copy cost: 16-byte BYTES/UUID reads average 2.2–2.3 ns and allocate 32 B, while 1-KiB BYTES averages 26.0 ns and allocates 1,040 B (base/pre-fix getters are approximately 0.46 ns with no allocation). INT and empty BYTES remain allocation-free. These are per-getter measurements; inspected readers cache defaults during setup, and the DISTINCT null-placeholder path reads once per segment rather than per row.
Memory fixtures retain 200 columns × 200 separately parsed segment metadata objects; shared FieldSpecs fall from 40,000 to 200. The graph includes instance fields and backing arrays but excludes static weak-interner/native tables and other server memory, so this is not a net whole-heap result. Deduplication activity is verified in GC logs and all graphs stabilize across three final passes. The scalar/mixed memory fixtures do not measure TIME or mutable collection/binary defaults. All raw fork data, source/dependency hashes, fixture snapshots, commands, and verification results are retained in the local benchmark artifact.
Stack
Based on
master. Review this PR against its base for this layer's changes.Current open chain; #19480 (lazy index-size storage) is already merged, and #19476 is absorbed into #19473.