[WIP] Optimize heap usage of dictionary build to support dict on high cardinality STRINGs (fast filtering)#229
Draft
siddharthteotia wants to merge 5 commits into
Draft
Conversation
…ING/BYTES (3a) For high-cardinality STRING/BYTES columns, the on-heap value->dictId map (Object2IntOpenHashMap) is a dominant heap cost during offline segment generation and a frequent OOM source. Above a configurable cardinality threshold, skip building that map and resolve dictIds by binary-searching the just-written, mmap-backed dictionary buffer (via the same immutable dictionary reader used at load time) instead. Trades an O(1) on-heap lookup for an O(log N) off-heap read per value, removing the map from heap entirely. - Opt-in: threshold defaults to Integer.MAX_VALUE, so default behavior is unchanged. Decision is per-column, against that column's own cardinality. - Scoped to STRING/BYTES; numeric dictionaries keep their cheap primitive maps. - Lookup reader + mmap buffer are released in postIndexingCleanup()/close(). - 22 functional tests prove indexOfSV/indexOfMV return identical dictIds in map mode vs binary-search mode (STRING/BYTES x var/fixed x SV/MV), and the PinotBuffersAfterMethodCheckRule confirms no buffer leaks. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… columns (3b) 3a removes the on-heap value->dictId map for high-cardinality STRING/BYTES columns, but the distinct values stay pinned by the sorted unique-values array held in the stats collector through the whole creation. This frees that array right after init() (once the dictionary and any value-array-consuming indexes such as FST are built, before the row-indexing pass), so the GC can reclaim the distinct values. Together with 3a this drops a high-card column's dictionary heap to ~0. - ColumnStatistics gains a default no-op releaseSortedValues(); String/Bytes collectors override it to cache min/max/cardinality, then null _sortedValues. Getters return the cached scalars once released; numeric collectors keep the default no-op (3b is scoped to STRING/BYTES, the OOM-driving types). - ColumnIndexCreationInfo.getDistinctValueCount() falls back to the cached cardinality when the array is null (previously a dead null-branch). - SegmentColumnarIndexCreator.init() releases the array only for columns in binary-search mode (creator.isUsingBinarySearchLookup()). - Like 3a, dormant until a threshold config flips columns to binary search. - Tests: collector release keeps min/max/cardinality and ColumnIndexCreationInfo reporting; numeric release is a no-op; DictionariesTest regression green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…a/3b) - BenchmarkDictionaryHeapFootprint: JOL GraphLayout retained size of the map-mode structures (sorted String[] + Object2IntOpenHashMap) across cardinality x avg length. Confirms the on-heap footprint binary-search mode eliminates, and validates the ~(60+L) bytes/value model. - BenchmarkSegmentDictionaryCreation: JMH lookupMap vs lookupBinarySearch per-row dictId cost, plus buildMap/buildBinarySearch, swept over cardinality x avg length x var/fixed encoding. - Adds jol-core (0.17) to pinot-perf. Measured (JOL): bytes/value tracks 60+L within a few percent (e.g. 1M x L=32 = 88 MB, 3M x L=100 = 455 MB), all eliminated in binary-search mode. Measured (JMH, indicative): binary search is a ~30-55x per-lookup penalty vs the hash map (constant factor, no cardinality crossover), i.e. ~2-4 s extra encode per fat column on a 5M-row segment -- the bounded CPU price for the heap saved. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…fig) 3a/3b added a per-column binary-search dictId lookup (skipping the on-heap value->dictId map) plus early release of the sorted unique-values array for high-cardinality STRING/BYTES columns, but the flip was driven only by SegmentDictionaryCreator._onHeapLookupMapMaxCardinality (default Integer.MAX_VALUE), which nothing in production sets -- so the feature was dormant. This wires it to a real, per-table config so it can fire end-to-end, gated on an estimated byte budget rather than a flat cardinality count. New knob IndexingConfig.dictionaryOnHeapLookupMapMaxBytes (a size in bytes; default Long.MAX_VALUE = effectively disabled, so behavior is unchanged unless set). A dict-enabled STRING/BYTES column flips to the low-heap path when its estimated on-heap footprint (60 + maxLen) * cardinality >= the budget, where cardinality = numValues and maxLen = the column's longest value in bytes (the L proxy from the (60 + L) per-value model measured in the design doc). A byte budget (vs a flat cardinality) correctly targets long-string columns, which a flat count under-targets. Plumbing: IndexingConfig -> SegmentGeneratorConfig -> IndexCreationContext -> DictionaryIndexType.createIndexCreator. The gate is applied by converting the byte budget to the equivalent per-column cardinality threshold (ceil(budget / (60 + maxLen)) - 1) and feeding the existing, already-tested setOnHeapLookupMapMaxCardinality(int) setter, so the tested lookup path in SegmentDictionaryCreator is untouched. No-op for numeric columns (cheap primitive maps, never the OOM driver) and when the budget is disabled. Compiles clean (pinot-spi, pinot-segment-spi, pinot-segment-local). Tests (gate-wiring unit test + end-to-end build/load test) to follow in a subsequent commit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Covers the byte-budget wiring added in the previous commit, which flips a high-cardinality STRING/BYTES column to the low-heap binary-search dictId lookup (3a) + early sorted-array release (3b) when its estimated on-heap footprint (60 + maxLen) * cardinality >= the configured budget. DictionaryIndexTypeOnHeapLookupBudgetTest (gate-wiring unit test): asserts the per-column flip decision made by DictionaryIndexType.createIndexCreator matches the byte-budget formula exactly at the boundary (one below stays on the on-heap map, at/above flips), across several (budget, maxLen) points, for STRING and BYTES and for both single-value (SV) and multi-value (MV) columns -- the switch is scoped to the stored type, so it must flip identically for SV and MV. Also verifies that every non-STRING/BYTES stored type (INT, LONG, FLOAT, DOUBLE, BIG_DECIMAL), SV and MV, never flips even at a tiny budget, and that the default/disabled budget (Long.MAX_VALUE, <= 0) keeps map mode -- including that a fresh, unconfigured IndexingConfig defaults to disabled (no behavior change). This nails the ceil/off-by-one conversion from byte budget to the creator's strict cardinality > threshold. SegmentGenerationWithDictionaryOnHeapLookupBudgetTest (end-to-end): builds a real segment carrying all four in-scope column types -- SV STRING, MV STRING, SV BYTES, MV BYTES (plus a LONG metric) -- and, because the on-disk segment is identical whether or not a column flips, directly confirms which path each column took by reading SegmentDictionaryCreator.isUsingBinarySearchLookup() off the driver's columnar index creator. It asserts the switch fired for every STRING/BYTES column when enabled, fired for none under the default/disabled config, and that in both cases every row round-trips through the forward-index -> dictionary path (incl. MV element order and BYTES values) with correct min/max/cardinality metadata. A parity build (ON vs OFF) asserts the two paths produce identical row values and metadata, so a bug in the binary-search dictId encoding for any SV/MV STRING/BYTES column would surface. The direct fired-flag observation is what makes these correctness checks non-vacuous. All new tests pass (18 gate-wiring + 4 end-to-end); existing dictionary/context tests (DictionariesTest, SegmentDictionaryCreatorBinarySearchLookupTest, ColumnStatisticsReleaseSortedValuesTest, DictionaryIndexTypeTest, InvertedIndexVersionConfigTest) remain green. Spotless + checkstyle clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
siddharthteotia
changed the title
siddharthteotia
changed the title
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
1 participant
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.
No description provided.