This is an automated email from the ASF dual-hosted git repository.
Jackie-Jiang pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git
The following commit(s) were added to refs/heads/master by this push:
new de208963939 Restore constant-column short-circuit in realtime segment
isSorted() (#19163)
de208963939 is described below
commit de20896393928a2e8f1425aa3fc8699f59a7e4ba
Author: Xiaotian (Jackie) Jiang <[email protected]>
AuthorDate: Wed Aug 5 14:42:08 2026 -0700
Restore constant-column short-circuit in realtime segment isSorted()
(#19163)
---
.../converter/stats/CompactedColumnStatistics.java | 33 ++++++++++------
.../converter/stats/MutableColumnStatistics.java | 46 +++++++++++++++++-----
.../stats/MutableNoDictColumnStatistics.java | 14 +++++++
.../stats/CompactedColumnStatisticsTest.java | 28 +++++++++++--
.../stats/MutableColumnStatisticsTest.java | 29 ++++++++++++--
.../stats/MutableNoDictColumnStatisticsTest.java | 26 ++++++++++--
6 files changed, 145 insertions(+), 31 deletions(-)
diff --git
a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/converter/stats/CompactedColumnStatistics.java
b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/converter/stats/CompactedColumnStatistics.java
index 95ad415eb04..a1464e584fc 100644
---
a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/converter/stats/CompactedColumnStatistics.java
+++
b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/converter/stats/CompactedColumnStatistics.java
@@ -67,41 +67,50 @@ public class CompactedColumnStatistics extends
MutableColumnStatistics {
// Single pass over valid documents to collect used dict IDs and entry
counts.
// For SV columns, sort order is tracked inline: when sortedDocIds is
provided, iterate in that order; when null,
- // iterate via the bitmap.
+ // iterate via the bitmap. A single-value dictionary is sorted by
construction, so the scan is skipped entirely.
// isSorted is initialized to false for sorted columns to skip the per-doc
dictionary compare entirely.
// MV columns are never sorted, so we always iterate via the bitmap
regardless of sortedDocIds.
IntOpenHashSet usedDictIds = new IntOpenHashSet();
boolean isSorted = !_isSortedColumn;
- int prevDictId = -1;
- int maxRowLength = 0;
int totalEntries = 0;
int maxMultiValues = 0;
+ int maxRowLength = 0;
if (isSingleValue) {
- if (_sortedDocIds != null) {
+ totalEntries = _totalDocs;
+ if (dictionary.length() == 1) {
+ // Every document maps to the only dict id, so scanning the forward
index cannot discover anything: the used
+ // dict ids are exactly {0}, and an SV column contributes one entry
per valid document
+ usedDictIds.add(0);
+ isSorted = true;
+ } else if (_sortedDocIds != null) {
+ int prevDictId = -1;
for (int docId : _sortedDocIds) {
if (!validDocIds.contains(docId)) {
continue;
}
int dictId = forwardIndex.getDictId(docId);
- totalEntries++;
- usedDictIds.add(dictId);
- if (isSorted) {
- if (prevDictId != -1 && dictionary.compare(prevDictId, dictId) >
0) {
+ // Repeating the previous dict id needs no work: it is already in
the set, and a value compared against
+ // itself can never break the sort order
+ if (dictId != prevDictId) {
+ usedDictIds.add(dictId);
+ if (isSorted && prevDictId != -1 && dictionary.compare(prevDictId,
dictId) > 0) {
isSorted = false;
}
prevDictId = dictId;
}
}
} else {
+ int prevDictId = -1;
org.roaringbitmap.IntIterator iterator = validDocIds.getIntIterator();
while (iterator.hasNext()) {
int docId = iterator.next();
int dictId = forwardIndex.getDictId(docId);
- totalEntries++;
- usedDictIds.add(dictId);
- if (isSorted) {
- if (prevDictId != -1 && dictionary.compare(prevDictId, dictId) >
0) {
+ // Repeating the previous dict id needs no work: it is already in
the set, and a value compared against
+ // itself can never break the sort order
+ if (dictId != prevDictId) {
+ usedDictIds.add(dictId);
+ if (isSorted && prevDictId != -1 && dictionary.compare(prevDictId,
dictId) > 0) {
isSorted = false;
}
prevDictId = dictId;
diff --git
a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/converter/stats/MutableColumnStatistics.java
b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/converter/stats/MutableColumnStatistics.java
index 1c9453c0252..0cd47f5971d 100644
---
a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/converter/stats/MutableColumnStatistics.java
+++
b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/converter/stats/MutableColumnStatistics.java
@@ -43,6 +43,13 @@ public class MutableColumnStatistics implements
ColumnStatistics {
// dictionary.
protected final Dictionary _dictionary;
+ // Lazily computed because it may require a full scan of the forward index,
and it is queried multiple times per
+ // column during segment creation. Left unsynchronized: an instance
describes a single column and is reached only
+ // through the per-column stats map, so it is confined to whichever thread
creates that column. Even if that ever
+ // changes, the segment no longer accepts documents by the time stats are
collected, so a race can only recompute
+ // the same value.
+ private Boolean _sorted;
+
public MutableColumnStatistics(DataSource dataSource, @Nullable int[]
sortedDocIds, boolean isSortedColumn) {
_dataSource = dataSource;
_dataSourceMetadata = dataSource.getDataSourceMetadata();
@@ -102,6 +109,13 @@ public class MutableColumnStatistics implements
ColumnStatistics {
@Override
public boolean isSorted() {
+ if (_sorted == null) {
+ _sorted = computeSorted();
+ }
+ return _sorted;
+ }
+
+ private boolean computeSorted() {
// Sorted column is guaranteed to be sorted by construction — no scan
needed
if (_isSortedColumn) {
return true;
@@ -112,28 +126,40 @@ public class MutableColumnStatistics implements
ColumnStatistics {
return false;
}
+ // A single distinct value is always sorted — no scan needed. Cardinality
cannot be 0 here because the segment is
+ // non-empty and every document of a dictionary-encoded column has a dict
id.
+ if (getCardinality() == 1) {
+ return true;
+ }
+
// Iterate over all data to figure out whether or not it's in sorted order
MutableForwardIndex forwardIndex = (MutableForwardIndex)
_dataSource.getForwardIndex();
Preconditions.checkState(forwardIndex != null, "Failed to find forward
index for column: %s", _fieldSpec.getName());
int numDocs = _dataSourceMetadata.getNumDocs();
// Iterate with the sorted order if provided
if (_sortedDocIds != null) {
- int previousDictId = forwardIndex.getDictId(_sortedDocIds[0]);
+ int prevDictId = forwardIndex.getDictId(_sortedDocIds[0]);
for (int i = 1; i < numDocs; i++) {
- int currentDictId = forwardIndex.getDictId(_sortedDocIds[i]);
- if (_dictionary.compare(previousDictId, currentDictId) > 0) {
- return false;
+ int dictId = forwardIndex.getDictId(_sortedDocIds[i]);
+ // A repeated dict id cannot break the sort order, so skip the
comparison entirely
+ if (dictId != prevDictId) {
+ if (_dictionary.compare(prevDictId, dictId) > 0) {
+ return false;
+ }
+ prevDictId = dictId;
}
- previousDictId = currentDictId;
}
} else {
- int previousDictId = forwardIndex.getDictId(0);
+ int prevDictId = forwardIndex.getDictId(0);
for (int i = 1; i < numDocs; i++) {
- int currentDictId = forwardIndex.getDictId(i);
- if (_dictionary.compare(previousDictId, currentDictId) > 0) {
- return false;
+ int dictId = forwardIndex.getDictId(i);
+ // A repeated dict id cannot break the sort order, so skip the
comparison entirely
+ if (dictId != prevDictId) {
+ if (_dictionary.compare(prevDictId, dictId) > 0) {
+ return false;
+ }
+ prevDictId = dictId;
}
- previousDictId = currentDictId;
}
}
diff --git
a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/converter/stats/MutableNoDictColumnStatistics.java
b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/converter/stats/MutableNoDictColumnStatistics.java
index 85c0e242329..91766579579 100644
---
a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/converter/stats/MutableNoDictColumnStatistics.java
+++
b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/converter/stats/MutableNoDictColumnStatistics.java
@@ -45,6 +45,13 @@ public class MutableNoDictColumnStatistics implements
ColumnStatistics, CLPStats
protected final boolean _isSortedColumn;
protected final MutableForwardIndex _forwardIndex;
+ // Lazily computed because it may require a full scan of the forward index,
and it is queried multiple times per
+ // column during segment creation. Left unsynchronized: an instance
describes a single column and is reached only
+ // through the per-column stats map, so it is confined to whichever thread
creates that column. Even if that ever
+ // changes, the segment no longer accepts documents by the time stats are
collected, so a race can only recompute
+ // the same value.
+ private Boolean _sorted;
+
public MutableNoDictColumnStatistics(DataSource dataSource, @Nullable int[]
sortedDocIds, boolean isSortedColumn) {
_dataSourceMetadata = dataSource.getDataSourceMetadata();
_fieldSpec = _dataSourceMetadata.getFieldSpec();
@@ -105,6 +112,13 @@ public class MutableNoDictColumnStatistics implements
ColumnStatistics, CLPStats
@Override
public boolean isSorted() {
+ if (_sorted == null) {
+ _sorted = computeSorted();
+ }
+ return _sorted;
+ }
+
+ private boolean computeSorted() {
// Sorted column is guaranteed to be sorted by construction — no scan
needed
if (_isSortedColumn) {
return true;
diff --git
a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/realtime/converter/stats/CompactedColumnStatisticsTest.java
b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/realtime/converter/stats/CompactedColumnStatisticsTest.java
index a8f5306fd21..a7cc6da065c 100644
---
a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/realtime/converter/stats/CompactedColumnStatisticsTest.java
+++
b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/realtime/converter/stats/CompactedColumnStatisticsTest.java
@@ -30,9 +30,7 @@ import org.apache.pinot.spi.utils.ByteArray;
import org.roaringbitmap.RoaringBitmap;
import org.testng.annotations.Test;
-import static org.mockito.Mockito.doReturn;
-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.when;
+import static org.mockito.Mockito.*;
import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertFalse;
import static org.testng.Assert.assertTrue;
@@ -43,6 +41,30 @@ import static org.testng.Assert.assertTrue;
/// and edge cases such as empty bitmaps and the `isSortedColumn` flag.
public class CompactedColumnStatisticsTest {
+ // ======== Single-value dictionary ========
+
+ @Test
+ public void testSingleValueDictionarySkipsScan() {
+ MutableForwardIndex forwardIndex = mock(MutableForwardIndex.class);
+ when(forwardIndex.isSingleValue()).thenReturn(true);
+
+ Dictionary dictionary = mockIntDictionary(new int[]{42});
+ when(dictionary.length()).thenReturn(1);
+
+ RoaringBitmap validDocIds = RoaringBitmap.bitmapOf(0, 1, 2);
+ CompactedColumnStatistics stats =
+ new CompactedColumnStatistics(mockDataSource(forwardIndex,
dictionary), null, false, validDocIds);
+
+ assertEquals(stats.getMinValue(), 42);
+ assertEquals(stats.getMaxValue(), 42);
+ assertEquals((int[]) stats.getUniqueValuesSet(), new int[]{42});
+ assertEquals(stats.getCardinality(), 1);
+ assertTrue(stats.isSorted());
+ assertEquals(stats.getTotalNumberOfEntries(), 3);
+ // Every document maps to the only dict id, so the forward index is never
read
+ verify(forwardIndex, never()).getDictId(anyInt());
+ }
+
// ======== INT SV ========
@Test
diff --git
a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/realtime/converter/stats/MutableColumnStatisticsTest.java
b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/realtime/converter/stats/MutableColumnStatisticsTest.java
index 4663c0d83d1..dbb0fffc911 100644
---
a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/realtime/converter/stats/MutableColumnStatisticsTest.java
+++
b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/realtime/converter/stats/MutableColumnStatisticsTest.java
@@ -57,9 +57,7 @@ import org.testng.annotations.AfterClass;
import org.testng.annotations.DataProvider;
import org.testng.annotations.Test;
-import static org.mockito.Mockito.doReturn;
-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.when;
+import static org.mockito.Mockito.*;
import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertFalse;
import static org.testng.Assert.assertNotNull;
@@ -173,6 +171,31 @@ public class MutableColumnStatisticsTest implements
PinotBuffersAfterClassCheckR
assertTrue(stats.isFixedLength());
assertFalse(stats.isAscii());
assertTrue(stats.isSorted());
+ // A single distinct value is sorted by construction, so the forward
index is never read
+ verify(forwardIndex, never()).getDictId(anyInt());
+ }
+ }
+
+ // ======== isSorted caching ========
+
+ @Test
+ public void testIsSortedScansOnce()
+ throws Exception {
+ try (MutableDictionary dictionary = createMutableDictionary(DataType.INT,
"onHeap")) {
+ int dictId0 = dictionary.index(10);
+ int dictId1 = dictionary.index(20);
+ int dictId2 = dictionary.index(30);
+
+ int numDocs = 3;
+ MutableForwardIndex forwardIndex = mockSvForwardIndex(dictId0, dictId1,
dictId2);
+ DataSourceMetadata metadata = mockMetadata(DataType.INT, true, numDocs);
+
+ MutableColumnStatistics stats =
+ new MutableColumnStatistics(mockDataSource(metadata, forwardIndex,
dictionary), null, false);
+
+ assertTrue(stats.isSorted());
+ assertTrue(stats.isSorted());
+ verify(forwardIndex, times(numDocs)).getDictId(anyInt());
}
}
diff --git
a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/realtime/converter/stats/MutableNoDictColumnStatisticsTest.java
b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/realtime/converter/stats/MutableNoDictColumnStatisticsTest.java
index 276e5ec203f..7f5da00a68e 100644
---
a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/realtime/converter/stats/MutableNoDictColumnStatisticsTest.java
+++
b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/realtime/converter/stats/MutableNoDictColumnStatisticsTest.java
@@ -32,9 +32,7 @@ import org.testng.annotations.DataProvider;
import org.testng.annotations.Test;
import static org.apache.pinot.segment.spi.Constants.UNKNOWN_CARDINALITY;
-import static org.mockito.Mockito.doReturn;
-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.when;
+import static org.mockito.Mockito.*;
import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertFalse;
import static org.testng.Assert.assertNull;
@@ -89,6 +87,28 @@ public class MutableNoDictColumnStatisticsTest {
assertEquals(stats.getMaxRowLengthInBytes(), type.size());
}
+ // ======== isSorted caching ========
+
+ @Test
+ public void testIsSortedScansOnce() {
+ int numDocs = 3;
+ FieldSpec fieldSpec = new DimensionFieldSpec("col", DataType.INT, true);
+ Comparable[] values = fixedWidthValues(DataType.INT);
+
+ DataSourceMetadata metadata = mockMetadata(fieldSpec, numDocs);
+
+ MutableForwardIndex forwardIndex = mock(MutableForwardIndex.class);
+ when(forwardIndex.isSingleValue()).thenReturn(true);
+ stubForwardIndexReads(forwardIndex, DataType.INT, values);
+
+ MutableNoDictColumnStatistics stats =
+ new MutableNoDictColumnStatistics(mockNoDictDataSource(metadata,
forwardIndex), null, false);
+
+ assertTrue(stats.isSorted());
+ assertTrue(stats.isSorted());
+ verify(forwardIndex, times(numDocs)).getInt(anyInt());
+ }
+
// ======== BigDecimal SV ========
@Test
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
