Repository navigation
Support range index on ingestion-aggregated no-dictionary columns - #19111
raghavyadav01 merged 7 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #19111 +/- ##
============================================
+ Coverage 68.07% 68.36% +0.28%
- Complexity 1450 1470 +20
============================================
Files 3518 3521 +3
Lines 228478 229096 +618
Branches 36201 36350 +149
============================================
+ Hits 155540 156624 +1084
+ Misses 60731 60185 -546
- Partials 12207 12287 +80
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:
|
Jackie-Jiang
left a comment
Jackie-Jiang
left a comment
There was a problem hiding this comment.
This is not the correct fix. We should allow building range index, and scan the column to figure out min/max value when they are unavailable.
|
Thanks @Jackie-Jiang . I am thinking to compute min/max on demand in MutableNoDictColumnStatistics — when the mutable segment reports null (aggregated columns skip min/max tracking since the value mutates), scan the sealed forward index once (same pass isSorted() already does), before the BitSliced creator is constructed. This adds one extra O(numDocs) scan per such column at seal time. Will this be OK? |
Yes. That is the overhead we need to pay in order to add range index. We should also check how RangeIndexHandler handles null min/max value. |
raghavyadav01
changed the title
raghavyadav01
changed the title
690e9ba to
542f75a
Compare
raghavyadav01
changed the title
Ingestion-time metrics aggregation forces its aggregated metric columns to be no-dictionary and skips min/max tracking during consumption (the values mutate in place). The BitSliced range index (version 2) creator subtracts the column min for INT/LONG columns, so with a null value domain segment commit failed with an NPE. Instead of rejecting the combination, recover the value domain: - MutableNoDictColumnStatistics computes min/max with a single scan of the sealed forward index when the mutable segment reports null, scoped to single-value INT/LONG (the only types whose BitSliced index reads min/max; FLOAT/DOUBLE use the full floating-point ordinal domain). The recovered values also flow into the committed segment metadata, improving pruning. - RangeIndexHandler recomputes min/max from the forward index on the index-reload path for segments committed before this change, via a new min/max override on IndexCreationContext.Builder. Adds unit coverage for min/max recovery and an end-to-end seal-path test that builds and verifies a range index on an aggregated no-dictionary column, and flips the validation tests to assert the combination is now accepted.
Recovering an untracked value domain was a second full scan of the column: computeSorted() calls getMinValue(), which now scans, and then scanned again itself. That regressed the single-pass guarantee two existing tests assert (testIsSortedScansOnce, testUntrackedMinMaxFallsBackToScan), and charged the extra pass to every ingestion-aggregated INT/LONG column at commit, whether or not a range index was configured -- BaseSegmentCreator reads min/max for every column when writing segment metadata. Fold the two together: the recovery scan walks docs in the order computeSorted() uses (honouring sortedDocIds) and records non-decreasing order alongside min and max; computeSorted() reuses that answer instead of rescanning. A segment commit reads such a column exactly once, as before.
542f75a to
6929ed4
Compare
The handler-side recovery had no test: the existing RangeIndexHandlerTest is mock-based and never builds an index, leaving that path at 22% coverage. Build a real segment over unsorted, partly negative INT and LONG no-dictionary columns, strip min/max from its metadata -- which is what a segment committed with untracked min/max looks like on disk -- then run the handler and query the resulting BitSliced index. Reverting RangeIndexHandler to its pre-change form fails both tests with the NPE this PR fixes. The second test pins the open-ended case: the reader still takes the column max from metadata and so falls back to Long.MAX_VALUE, but the RangeBitmap domain is self-contained, so matches stay correct.
The note claimed metadata min/max stay null until the segment is rebuilt. Under the default columnMinMaxValueGeneratorMode they do not: ColumnMinMaxValueGenerator runs later in the same SegmentPreProcessor pass and populates them. The weaker max-pruning caveat applies only when that mode is NONE. Also record that the default path consequently reads the column twice per preprocess.
Jackie-Jiang
left a comment
Jackie-Jiang
left a comment
There was a problem hiding this comment.
LGTM. The min/max recovery covers both realtime segment creation and reloading legacy segments, and I found no blocking correctness or compatibility issues. All 13 reported CI checks pass on this revision.
Left a few optional simplifications below. The main improvement would be caching the effective min/max directly in MutableNoDictColumnStatistics. The explicit INT/LONG loops are reasonable to keep.
| /// | ||
| /// Segments sealed with the recovery in {@code MutableNoDictColumnStatistics} carry proper metadata min/max and do | ||
| /// not reach the scan here. | ||
| private CombinedInvertedIndexCreator newRangeIndexCreator(ColumnMetadata columnMetadata, |
There was a problem hiding this comment.
Optional simplification: both callers already have a forward reader and context, so we could consolidate the two newRangeIndexCreator overloads into one helper. Add !columnMetadata.hasDictionary() to the recovery condition and keep the existing single-value/v2/missing-bounds checks. This removes the duplicated builder/config setup. The helper can also get numDocs from columnMetadata instead of receiving it separately.
Keep plain _minValue/_maxValue fields and the existing _sorted cache: recover missing bounds for single-value INT/LONG columns once at construction and record sortedness from the same scan, so the getters are field reads.
The compacted subclass computes its own bounds over valid docs only, and its sortedDocIds covers just those docs, so the base-class constructor scan would index past the array. Let subclasses opt out via a protected constructor.
Clarifies range-index support for ingestion-aggregated no-dictionary metrics and the extra scan when older INT/LONG segments lack min/max metadata. Follow-up to apache/pinot#19111. Co-authored-by: Xiang Fu <xiangfu@Xiang-mac-mtv-2.local>
|
Docs follow-up merged: pinot-contrib/pinot-docs#1091. The range-index guide now covers ingestion-aggregated no-dictionary metrics and the min/max recovery scan for older INT/LONG segments. |
4 participants
PR flow
Recover min/max from forward index for ingestion-aggregated no-dictionary columns to enable BitSliced range index.
AI-generated · Green: added · Yellow: modified · Red: removed · Gray: existing
Diff evidence
Problem
Configuring a BitSliced (version 2) range index on an ingestion-aggregated metric column causes segments to fail to commit.
Ingestion-time metrics aggregation (
aggregationConfigs, or the legacyaggregateMetricsflag) forces its aggregated metric columns to be no-dictionary and skips min/max tracking while the consuming segment is being built — the values mutate in place, so a running min/max would be meaningless (MutableSegmentImpldeliberately skips it). At segment commit time min/max are thereforenull, and the BitSliced range index creator reads them for a single-value no-dictionary column:For INT/LONG columns this throws a
NullPointerExceptionat segment build time (((Number) minValue).longValue()), so the affected real-time segments can never commit.Fix
Rather than reject the combination, recover the value domain at build time:
MutableNoDictColumnStatistics— when the mutable segment reportsnullmin/max, compute them with a single scan of the sealed forward index. Scoped to single-value INT/LONG, the only stored types whose BitSliced range index reads min/max (FLOAT/DOUBLE use the full floating-point ordinal domain and never dereference min/max). The recovered values also flow into the committed segment metadata via the existing metadata path, so aggregated columns now get accurate segment-level min/max (a pruning improvement).RangeIndexHandler— the index-reload path recomputes min/max from the forward index for segments that were committed before this change (whose metadata min/max are stillnull), via a new min/max override onIndexCreationContext.Builder. The recovered domain is written into the range index header (which the reader uses for the subtract-min); for such legacy segments the reader still reads a null metadata max and falls back toLong.MAX_VALUE— results stay correct (the RangeBitmap domain is self-contained), only segment-level max pruning is weaker until the segment is rebuilt. Segments sealed after this change carry proper metadata min/max and do not hit that path.Reverts the interim table-config rejection in
RangeIndexType.validate()and the loud-fail guard inBitSlicedRangeIndexCreator(both introduced on this branch, never released).Notes / discussion
isSorted()scan already performed there). This is the "overhead we pay to add the range index"; it doubles as a pruning benefit since these columns previously had no segment min/max. Happy to gate it further if preferred.Testing
MutableNoDictColumnStatisticsTest— min/max recovered for single-value INT/LONG when metadata is null; FLOAT/DOUBLE and multi-value columns intentionally stay null.RealtimeSegmentConverterTest— end-to-end: an ingestion-aggregated no-dictionary LONG metric column with a range index seals successfully, its column metadata carries the recovered min/max, and a BitSliced range index is present on the column.IndexCombinationValidationTest— the previously-rejected combinations (both aggregation paths, REALTIME) now validate; version-1, offline, and non-aggregated cases still validate.BitSlicedIndexCreatorTest— reverted to the pre-guard state.Release Notes
A version-2 (BitSliced) range index on an ingestion-aggregated no-dictionary column is now supported: its min/max value domain is recovered from the sealed forward index at segment build time instead of failing to commit.