You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Jackie-Jiang (via GitHub)" <gi...@apache.org> on 2026/03/25 01:52:36 UTC
[PR] Allow sorting on raw (no-dictionary) column [pinot]
Jackie-Jiang opened a new pull request, #17965:
URL: https://github.com/apache/pinot/pull/17965
## Summary
- `RealtimeSegmentConfig.Builder` no longer forces an inverted index on the sorted column when the column is configured as no-dictionary (raw), allowing raw sorted columns to be created correctly
- `MutableSegmentImpl.getSortedDocIdIterationOrderWithSortedColumn` is split into two helpers: one for the dictionary+inverted-index path and one for the raw forward-index path that sorts doc IDs by reading raw values directly
- `MutableNoDictionaryColStatistics.isSorted()` now returns `true` for the designated sorted column (short-circuit) instead of hardcoded `false`, and also supports a full scan for non-sorted raw columns
- `MutableColumnStatistics.isSorted()` short-circuits to `true` when the column is the designated sorted column
- `isSortedColumn` flag threaded through the stats pipeline: `RealtimeSegmentConverter` → `RealtimeSegmentSegmentCreationDataSource` → `RealtimeSegmentStatsContainer` → per-column stats classes
## Test plan
- `RealtimeSegmentConverterTest#testSortedColumnMetadata` — verifies sorted doc ID order (mutable) and `ColumnMetadata.isSorted()` (immutable) for both dictionary-encoded and no-dictionary sorted columns, with rows inserted in reverse order
- `SegmentGenerationWithSortedColumnTest#testIsSortedMetadataForSortedColumn` — verifies `isSorted` metadata via the direct immutable segment creation path (`SegmentIndexCreationDriverImpl`) for both encodings
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Allow sorting on raw (no-dictionary) column [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #17965:
URL: https://github.com/apache/pinot/pull/17965#issuecomment-4143645776
Documentation PR: https://github.com/pinot-contrib/pinot-docs/pull/583
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Allow sorting on raw (no-dictionary) column [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #17965:
URL: https://github.com/apache/pinot/pull/17965#discussion_r2985285601
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/converter/stats/RealtimeSegmentStatsContainer.java:
##########
@@ -91,12 +80,14 @@ public RealtimeSegmentStatsContainer(MutableSegment mutableSegment, @Nullable in
// Handle dictionary columns
if (dataSource.getDictionary() != null) {
_columnStatisticsMap.put(columnName,
- createDictionaryColumnStatistics(dataSource, sortedDocIds, isUsingCompactedReader, validDocIdsSnapshot));
+ createDictionaryColumnStatistics(dataSource, sortedDocIds, isSortedColumn, isUsingCompactedReader,
+ validDocIdsSnapshot));
continue;
}
// Handle no dictionary columns
- _columnStatisticsMap.put(columnName, new MutableNoDictionaryColStatistics(dataSource));
+ _columnStatisticsMap.put(columnName,
+ new MutableNoDictionaryColStatistics(dataSource, sortedDocIds, isSortedColumn));
Review Comment:
When using a `CompactedPinotSegmentRecordReader`, stats for dictionary/map columns are computed on `validDocIdsSnapshot`, but no-dictionary columns still use `MutableNoDictionaryColStatistics` without `validDocIdsSnapshot`. With the new full-scan `isSorted()` implementation, this can (a) scan deleted/invalid docs unnecessarily and (b) compute sortedness against docs that will not be written to the compacted segment. Consider threading `validDocIdsSnapshot` into `MutableNoDictionaryColStatistics` (similar to the compacted dictionary stats) and iterating only valid docs (in `sortedDocIds` order when provided).
```suggestion
if (isUsingCompactedReader) {
_columnStatisticsMap.put(columnName,
new MutableNoDictionaryColStatistics(dataSource, sortedDocIds, isSortedColumn, validDocIdsSnapshot));
} else {
_columnStatisticsMap.put(columnName,
new MutableNoDictionaryColStatistics(dataSource, sortedDocIds, isSortedColumn));
}
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java:
##########
@@ -1386,6 +1399,50 @@ public int[] getSortedDocIdIterationOrderWithSortedColumn(String column) {
return docIds;
}
+ /// Returns sorted docIds for a no-dictionary (raw) sorted column by reading raw values directly from the forward
+ /// index and sorting by them.
+ private int[] getSortedDocIdsWithRawForwardIndex(String column, IndexContainer indexContainer) {
+ MutableForwardIndex forwardIndex =
+ (MutableForwardIndex) indexContainer._mutableIndexes.get(StandardIndexes.forward());
+ int numDocsIndexed = _numDocsIndexed;
+ int[] docIds = new int[numDocsIndexed];
+ for (int i = 0; i < numDocsIndexed; i++) {
+ docIds[i] = i;
+ }
+
+ DataType storedType = indexContainer._fieldSpec.getDataType().getStoredType();
+ switch (storedType) {
+ case INT:
+ IntArrays.quickSort(docIds, (d1, d2) -> Integer.compare(forwardIndex.getInt(d1), forwardIndex.getInt(d2)));
+ break;
+ case LONG:
+ IntArrays.quickSort(docIds, (d1, d2) -> Long.compare(forwardIndex.getLong(d1), forwardIndex.getLong(d2)));
+ break;
+ case FLOAT:
+ IntArrays.quickSort(docIds, (d1, d2) -> Float.compare(forwardIndex.getFloat(d1), forwardIndex.getFloat(d2)));
+ break;
+ case DOUBLE:
+ IntArrays.quickSort(docIds, (d1, d2) -> Double.compare(forwardIndex.getDouble(d1), forwardIndex.getDouble(d2)));
+ break;
+ case BIG_DECIMAL:
+ IntArrays.quickSort(docIds,
+ (d1, d2) -> forwardIndex.getBigDecimal(d1).compareTo(forwardIndex.getBigDecimal(d2)));
+ break;
+ case STRING:
+ IntArrays.quickSort(docIds, (d1, d2) -> forwardIndex.getString(d1).compareTo(forwardIndex.getString(d2)));
+ break;
+ case BYTES:
+ IntArrays.quickSort(docIds,
+ (d1, d2) -> new ByteArray(forwardIndex.getBytes(d1)).compareTo(new ByteArray(forwardIndex.getBytes(d2))));
+ break;
Review Comment:
`getSortedDocIdsWithRawForwardIndex` sorts docIds using a comparator that repeatedly calls `forwardIndex.get*()` during `quickSort`. For large consuming segments this will perform many random reads (and allocations for object types), making conversion significantly slower than necessary. Consider pre-reading values into an array (or using type-specific indirect sort with cached values) to avoid repeated forward-index reads during comparisons.
```suggestion
case INT: {
int[] values = new int[numDocsIndexed];
for (int i = 0; i < numDocsIndexed; i++) {
values[i] = forwardIndex.getInt(i);
}
IntArrays.quickSort(docIds, (d1, d2) -> Integer.compare(values[d1], values[d2]));
break;
}
case LONG: {
long[] values = new long[numDocsIndexed];
for (int i = 0; i < numDocsIndexed; i++) {
values[i] = forwardIndex.getLong(i);
}
IntArrays.quickSort(docIds, (d1, d2) -> Long.compare(values[d1], values[d2]));
break;
}
case FLOAT: {
float[] values = new float[numDocsIndexed];
for (int i = 0; i < numDocsIndexed; i++) {
values[i] = forwardIndex.getFloat(i);
}
IntArrays.quickSort(docIds, (d1, d2) -> Float.compare(values[d1], values[d2]));
break;
}
case DOUBLE: {
double[] values = new double[numDocsIndexed];
for (int i = 0; i < numDocsIndexed; i++) {
values[i] = forwardIndex.getDouble(i);
}
IntArrays.quickSort(docIds, (d1, d2) -> Double.compare(values[d1], values[d2]));
break;
}
case BIG_DECIMAL: {
java.math.BigDecimal[] values = new java.math.BigDecimal[numDocsIndexed];
for (int i = 0; i < numDocsIndexed; i++) {
values[i] = forwardIndex.getBigDecimal(i);
}
IntArrays.quickSort(docIds, (d1, d2) -> values[d1].compareTo(values[d2]));
break;
}
case STRING: {
String[] values = new String[numDocsIndexed];
for (int i = 0; i < numDocsIndexed; i++) {
values[i] = forwardIndex.getString(i);
}
IntArrays.quickSort(docIds, (d1, d2) -> values[d1].compareTo(values[d2]));
break;
}
case BYTES: {
ByteArray[] values = new ByteArray[numDocsIndexed];
for (int i = 0; i < numDocsIndexed; i++) {
values[i] = new ByteArray(forwardIndex.getBytes(i));
}
IntArrays.quickSort(docIds, (d1, d2) -> values[d1].compareTo(values[d2]));
break;
}
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java:
##########
@@ -1386,6 +1399,50 @@ public int[] getSortedDocIdIterationOrderWithSortedColumn(String column) {
return docIds;
}
+ /// Returns sorted docIds for a no-dictionary (raw) sorted column by reading raw values directly from the forward
+ /// index and sorting by them.
+ private int[] getSortedDocIdsWithRawForwardIndex(String column, IndexContainer indexContainer) {
+ MutableForwardIndex forwardIndex =
+ (MutableForwardIndex) indexContainer._mutableIndexes.get(StandardIndexes.forward());
+ int numDocsIndexed = _numDocsIndexed;
+ int[] docIds = new int[numDocsIndexed];
+ for (int i = 0; i < numDocsIndexed; i++) {
+ docIds[i] = i;
+ }
+
+ DataType storedType = indexContainer._fieldSpec.getDataType().getStoredType();
+ switch (storedType) {
+ case INT:
+ IntArrays.quickSort(docIds, (d1, d2) -> Integer.compare(forwardIndex.getInt(d1), forwardIndex.getInt(d2)));
+ break;
+ case LONG:
+ IntArrays.quickSort(docIds, (d1, d2) -> Long.compare(forwardIndex.getLong(d1), forwardIndex.getLong(d2)));
+ break;
+ case FLOAT:
+ IntArrays.quickSort(docIds, (d1, d2) -> Float.compare(forwardIndex.getFloat(d1), forwardIndex.getFloat(d2)));
+ break;
+ case DOUBLE:
+ IntArrays.quickSort(docIds, (d1, d2) -> Double.compare(forwardIndex.getDouble(d1), forwardIndex.getDouble(d2)));
+ break;
+ case BIG_DECIMAL:
+ IntArrays.quickSort(docIds,
+ (d1, d2) -> forwardIndex.getBigDecimal(d1).compareTo(forwardIndex.getBigDecimal(d2)));
+ break;
+ case STRING:
+ IntArrays.quickSort(docIds, (d1, d2) -> forwardIndex.getString(d1).compareTo(forwardIndex.getString(d2)));
+ break;
+ case BYTES:
+ IntArrays.quickSort(docIds,
+ (d1, d2) -> new ByteArray(forwardIndex.getBytes(d1)).compareTo(new ByteArray(forwardIndex.getBytes(d2))));
Review Comment:
In the BYTES case, the comparator allocates new `ByteArray` instances for every comparison (`new ByteArray(forwardIndex.getBytes(...))`), which can create substantial GC pressure during segment conversion. Consider comparing byte[] values without per-compare allocations (e.g., cache wrapped objects per docId or use a dedicated byte[] comparator).
```suggestion
ByteArray[] byteValues = new ByteArray[numDocsIndexed];
for (int i = 0; i < numDocsIndexed; i++) {
byteValues[i] = new ByteArray(forwardIndex.getBytes(i));
}
IntArrays.quickSort(docIds, (d1, d2) -> byteValues[d1].compareTo(byteValues[d2]));
```
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Allow sorting on raw (no-dictionary) column [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #17965:
URL: https://github.com/apache/pinot/pull/17965#issuecomment-4130163095
> Pinot's immutable-segment sorter still hard-fails on them (Unsupported sorted no-dictionary column in PinotSegmentSorter). SegmentIndexCreationDriverImpl still routes FileFormat.PINOT inputs through PinotSegmentRecordReader(..., columnSortOrder), so segment-processing / minion flows that need to re-sort Pinot input remain broken for segments produced by this feature.
This is out of the scope of this change. Let's create an issue and address it separately
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Allow sorting on raw (no-dictionary) column [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #17965:
URL: https://github.com/apache/pinot/pull/17965
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Allow sorting on raw (no-dictionary) column [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #17965:
URL: https://github.com/apache/pinot/pull/17965#issuecomment-4122736557
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/17965?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `119 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.46%. Comparing base ([`43c8ecc`](https://app.codecov.io/gh/apache/pinot/commit/43c8ecccffdadf6a2b31b3383f4462ca0f7e2994?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`262143a`](https://app.codecov.io/gh/apache/pinot/commit/262143a67c7b33390d6cd0c33fb997121f9707c2?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/17965?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...verter/stats/MutableNoDictionaryColStatistics.java](https://app.codecov.io/gh/apache/pinot/pull/17965?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Frealtime%2Fconverter%2Fstats%2FMutableNoDictionaryColStatistics.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9jb252ZXJ0ZXIvc3RhdHMvTXV0YWJsZU5vRGljdGlvbmFyeUNvbFN0YXRpc3RpY3MuamF2YQ==) | 0.00% | [63 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17965?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...local/indexsegment/mutable/MutableSegmentImpl.java](https://app.codecov.io/gh/apache/pinot/pull/17965?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Findexsegment%2Fmutable%2FMutableSegmentImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9pbmRleHNlZ21lbnQvbXV0YWJsZS9NdXRhYmxlU2VnbWVudEltcGwuamF2YQ==) | 0.00% | [29 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17965?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ent/local/realtime/impl/RealtimeSegmentConfig.java](https://app.codecov.io/gh/apache/pinot/pull/17965?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Frealtime%2Fimpl%2FRealtimeSegmentConfig.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9pbXBsL1JlYWx0aW1lU2VnbWVudENvbmZpZy5qYXZh) | 0.00% | [7 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17965?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...converter/stats/RealtimeSegmentStatsContainer.java](https://app.codecov.io/gh/apache/pinot/pull/17965?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Frealtime%2Fconverter%2Fstats%2FRealtimeSegmentStatsContainer.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9jb252ZXJ0ZXIvc3RhdHMvUmVhbHRpbWVTZWdtZW50U3RhdHNDb250YWluZXIuamF2YQ==) | 0.00% | [6 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17965?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...tats/RealtimeSegmentSegmentCreationDataSource.java](https://app.codecov.io/gh/apache/pinot/pull/17965?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Frealtime%2Fconverter%2Fstats%2FRealtimeSegmentSegmentCreationDataSource.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9jb252ZXJ0ZXIvc3RhdHMvUmVhbHRpbWVTZWdtZW50U2VnbWVudENyZWF0aW9uRGF0YVNvdXJjZS5qYXZh) | 0.00% | [5 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17965?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ltime/converter/stats/MutableColumnStatistics.java](https://app.codecov.io/gh/apache/pinot/pull/17965?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Frealtime%2Fconverter%2Fstats%2FMutableColumnStatistics.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9jb252ZXJ0ZXIvc3RhdHMvTXV0YWJsZUNvbHVtblN0YXRpc3RpY3MuamF2YQ==) | 0.00% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17965?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...l/realtime/converter/RealtimeSegmentConverter.java](https://app.codecov.io/gh/apache/pinot/pull/17965?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Frealtime%2Fconverter%2FRealtimeSegmentConverter.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9jb252ZXJ0ZXIvUmVhbHRpbWVTZWdtZW50Q29udmVydGVyLmphdmE=) | 0.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17965?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...er/stats/CompactedDictEncodedColumnStatistics.java](https://app.codecov.io/gh/apache/pinot/pull/17965?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Frealtime%2Fconverter%2Fstats%2FCompactedDictEncodedColumnStatistics.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9jb252ZXJ0ZXIvc3RhdHMvQ29tcGFjdGVkRGljdEVuY29kZWRDb2x1bW5TdGF0aXN0aWNzLmphdmE=) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17965?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...r/stats/CompactedRawIndexDictColumnStatistics.java](https://app.codecov.io/gh/apache/pinot/pull/17965?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Frealtime%2Fconverter%2Fstats%2FCompactedRawIndexDictColumnStatistics.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9jb252ZXJ0ZXIvc3RhdHMvQ29tcGFjdGVkUmF3SW5kZXhEaWN0Q29sdW1uU3RhdGlzdGljcy5qYXZh) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17965?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (43c8ecc) and HEAD (262143a). Click for more details.
>
> <details><summary>HEAD has 36 uploads less than BASE</summary>
>
>| Flag | BASE (43c8ecc) | HEAD (262143a) |
>|------|------|------|
>|java-21|5|1|
>|unittests1|2|1|
>|unittests|4|1|
>|temurin|10|1|
>|java-11|5|0|
>|unittests2|2|0|
>|integration|6|0|
>|integration2|2|0|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #17965 +/- ##
============================================
- Coverage 63.29% 55.46% -7.83%
+ Complexity 1525 729 -796
============================================
Files 3194 2496 -698
Lines 193645 142685 -50960
Branches 29787 22878 -6909
============================================
- Hits 122559 79146 -43413
+ Misses 61466 56801 -4665
+ Partials 9620 6738 -2882
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/17965/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/17965/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/17965/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/17965/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/17965/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/17965/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/17965/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.46% <0.00%> (-7.77%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/17965/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.46% <0.00%> (-7.83%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/17965/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.46% <0.00%> (-7.82%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/17965/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.46% <0.00%> (-0.09%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/17965/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/17965?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Allow sorting on raw (no-dictionary) column [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #17965:
URL: https://github.com/apache/pinot/pull/17965#issuecomment-4136252421
Documentation PR created for this enhancement: https://github.com/pinot-contrib/pinot-docs/pull/579
The docs have been updated to document that sorted raw columns are now properly supported without forcing an inverted index.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Allow sorting on raw (no-dictionary) column [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #17965:
URL: https://github.com/apache/pinot/pull/17965#discussion_r2990730573
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/converter/stats/MutableNoDictionaryColStatistics.java:
##########
@@ -76,7 +86,188 @@ public int getLengthOfLargestElement() {
@Override
public boolean isSorted() {
- return false;
+ // Sorted column is guaranteed to be sorted by construction — no scan needed
+ if (_isSortedColumn) {
+ return true;
+ }
+
+ // Multi-valued column cannot be sorted
+ if (!_dataSourceMetadata.isSingleValue()) {
+ return false;
+ }
+
+ int numDocs = _dataSourceMetadata.getNumDocs();
+ if (numDocs <= 1) {
+ return true;
+ }
+
+ // Verify that values are non-decreasing when iterated in the given order
+ DataType storedType = _dataSourceMetadata.getFieldSpec().getDataType().getStoredType();
+ if (_sortedDocIds != null) {
+ switch (storedType) {
+ case INT: {
+ int prev = _forwardIndex.getInt(_sortedDocIds[0]);
+ for (int i = 1; i < numDocs; i++) {
+ int curr = _forwardIndex.getInt(_sortedDocIds[i]);
+ if (curr < prev) {
+ return false;
+ }
+ prev = curr;
+ }
+ return true;
+ }
+ case LONG: {
+ long prev = _forwardIndex.getLong(_sortedDocIds[0]);
+ for (int i = 1; i < numDocs; i++) {
+ long curr = _forwardIndex.getLong(_sortedDocIds[i]);
+ if (curr < prev) {
+ return false;
+ }
+ prev = curr;
+ }
+ return true;
+ }
+ case FLOAT: {
+ float prev = _forwardIndex.getFloat(_sortedDocIds[0]);
+ for (int i = 1; i < numDocs; i++) {
+ float curr = _forwardIndex.getFloat(_sortedDocIds[i]);
+ if (curr < prev) {
+ return false;
+ }
+ prev = curr;
+ }
+ return true;
+ }
+ case DOUBLE: {
+ double prev = _forwardIndex.getDouble(_sortedDocIds[0]);
+ for (int i = 1; i < numDocs; i++) {
+ double curr = _forwardIndex.getDouble(_sortedDocIds[i]);
+ if (curr < prev) {
+ return false;
+ }
+ prev = curr;
+ }
+ return true;
+ }
+ case BIG_DECIMAL: {
+ BigDecimal prev = _forwardIndex.getBigDecimal(_sortedDocIds[0]);
+ for (int i = 1; i < numDocs; i++) {
+ BigDecimal curr = _forwardIndex.getBigDecimal(_sortedDocIds[i]);
+ if (curr.compareTo(prev) < 0) {
+ return false;
+ }
+ prev = curr;
+ }
+ return true;
+ }
+ case STRING: {
+ String prev = _forwardIndex.getString(_sortedDocIds[0]);
+ for (int i = 1; i < numDocs; i++) {
+ String curr = _forwardIndex.getString(_sortedDocIds[i]);
+ if (curr.compareTo(prev) < 0) {
+ return false;
+ }
+ prev = curr;
+ }
+ return true;
+ }
+ case BYTES: {
+ byte[] prev = _forwardIndex.getBytes(_sortedDocIds[0]);
+ for (int i = 1; i < numDocs; i++) {
+ byte[] curr = _forwardIndex.getBytes(_sortedDocIds[i]);
+ if (ByteArray.compare(prev, curr) < 0) {
Review Comment:
this should be ByteArray.compare(curr, prev)?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Allow sorting on raw (no-dictionary) column [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #17965:
URL: https://github.com/apache/pinot/pull/17965#discussion_r2991381417
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/converter/stats/RealtimeSegmentStatsContainer.java:
##########
@@ -91,12 +80,14 @@ public RealtimeSegmentStatsContainer(MutableSegment mutableSegment, @Nullable in
// Handle dictionary columns
if (dataSource.getDictionary() != null) {
_columnStatisticsMap.put(columnName,
- createDictionaryColumnStatistics(dataSource, sortedDocIds, isUsingCompactedReader, validDocIdsSnapshot));
+ createDictionaryColumnStatistics(dataSource, sortedDocIds, isSortedColumn, isUsingCompactedReader,
+ validDocIdsSnapshot));
continue;
}
// Handle no dictionary columns
- _columnStatisticsMap.put(columnName, new MutableNoDictionaryColStatistics(dataSource));
+ _columnStatisticsMap.put(columnName,
+ new MutableNoDictionaryColStatistics(dataSource, sortedDocIds, isSortedColumn));
}
Review Comment:
In the commit-time compaction path, `validDocIdsSnapshot` is used to compute `_totalDocCount`, but no-dictionary columns still build stats (including `isSorted()` scans) over *all* docIds via `sortedDocIds`/`DataSourceMetadata.getNumDocs()`. This can incorrectly mark a column as unsorted even if the compacted output rows (valid docs only, in recordReader iteration order) are sorted. Consider passing a filtered docId iteration order (e.g., sorted valid docIds) and/or `validDocIdsSnapshot` into `MutableNoDictionaryColStatistics` so `isSorted()` and other scans operate on the same rows that will be written to the output segment.
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/realtime/converter/RealtimeSegmentConverterTest.java:
##########
@@ -977,6 +976,287 @@ public void testFnvPartitionFunctionConfigPreservedInConvertedSegment()
}
}
+ @DataProvider
+ public static Object[][] sortedColumnParams() {
+ FieldSpec.DataType[] dataTypes = {
+ FieldSpec.DataType.INT,
+ FieldSpec.DataType.LONG,
+ FieldSpec.DataType.FLOAT,
+ FieldSpec.DataType.DOUBLE,
+ FieldSpec.DataType.BIG_DECIMAL,
+ FieldSpec.DataType.BOOLEAN,
+ FieldSpec.DataType.TIMESTAMP,
+ FieldSpec.DataType.STRING,
+ FieldSpec.DataType.BYTES,
+ };
+ List<Object[]> params = new ArrayList<>();
+ for (FieldSpec.DataType dataType : dataTypes) {
+ params.add(new Object[]{dataType, true}); // dictionary-encoded
+ params.add(new Object[]{dataType, false}); // no-dictionary
+ }
+ return params.toArray(new Object[0][]);
+ }
+
+ /// Verifies that the isSorted metadata flag is correctly set on both the mutable segment (via sorted doc ID order)
+ /// and the converted immutable segment (via ColumnMetadata) for all supported single-value data types, both
+ /// dictionary-encoded and no-dictionary.
+ @Test(dataProvider = "sortedColumnParams")
+ public void testSortedColumnMetadata(FieldSpec.DataType dataType, boolean dictionaryEncoded)
+ throws Exception {
+ File tmpDir = new File(TMP_DIR, "tmp_sorted_" + dataType + "_" + dictionaryEncoded + "_" + System.nanoTime());
+ String sortedColumn = "sorted_col";
+ String dupColumn = "sorted_col2";
+
+ List<String> noDictColumns = dictionaryEncoded ? List.of() : List.of(sortedColumn, dupColumn);
+ TableConfig tableConfig = new TableConfigBuilder(TableType.REALTIME)
+ .setTableName("testTable")
+ .setTimeColumnName(DATE_TIME_COLUMN)
+ .setSortedColumn(sortedColumn)
+ .setNoDictionaryColumns(noDictColumns)
+ .setColumnMajorSegmentBuilderEnabled(false)
+ .build();
+ Schema schema = new Schema.SchemaBuilder()
+ .addSingleValueDimension(sortedColumn, dataType)
+ .addSingleValueDimension(dupColumn, dataType)
+ .addSingleValueDimension(STRING_COLUMN1, FieldSpec.DataType.STRING)
+ .addDateTime(DATE_TIME_COLUMN, FieldSpec.DataType.LONG, "1:MILLISECONDS:EPOCH", "1:MILLISECONDS")
+ .build();
+
+ String tableNameWithType = tableConfig.getTableName();
+ String segmentName = "testTable__0__0__123456";
+
+ // Use Builder(tableConfig, schema) so the inverted index is automatically added for the
+ // dictionary-encoded sorted column (required by getSortedDocIdsWithInvertedIndex).
+ RealtimeSegmentConfig.Builder configBuilder = new RealtimeSegmentConfig.Builder(tableConfig, schema)
+ .setTableNameWithType(tableNameWithType)
+ .setSegmentName(segmentName)
+ .setStreamName(tableNameWithType)
+ .setSchema(schema)
+ .setTimeColumnName(DATE_TIME_COLUMN)
+ .setCapacity(1000)
+ .setSegmentZKMetadata(getSegmentZKMetadata(segmentName))
+ .setOffHeap(true)
+ .setMemoryManager(new DirectMemoryManager(segmentName))
+ .setStatsHistory(RealtimeSegmentStatsHistory.deserializeFrom(new File(tmpDir, "stats")))
+ .setConsumerDir(new File(tmpDir, "consumerDir").getAbsolutePath());
+
+ MutableSegmentImpl mutableSegment = new MutableSegmentImpl(configBuilder.build(), null);
+ try {
+ // Insert rows in REVERSE sorted order to confirm conversion actually reorders them.
+ // dupColumn carries the same values as sortedColumn — it must also be detected as sorted.
+ // STRING_COLUMN1 values are intentionally correlated inversely with the sorted column so that when rows are
+ // sorted ascending, STRING_COLUMN1 ends up descending (i.e. not sorted).
+ int numRows = 10;
+ for (int i = numRows - 1; i >= 0; i--) {
+ GenericRow row = new GenericRow();
+ Object sortedValue = sortedValue(dataType, i, numRows);
+ row.putValue(sortedColumn, sortedValue);
+ row.putValue(dupColumn, sortedValue);
+ row.putValue(STRING_COLUMN1, "str" + (numRows - 1 - i));
+ row.putValue(DATE_TIME_COLUMN, 1697814309L + i);
+ mutableSegment.index(row, null);
+ }
+
+ // -- Mutable segment: getSortedDocIdIterationOrderWithSortedColumn must return doc IDs in non-decreasing
+ // value order regardless of encoding --
+ int[] sortedDocIds = mutableSegment.getSortedDocIdIterationOrderWithSortedColumn(sortedColumn);
+ assertEquals(sortedDocIds.length, numRows);
+ FieldSpec.DataType storedType = dataType.getStoredType();
+ Object prevValue = null;
+ for (int docId : sortedDocIds) {
+ Object currValue = mutableSegment.getValue(docId, sortedColumn);
+ if (prevValue != null) {
+ assertTrue(compareValues(storedType, currValue, prevValue) >= 0,
+ "Mutable segment: " + sortedColumn + " values must be non-decreasing in sorted doc ID order"
+ + " (dataType=" + dataType + ", dictionaryEncoded=" + dictionaryEncoded + ")");
+ }
+ prevValue = currValue;
+ }
+
+ // -- Immutable segment: ColumnMetadata.isSorted() must be true for the sorted column --
+ File outputDir = new File(tmpDir, "outputDir");
+ SegmentZKPropsConfig segmentZKPropsConfig = new SegmentZKPropsConfig();
+ segmentZKPropsConfig.setStartOffset("1");
+ segmentZKPropsConfig.setEndOffset("100");
+ RealtimeSegmentConverter converter =
+ new RealtimeSegmentConverter(mutableSegment, segmentZKPropsConfig, outputDir.getAbsolutePath(), schema,
+ tableNameWithType, tableConfig, segmentName, false);
+ converter.build(SegmentVersion.v3, null);
+
+ SegmentMetadataImpl segmentMetadata = new SegmentMetadataImpl(new File(outputDir, segmentName));
+
+ ColumnMetadata sortedColMeta = segmentMetadata.getColumnMetadataFor(sortedColumn);
+ assertTrue(sortedColMeta.isSorted(),
+ "Immutable segment: sorted column must have isSorted=true (dataType=" + dataType + ", dictionaryEncoded="
+ + dictionaryEncoded + ")");
+ assertEquals(sortedColMeta.hasDictionary(), dictionaryEncoded,
+ "Immutable segment: dictionary encoding must match config (dataType=" + dataType + ", dictionaryEncoded="
+ + dictionaryEncoded + ")");
+
+ // A non-sorted column must not be marked as sorted
+ ColumnMetadata nonSortedColMeta = segmentMetadata.getColumnMetadataFor(STRING_COLUMN1);
+ assertFalse(nonSortedColMeta.isSorted(), "Immutable segment: non-sorted column must have isSorted=false");
+
+ // Duplicate column with same values must also be detected as sorted
+ ColumnMetadata dupColMeta = segmentMetadata.getColumnMetadataFor(dupColumn);
+ assertTrue(dupColMeta.isSorted(),
+ "Immutable segment: duplicate column with same values must also have isSorted=true (dataType=" + dataType
+ + ", dictionaryEncoded=" + dictionaryEncoded + ")");
+ assertEquals(dupColMeta.hasDictionary(), dictionaryEncoded,
+ "Immutable segment: dictionary encoding must match config for duplicate column (dataType=" + dataType
+ + ", dictionaryEncoded=" + dictionaryEncoded + ")");
+
+ // Verify the physical row order in the converted segment: values must be non-decreasing
+ File indexDir = new File(outputDir, segmentName);
+ SegmentLocalFSDirectory segmentDir = new SegmentLocalFSDirectory(indexDir, segmentMetadata, ReadMode.mmap);
+ SegmentDirectory.Reader segmentReader = segmentDir.createReader();
+ Map<String, ColumnIndexContainer> indexContainerMap = new HashMap<>();
+ IndexLoadingConfig indexLoadingConfig = new IndexLoadingConfig(null, tableConfig);
+ for (Map.Entry<String, ColumnMetadata> entry : segmentMetadata.getColumnMetadataMap().entrySet()) {
+ indexContainerMap.put(entry.getKey(),
+ new PhysicalColumnIndexContainer(segmentReader, entry.getValue(), indexLoadingConfig));
+ }
+ ImmutableSegmentImpl immutableSegment =
+ new ImmutableSegmentImpl(segmentDir, segmentMetadata, indexContainerMap, null);
+ try {
+ prevValue = null;
+ Object prevDupValue = null;
+ for (int docId = 0; docId < numRows; docId++) {
+ Object currValue = immutableSegment.getValue(docId, sortedColumn);
+ Object currDupValue = immutableSegment.getValue(docId, dupColumn);
+ if (prevValue != null) {
+ assertTrue(compareValues(storedType, currValue, prevValue) >= 0,
+ "Immutable segment rows must be in non-decreasing " + sortedColumn + " order at docId=" + docId
+ + " (dataType=" + dataType + ", dictionaryEncoded=" + dictionaryEncoded + ")");
+ assertTrue(compareValues(storedType, currDupValue, prevDupValue) >= 0,
+ "Immutable segment rows must be in non-decreasing " + dupColumn + " order at docId=" + docId
+ + " (dataType=" + dataType + ", dictionaryEncoded=" + dictionaryEncoded + ")");
+ }
+ prevValue = currValue;
+ prevDupValue = currDupValue;
+ }
+ } finally {
+ immutableSegment.destroy();
+ }
+ } finally {
+ mutableSegment.destroy();
+ }
+ }
+
+ /// Verifies that when records are inserted already in sorted order on a column that is NOT configured as the
+ /// sorted column, the converted immutable segment still detects isSorted=true for that column.
+ @Test(dataProvider = "sortedColumnParams")
+ public void testPreSortedRecordsWithoutSortedColumn(FieldSpec.DataType dataType, boolean dictionaryEncoded)
+ throws Exception {
+ File tmpDir =
+ new File(TMP_DIR, "tmp_presorted_" + dataType + "_" + dictionaryEncoded + "_" + System.nanoTime());
+ String preSortedColumn = "sorted_col";
+
+ List<String> noDictColumns = dictionaryEncoded ? List.of() : List.of(preSortedColumn);
+ TableConfig tableConfig = new TableConfigBuilder(TableType.REALTIME)
+ .setTableName("testTable")
+ .setTimeColumnName(DATE_TIME_COLUMN)
+ .setNoDictionaryColumns(noDictColumns)
+ .setColumnMajorSegmentBuilderEnabled(false)
+ .build();
+ Schema schema = new Schema.SchemaBuilder()
+ .addSingleValueDimension(preSortedColumn, dataType)
+ .addSingleValueDimension(STRING_COLUMN1, FieldSpec.DataType.STRING)
+ .addDateTime(DATE_TIME_COLUMN, FieldSpec.DataType.LONG, "1:MILLISECONDS:EPOCH", "1:MILLISECONDS")
+ .build();
+
+ String tableNameWithType = tableConfig.getTableName();
+ String segmentName = "testTable__0__0__123456";
+
+ RealtimeSegmentConfig.Builder configBuilder = new RealtimeSegmentConfig.Builder(tableConfig, schema)
+ .setTableNameWithType(tableNameWithType)
+ .setSegmentName(segmentName)
+ .setStreamName(tableNameWithType)
+ .setSchema(schema)
+ .setTimeColumnName(DATE_TIME_COLUMN)
+ .setCapacity(1000)
+ .setSegmentZKMetadata(getSegmentZKMetadata(segmentName))
+ .setOffHeap(true)
+ .setMemoryManager(new DirectMemoryManager(segmentName))
+ .setStatsHistory(RealtimeSegmentStatsHistory.deserializeFrom(new File(tmpDir, "stats")))
+ .setConsumerDir(new File(tmpDir, "consumerDir").getAbsolutePath());
+
+ MutableSegmentImpl mutableSegment = new MutableSegmentImpl(configBuilder.build(), null);
+ try {
+ // Insert rows in ascending order on preSortedColumn.
+ // STRING_COLUMN1 is descending so it is not sorted.
+ int numRows = 10;
+ for (int i = 0; i < numRows; i++) {
+ GenericRow row = new GenericRow();
+ row.putValue(preSortedColumn, sortedValue(dataType, i, numRows));
+ row.putValue(STRING_COLUMN1, "str" + (numRows - 1 - i));
+ row.putValue(DATE_TIME_COLUMN, 1697814309L + i);
+ mutableSegment.index(row, null);
+ }
+
+ File outputDir = new File(tmpDir, "outputDir");
+ SegmentZKPropsConfig segmentZKPropsConfig = new SegmentZKPropsConfig();
+ segmentZKPropsConfig.setStartOffset("1");
+ segmentZKPropsConfig.setEndOffset("100");
+ RealtimeSegmentConverter converter =
+ new RealtimeSegmentConverter(mutableSegment, segmentZKPropsConfig, outputDir.getAbsolutePath(), schema,
+ tableNameWithType, tableConfig, segmentName, false);
+ converter.build(SegmentVersion.v3, null);
+
+ SegmentMetadataImpl segmentMetadata = new SegmentMetadataImpl(new File(outputDir, segmentName));
+
+ ColumnMetadata preSortedColMeta = segmentMetadata.getColumnMetadataFor(preSortedColumn);
+ assertTrue(preSortedColMeta.isSorted(),
+ "Pre-sorted column must have isSorted=true even without sortedColumn config (dataType=" + dataType
+ + ", dictionaryEncoded=" + dictionaryEncoded + ")");
+ assertEquals(preSortedColMeta.hasDictionary(), dictionaryEncoded,
+ "Dictionary encoding must match config (dataType=" + dataType + ", dictionaryEncoded=" + dictionaryEncoded
+ + ")");
+
+ ColumnMetadata nonSortedColMeta = segmentMetadata.getColumnMetadataFor(STRING_COLUMN1);
+ assertFalse(nonSortedColMeta.isSorted(), "Non-sorted column must have isSorted=false");
+ } finally {
+ mutableSegment.destroy();
+ }
+ }
+
+ /// Returns the i-th ascending value for the given data type, out of numRows total rows.
+ private static Object sortedValue(FieldSpec.DataType dataType, int i, int numRows) {
+ switch (dataType) {
+ case INT:
+ return i;
+ case LONG:
+ return (long) i;
+ case FLOAT:
+ return (float) i;
+ case DOUBLE:
+ return (double) i;
+ case BIG_DECIMAL:
+ return new BigDecimal(i);
+ case BOOLEAN:
+ // First half false (0), second half true (1) — yields a non-decreasing sequence
+ return i >= numRows / 2 ? 1 : 0;
+ case TIMESTAMP:
+ return (long) i * 1000L;
+ case STRING:
+ // Zero-padded to preserve lexicographic order
+ return String.format("%05d", i);
+ case BYTES:
+ return new byte[]{(byte) i};
+ default:
+ throw new UnsupportedOperationException("Unsupported data type: " + dataType);
+ }
+ }
+
+ /// Compares two values of the given stored type. Handles BYTES gracefully whether the value is
+ /// a raw {@code byte[]} (no-dictionary path) or a {@link ByteArray} (dictionary path).
+ private static int compareValues(FieldSpec.DataType storedType, Object v1, Object v2) {
+ if (storedType == FieldSpec.DataType.BYTES) {
+ return ByteArray.compare((byte[]) v1, (byte[]) v2);
Review Comment:
`compareValues` claims it handles BYTES values as either `byte[]` (no-dictionary) or `ByteArray` (dictionary), but it unconditionally casts both inputs to `byte[]`. Either update the comment to match actual behavior, or add handling for `ByteArray` to avoid a potential `ClassCastException` if callers pass internal dictionary values.
```suggestion
final byte[] b1;
if (v1 instanceof byte[]) {
b1 = (byte[]) v1;
} else if (v1 instanceof ByteArray) {
b1 = ((ByteArray) v1).getBytes();
} else {
throw new IllegalArgumentException("Unsupported BYTES value type for v1: " + v1.getClass());
}
final byte[] b2;
if (v2 instanceof byte[]) {
b2 = (byte[]) v2;
} else if (v2 instanceof ByteArray) {
b2 = ((ByteArray) v2).getBytes();
} else {
throw new IllegalArgumentException("Unsupported BYTES value type for v2: " + v2.getClass());
}
return ByteArray.compare(b1, b2);
```
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Allow sorting on raw (no-dictionary) column [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #17965:
URL: https://github.com/apache/pinot/pull/17965#discussion_r2990839492
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/converter/stats/MutableNoDictionaryColStatistics.java:
##########
@@ -76,7 +86,188 @@ public int getLengthOfLargestElement() {
@Override
public boolean isSorted() {
- return false;
+ // Sorted column is guaranteed to be sorted by construction — no scan needed
+ if (_isSortedColumn) {
+ return true;
+ }
+
+ // Multi-valued column cannot be sorted
+ if (!_dataSourceMetadata.isSingleValue()) {
+ return false;
+ }
+
+ int numDocs = _dataSourceMetadata.getNumDocs();
+ if (numDocs <= 1) {
+ return true;
+ }
+
+ // Verify that values are non-decreasing when iterated in the given order
+ DataType storedType = _dataSourceMetadata.getFieldSpec().getDataType().getStoredType();
+ if (_sortedDocIds != null) {
+ switch (storedType) {
+ case INT: {
+ int prev = _forwardIndex.getInt(_sortedDocIds[0]);
+ for (int i = 1; i < numDocs; i++) {
+ int curr = _forwardIndex.getInt(_sortedDocIds[i]);
+ if (curr < prev) {
+ return false;
+ }
+ prev = curr;
+ }
+ return true;
+ }
+ case LONG: {
+ long prev = _forwardIndex.getLong(_sortedDocIds[0]);
+ for (int i = 1; i < numDocs; i++) {
+ long curr = _forwardIndex.getLong(_sortedDocIds[i]);
+ if (curr < prev) {
+ return false;
+ }
+ prev = curr;
+ }
+ return true;
+ }
+ case FLOAT: {
+ float prev = _forwardIndex.getFloat(_sortedDocIds[0]);
+ for (int i = 1; i < numDocs; i++) {
+ float curr = _forwardIndex.getFloat(_sortedDocIds[i]);
+ if (curr < prev) {
+ return false;
+ }
+ prev = curr;
+ }
+ return true;
+ }
+ case DOUBLE: {
+ double prev = _forwardIndex.getDouble(_sortedDocIds[0]);
+ for (int i = 1; i < numDocs; i++) {
+ double curr = _forwardIndex.getDouble(_sortedDocIds[i]);
+ if (curr < prev) {
+ return false;
Review Comment:
For FLOAT/DOUBLE sortedness checks, using `curr < prev` does not provide a total ordering (e.g., NaN, -0.0/+0.0). Consider using `Float.compare(curr, prev) < 0` / `Double.compare(curr, prev) < 0` (and apply the same logic in both the sortedDocIds and linear-scan branches) to match the ordering used elsewhere (e.g., the raw sorted-docId computation).
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/converter/stats/RealtimeSegmentStatsContainer.java:
##########
@@ -44,20 +44,8 @@ public class RealtimeSegmentStatsContainer implements SegmentPreIndexStatsContai
private final Map<String, ColumnStatistics> _columnStatisticsMap = new HashMap<>();
private final int _totalDocCount;
- @Deprecated
public RealtimeSegmentStatsContainer(MutableSegment mutableSegment, @Nullable int[] sortedDocIds,
- StatsCollectorConfig statsCollectorConfig) {
- this(mutableSegment, sortedDocIds, statsCollectorConfig, null, null);
- }
-
- @Deprecated
- public RealtimeSegmentStatsContainer(MutableSegment mutableSegment, @Nullable int[] sortedDocIds,
- StatsCollectorConfig statsCollectorConfig, @Nullable RecordReader recordReader) {
- this(mutableSegment, sortedDocIds, statsCollectorConfig, recordReader, null);
- }
-
- public RealtimeSegmentStatsContainer(MutableSegment mutableSegment, @Nullable int[] sortedDocIds,
- StatsCollectorConfig statsCollectorConfig, @Nullable RecordReader recordReader,
+ @Nullable String sortedColumn, StatsCollectorConfig statsCollectorConfig, @Nullable RecordReader recordReader,
@Nullable ThreadSafeMutableRoaringBitmap validDocIdsSnapshot) {
Review Comment:
This change removes previously public (though deprecated) constructors, which is a binary-breaking API change for any downstream code instantiating `RealtimeSegmentStatsContainer`. Consider keeping the deprecated overloads (delegating to the new constructor, passing `null` for `sortedColumn`) to preserve compatibility until a planned removal release.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/converter/stats/MutableNoDictionaryColStatistics.java:
##########
@@ -76,7 +86,188 @@ public int getLengthOfLargestElement() {
@Override
public boolean isSorted() {
- return false;
+ // Sorted column is guaranteed to be sorted by construction — no scan needed
+ if (_isSortedColumn) {
+ return true;
+ }
+
+ // Multi-valued column cannot be sorted
+ if (!_dataSourceMetadata.isSingleValue()) {
+ return false;
+ }
+
+ int numDocs = _dataSourceMetadata.getNumDocs();
+ if (numDocs <= 1) {
+ return true;
+ }
+
+ // Verify that values are non-decreasing when iterated in the given order
+ DataType storedType = _dataSourceMetadata.getFieldSpec().getDataType().getStoredType();
+ if (_sortedDocIds != null) {
+ switch (storedType) {
+ case INT: {
+ int prev = _forwardIndex.getInt(_sortedDocIds[0]);
+ for (int i = 1; i < numDocs; i++) {
+ int curr = _forwardIndex.getInt(_sortedDocIds[i]);
+ if (curr < prev) {
+ return false;
+ }
+ prev = curr;
+ }
+ return true;
+ }
+ case LONG: {
+ long prev = _forwardIndex.getLong(_sortedDocIds[0]);
+ for (int i = 1; i < numDocs; i++) {
+ long curr = _forwardIndex.getLong(_sortedDocIds[i]);
+ if (curr < prev) {
+ return false;
+ }
+ prev = curr;
+ }
+ return true;
+ }
+ case FLOAT: {
+ float prev = _forwardIndex.getFloat(_sortedDocIds[0]);
+ for (int i = 1; i < numDocs; i++) {
+ float curr = _forwardIndex.getFloat(_sortedDocIds[i]);
+ if (curr < prev) {
+ return false;
+ }
+ prev = curr;
+ }
+ return true;
+ }
+ case DOUBLE: {
+ double prev = _forwardIndex.getDouble(_sortedDocIds[0]);
+ for (int i = 1; i < numDocs; i++) {
+ double curr = _forwardIndex.getDouble(_sortedDocIds[i]);
+ if (curr < prev) {
+ return false;
+ }
+ prev = curr;
+ }
+ return true;
+ }
+ case BIG_DECIMAL: {
+ BigDecimal prev = _forwardIndex.getBigDecimal(_sortedDocIds[0]);
+ for (int i = 1; i < numDocs; i++) {
+ BigDecimal curr = _forwardIndex.getBigDecimal(_sortedDocIds[i]);
+ if (curr.compareTo(prev) < 0) {
+ return false;
+ }
+ prev = curr;
+ }
+ return true;
+ }
+ case STRING: {
+ String prev = _forwardIndex.getString(_sortedDocIds[0]);
+ for (int i = 1; i < numDocs; i++) {
+ String curr = _forwardIndex.getString(_sortedDocIds[i]);
+ if (curr.compareTo(prev) < 0) {
+ return false;
+ }
+ prev = curr;
+ }
+ return true;
+ }
+ case BYTES: {
+ byte[] prev = _forwardIndex.getBytes(_sortedDocIds[0]);
+ for (int i = 1; i < numDocs; i++) {
+ byte[] curr = _forwardIndex.getBytes(_sortedDocIds[i]);
+ if (ByteArray.compare(prev, curr) < 0) {
+ return false;
+ }
+ prev = curr;
+ }
Review Comment:
BYTES sortedness check is inverted: `ByteArray.compare(prev, curr) < 0` returns false when `prev < curr`, causing increasing byte sequences to be marked unsorted and decreasing sequences to be marked sorted. The comparison should detect `curr < prev` (e.g., `ByteArray.compare(prev, curr) > 0` or `ByteArray.compare(curr, prev) < 0`).
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/converter/stats/RealtimeSegmentSegmentCreationDataSource.java:
##########
@@ -36,28 +36,33 @@ public class RealtimeSegmentSegmentCreationDataSource implements SegmentCreation
private final MutableSegment _mutableSegment;
private final RecordReader _recordReader;
private final int[] _sortedDocIds;
+ @Nullable
+ private final String _sortedColumn;
private final ThreadSafeMutableRoaringBitmap _validDocIdsSnapshot;
public RealtimeSegmentSegmentCreationDataSource(MutableSegment mutableSegment,
- PinotSegmentRecordReader recordReader) {
+ PinotSegmentRecordReader recordReader, @Nullable String sortedColumn) {
_mutableSegment = mutableSegment;
_recordReader = recordReader;
_sortedDocIds = recordReader.getSortedDocIds();
Review Comment:
This constructor signature change (adding `sortedColumn`) is a binary-breaking API change for any external code constructing `RealtimeSegmentSegmentCreationDataSource`. Consider reintroducing the old overload(s) that delegate with `sortedColumn = null` and mark them @Deprecated.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/converter/stats/MutableNoDictionaryColStatistics.java:
##########
@@ -76,7 +86,188 @@ public int getLengthOfLargestElement() {
@Override
public boolean isSorted() {
- return false;
+ // Sorted column is guaranteed to be sorted by construction — no scan needed
+ if (_isSortedColumn) {
+ return true;
+ }
+
+ // Multi-valued column cannot be sorted
+ if (!_dataSourceMetadata.isSingleValue()) {
+ return false;
+ }
+
+ int numDocs = _dataSourceMetadata.getNumDocs();
+ if (numDocs <= 1) {
+ return true;
+ }
+
+ // Verify that values are non-decreasing when iterated in the given order
+ DataType storedType = _dataSourceMetadata.getFieldSpec().getDataType().getStoredType();
+ if (_sortedDocIds != null) {
+ switch (storedType) {
+ case INT: {
+ int prev = _forwardIndex.getInt(_sortedDocIds[0]);
+ for (int i = 1; i < numDocs; i++) {
+ int curr = _forwardIndex.getInt(_sortedDocIds[i]);
+ if (curr < prev) {
+ return false;
+ }
+ prev = curr;
+ }
+ return true;
+ }
+ case LONG: {
+ long prev = _forwardIndex.getLong(_sortedDocIds[0]);
+ for (int i = 1; i < numDocs; i++) {
+ long curr = _forwardIndex.getLong(_sortedDocIds[i]);
+ if (curr < prev) {
+ return false;
+ }
+ prev = curr;
+ }
+ return true;
+ }
+ case FLOAT: {
+ float prev = _forwardIndex.getFloat(_sortedDocIds[0]);
+ for (int i = 1; i < numDocs; i++) {
+ float curr = _forwardIndex.getFloat(_sortedDocIds[i]);
+ if (curr < prev) {
+ return false;
+ }
+ prev = curr;
+ }
+ return true;
+ }
+ case DOUBLE: {
+ double prev = _forwardIndex.getDouble(_sortedDocIds[0]);
+ for (int i = 1; i < numDocs; i++) {
+ double curr = _forwardIndex.getDouble(_sortedDocIds[i]);
+ if (curr < prev) {
+ return false;
+ }
+ prev = curr;
+ }
+ return true;
+ }
+ case BIG_DECIMAL: {
+ BigDecimal prev = _forwardIndex.getBigDecimal(_sortedDocIds[0]);
+ for (int i = 1; i < numDocs; i++) {
+ BigDecimal curr = _forwardIndex.getBigDecimal(_sortedDocIds[i]);
+ if (curr.compareTo(prev) < 0) {
+ return false;
+ }
+ prev = curr;
+ }
+ return true;
+ }
+ case STRING: {
+ String prev = _forwardIndex.getString(_sortedDocIds[0]);
+ for (int i = 1; i < numDocs; i++) {
+ String curr = _forwardIndex.getString(_sortedDocIds[i]);
+ if (curr.compareTo(prev) < 0) {
+ return false;
+ }
+ prev = curr;
+ }
+ return true;
+ }
+ case BYTES: {
+ byte[] prev = _forwardIndex.getBytes(_sortedDocIds[0]);
+ for (int i = 1; i < numDocs; i++) {
+ byte[] curr = _forwardIndex.getBytes(_sortedDocIds[i]);
+ if (ByteArray.compare(prev, curr) < 0) {
+ return false;
+ }
+ prev = curr;
+ }
+ return true;
+ }
+ default:
+ return false;
+ }
+ } else {
+ switch (storedType) {
+ case INT: {
+ int prev = _forwardIndex.getInt(0);
+ for (int i = 1; i < numDocs; i++) {
+ int curr = _forwardIndex.getInt(i);
+ if (curr < prev) {
+ return false;
+ }
+ prev = curr;
+ }
+ return true;
+ }
+ case LONG: {
+ long prev = _forwardIndex.getLong(0);
+ for (int i = 1; i < numDocs; i++) {
+ long curr = _forwardIndex.getLong(i);
+ if (curr < prev) {
+ return false;
+ }
+ prev = curr;
+ }
+ return true;
+ }
+ case FLOAT: {
+ float prev = _forwardIndex.getFloat(0);
+ for (int i = 1; i < numDocs; i++) {
+ float curr = _forwardIndex.getFloat(i);
+ if (curr < prev) {
+ return false;
+ }
+ prev = curr;
+ }
+ return true;
+ }
+ case DOUBLE: {
+ double prev = _forwardIndex.getDouble(0);
+ for (int i = 1; i < numDocs; i++) {
+ double curr = _forwardIndex.getDouble(i);
+ if (curr < prev) {
+ return false;
+ }
+ prev = curr;
+ }
+ return true;
+ }
+ case BIG_DECIMAL: {
+ BigDecimal prev = _forwardIndex.getBigDecimal(0);
+ for (int i = 1; i < numDocs; i++) {
+ BigDecimal curr = _forwardIndex.getBigDecimal(i);
+ if (curr.compareTo(prev) < 0) {
+ return false;
+ }
+ prev = curr;
+ }
+ return true;
+ }
+ case STRING: {
+ String prev = _forwardIndex.getString(0);
+ for (int i = 1; i < numDocs; i++) {
+ String curr = _forwardIndex.getString(i);
+ if (curr.compareTo(prev) < 0) {
+ return false;
+ }
+ prev = curr;
+ }
+ return true;
+ }
+ case BYTES: {
+ byte[] prev = _forwardIndex.getBytes(0);
+ for (int i = 1; i < numDocs; i++) {
+ byte[] curr = _forwardIndex.getBytes(i);
+ if (ByteArray.compare(prev, curr) < 0) {
+ return false;
+ }
+ prev = curr;
+ }
Review Comment:
Same issue in the non-sorted-docIds BYTES path: `ByteArray.compare(prev, curr) < 0` checks the wrong direction and will report descending data as sorted and ascending as unsorted. Flip the comparison to detect `curr < prev`.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org