arunkumarucet commented on code in PR #18891:
URL: https://github.com/apache/pinot/pull/18891#discussion_r3627640593
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/json/ImmutableJsonIndexReader.java:
##########
@@ -783,34 +797,76 @@ public String[][] getValuesMV(int[] docIds, int length,
Map<String, RoaringBitma
@Override
public String[] getValuesSV(int[] docIds, int length, Map<String,
RoaringBitmap> valueToMatchingDocs,
boolean isFlattenedDocIds) {
- Int2ObjectOpenHashMap<String> docIdToValues = new
Int2ObjectOpenHashMap<>(docIds.length);
- RoaringBitmap docIdMask = RoaringBitmap.bitmapOf(docIds);
-
- for (Map.Entry<String, RoaringBitmap> entry :
valueToMatchingDocs.entrySet()) {
- String value = entry.getKey();
- RoaringBitmap matchingDocIds = entry.getValue();
- if (isFlattenedDocIds) {
- matchingDocIds.forEach((IntConsumer) flattenedDocId -> {
+ String[] values = new String[length];
+ if (length == 0) {
+ return values;
+ }
+
+ if (isFlattenedDocIds) {
+ // Flattened doc ids must be converted to real doc ids before they can
be matched against the block's doc ids,
+ // so we still build a docId -> value lookup keyed by the converted doc
id.
+ Int2ObjectOpenHashMap<String> docIdToValues = new
Int2ObjectOpenHashMap<>(length);
+ RoaringBitmap docIdMask = RoaringBitmap.bitmapOf(docIds);
+ for (Map.Entry<String, RoaringBitmap> entry :
valueToMatchingDocs.entrySet()) {
+ String value = entry.getKey();
+ entry.getValue().forEach((IntConsumer) flattenedDocId -> {
int docId = getDocId(flattenedDocId);
if (docIdMask.contains(docId)) {
docIdToValues.put(docId, value);
}
});
- } else {
- RoaringBitmap intersection = RoaringBitmap.and(matchingDocIds,
docIdMask);
- if (intersection.isEmpty()) {
- continue;
+ }
+ for (int i = 0; i < length; i++) {
+ values[i] = docIdToValues.get(docIds[i]);
+ }
+ return values;
+ }
+
+ // Non-flattened path (the only path used by jsonExtractIndex SV): docIds
are real, ascending-sorted doc ids for
+ // the current block, and for a single-value path each doc belongs to at
most one value's posting list. We can
+ // therefore scatter values directly, iterating each value's posting list
only over the block's [lo, hi] doc id
+ // range with a peekable iterator. This avoids allocating a per-block doc
id mask bitmap, a RoaringBitmap.and per
+ // distinct value, and an intermediate docId -> value hash map.
+ int lo = docIds[0];
Review Comment:
Good catch, thanks — you're right. Fixed in a7c1164.
Root cause was exactly as you described: the fast path assumed ascending
block doc ids, but a selection with `ORDER BY <sortedColumn> DESC` (with
`allowReverseOrder`) feeds strictly descending, gap-free blocks via
`ReverseDocIdSetOperator`, so `lo > hi`, the dense guard went negative, and the
sparse branch's `advanceIfNeeded(lo)` + `docId > hi` break wrote nothing —
`jsonExtractIndex` then returned null/default (or threw `Illegal Json Path`
when no defaultValue was set) for every row that had the path.
Fix:
- The ascending offset-write fast path now also requires `first <= last`.
- Added a symmetric **descending** fast path (`values[first - docId]`) so
`ORDER BY ... DESC` keeps the allocation-free path rather than falling into a
per-block hash map.
- The sparse fallback now computes the block's true min/max, so its range
scan is correct for any ordering.
Added a reader-level regression test (dense + sparse descending blocks) and
an end-to-end `JsonPathTest` case projecting `json_extract_index` under `ORDER
BY <sortedColumn> DESC`; both fail against the pre-fix reader and pass now.
--
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]
