Jackie-Jiang commented on code in PR #19111:
URL: https://github.com/apache/pinot/pull/19111#discussion_r4187512019
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/RangeIndexHandler.java:
##########
@@ -301,4 +304,72 @@ private CombinedInvertedIndexCreator
newRangeIndexCreator(ColumnMetadata columnM
.getConfig(StandardIndexes.range());
return StandardIndexes.range().createIndexCreator(context, config);
}
+
+ /// Variant for the non-dictionary path. The BitSliced (v2) range index
subtracts the column min for INT/LONG
+ /// columns, so it needs the value domain up front. Ingestion-aggregated
no-dictionary columns committed before
+ /// min/max recovery report null min/max in their metadata, so recompute it
here with a single scan of the forward
+ /// index before building the creator. This is a deliberate extra pass over
the column: {@code RangeBitmap.appender}
+ /// requires the max at construction, so it cannot be folded into the
add-loop that follows.
+ ///
+ /// Note: the recovered domain goes into the range index header, which is
what the reader uses for the subtract-min,
+ /// not into the column metadata. Under the default {@code
columnMinMaxValueGeneratorMode} the metadata is populated
+ /// anyway, because {@code ColumnMinMaxValueGenerator} runs later in the
same preprocess and scans the column itself
+ /// -- which also means such a column is read twice per preprocess, once
here and once there. Only when that mode is
+ /// NONE does the metadata stay empty, leaving the reader to fall back to
{@code Long.MAX_VALUE} for the column max;
+ /// results are still correct there (the RangeBitmap domain is
self-contained), but segment-level max pruning is
+ /// weaker.
+ ///
+ /// 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,
Review 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.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/converter/stats/MutableNoDictColumnStatistics.java:
##########
@@ -52,6 +52,18 @@ public class MutableNoDictColumnStatistics implements
ColumnStatistics, CLPStats
// the same value.
private Boolean _sorted;
+ // Lazily-computed min/max for columns whose mutable segment does not track
them (ingestion-aggregated metric
Review Comment:
Optional simplification: can we keep `_minValue`, `_maxValue`, and the
existing `_sorted`? Initialize the bounds from metadata in the constructor, and
recover missing bounds for single-value INT/LONG columns there, recording
sortedness in the same scan while respecting `_isSortedColumn`. Then the
min/max getters become simple field reads, and we can remove `_minMaxComputed`,
`_scanSorted`, and the distinction between metadata values and
`_computedMinValue`/`_computedMaxValue`. Segment creation reads these bounds
anyway. The constructor already requires `numDocs > 0`, so the additional
positive-count check in this class can also go; the reload helper's
empty-column handling should remain.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/converter/stats/MutableNoDictColumnStatistics.java:
##########
@@ -76,12 +88,94 @@ public int getTotalDocs() {
@Override
public Comparable<?> getMinValue() {
- return (Comparable<?>) _dataSourceMetadata.getMinValue();
+ Comparable<?> minValue = (Comparable<?>) _dataSourceMetadata.getMinValue();
+ if (minValue != null) {
+ return minValue;
+ }
+ computeMinMaxIfNeeded();
+ return _computedMinValue;
}
@Override
public Comparable<?> getMaxValue() {
- return (Comparable<?>) _dataSourceMetadata.getMaxValue();
+ Comparable<?> maxValue = (Comparable<?>) _dataSourceMetadata.getMaxValue();
+ if (maxValue != null) {
+ return maxValue;
+ }
+ computeMinMaxIfNeeded();
+ return _computedMaxValue;
+ }
+
+ /// Computes min/max by scanning the sealed forward index once, caching the
result. Only invoked when the mutable
+ /// segment reports null min/max, which happens for ingestion-aggregated
metric columns: their values mutate in
+ /// place during consumption, so `MutableSegmentImpl` deliberately skips
min/max tracking for them. Without a value
+ /// domain the BitSliced range index creator cannot subtract the min for
INT/LONG columns, so we recover it here.
+ ///
+ /// This is the same pass {@link #isSorted()} already performs at seal time
for these columns, not an extra one: it
Review Comment:
Optional documentation cleanup: the previous sortedness scan could exit on
the first out-of-order pair, whereas min/max recovery must scan the whole
column. Saying that min/max and sortedness share one full scan would be more
precise than implying unchanged scanning cost. The comments can be shortened to
explain why bounds are missing, why recovery is limited to INT/LONG, and how
the scan reuses sortedness. The reentrancy explanation at the end of the method
can also be removed.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/RangeIndexHandler.java:
##########
@@ -301,4 +304,72 @@ private CombinedInvertedIndexCreator
newRangeIndexCreator(ColumnMetadata columnM
.getConfig(StandardIndexes.range());
return StandardIndexes.range().createIndexCreator(context, config);
}
+
+ /// Variant for the non-dictionary path. The BitSliced (v2) range index
subtracts the column min for INT/LONG
+ /// columns, so it needs the value domain up front. Ingestion-aggregated
no-dictionary columns committed before
+ /// min/max recovery report null min/max in their metadata, so recompute it
here with a single scan of the forward
+ /// index before building the creator. This is a deliberate extra pass over
the column: {@code RangeBitmap.appender}
+ /// requires the max at construction, so it cannot be folded into the
add-loop that follows.
+ ///
+ /// Note: the recovered domain goes into the range index header, which is
what the reader uses for the subtract-min,
+ /// not into the column metadata. Under the default {@code
columnMinMaxValueGeneratorMode} the metadata is populated
+ /// anyway, because {@code ColumnMinMaxValueGenerator} runs later in the
same preprocess and scans the column itself
+ /// -- which also means such a column is read twice per preprocess, once
here and once there. Only when that mode is
Review Comment:
Small documentation correction: `NONE` is not the only mode that leaves
these metric bounds absent. `TIME` and `NON_METRIC` also skip metric columns.
We can shorten this to say that metadata may remain absent when the configured
generator mode excludes the column; the reader's conservative fallback remains
correct.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
