You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "arunkumarucet (via GitHub)" <gi...@apache.org> on 2026/06/30 12:32:17 UTC
[PR] Optimize ImmutableJsonIndexReader hot paths for json_extract_index and json_match [pinot]
arunkumarucet opened a new pull request, #18891:
URL: https://github.com/apache/pinot/pull/18891
## Summary
Profiling a JSON analytics workload — `JSON_EXTRACT_INDEX(col, '\$.path', ...)` group-by and `JSON_MATCH(col, ...)` filters over a JSON-indexed column — surfaced three hot spots in `ImmutableJsonIndexReader`, all in the per-query value / doc-id materialization. This PR optimizes them with no behavior change.
### Changes
1. **`getValuesSV`** (~16% of query CPU): previously, for every value-block it allocated a `RoaringBitmap` mask via `bitmapOf`, ran a `RoaringBitmap.and` per distinct value, and populated an `Int2ObjectOpenHashMap` (find/get/insert). The non-flattened path — the only path used by `jsonExtractIndex` SV — now scatters values with a bounded `PeekableIntIterator`:
- **Dense, gap-free block** (the common full-scan case): the result position is the doc-id offset, so values are written directly with no map, no mask, and no per-value `and`.
- **Sparse block** (e.g. after a selective filter): a primitive `Int2IntOpenHashMap` maps each doc id to its position once, then each value's posting list is range-scanned within `[lo, hi]`.
2. **`convertFlattenedDocIdsToDocIds`** and the `JSON_MATCH` **`getMatchingDocIds`** path both built result bitmaps with a per-element `bitmap.add(getDocId(f))`, causing `RoaringArray.setContainerAtIndex` churn (~10% of query CPU). The flattened → real doc-id mapping is monotonically non-decreasing (the index flattens documents in doc-id order), so the mapped doc ids are produced sorted and are now appended through an ordered `RoaringBitmapWriter`, avoiding the per-element binary search and container reallocation.
### Results
On a 1M-row segment a `JSON_EXTRACT_INDEX` group-by query drops from ~34ms to ~16ms (~2x), and `setContainerAtIndex` disappears from the profile. Output is identical.
### Testing
`JsonIndexTest`, `JsonExtractIndexTransformFunctionTest`, and `JsonIndexDistinctOperatorQueriesTest` all pass (62 tests). `spotless:apply` and `checkstyle:check` clean.
🤖 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] Optimize ImmutableJsonIndexReader hot paths for json_extract_index and json_match [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #18891:
URL: https://github.com/apache/pinot/pull/18891#discussion_r3532886919
##########
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:
This assumes the block docIds are ascending, but selection queries can run the TransformOperator over DESC doc-id blocks when ordering by a sorted column via withOrder(DESC). In that case lo > hi, so the range scan never fills matching values and jsonExtractIndex can return null/default or throw for rows that do have the path. Keep the order-insensitive mask path unless ascending is guaranteed, or detect non-ascending blocks and scatter with the true min/max range.
--
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] Optimize ImmutableJsonIndexReader hot paths for json_extract_index and json_match [pinot]
Posted by "arunkumarucet (via GitHub)" <gi...@apache.org>.
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: 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] Optimize ImmutableJsonIndexReader hot paths for json_extract_index and json_match [pinot]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
Vamsi-klu commented on PR #18891:
URL: https://github.com/apache/pinot/pull/18891#issuecomment-4912347954
+1 to xiangfu0's DESC-order finding — the concrete producer is ReverseDocIdSetOperator.getNextBlock(), which fills the block from getReverseIntIterator(), so an ORDER BY <sortedCol> DESC selection feeds strictly descending doc ids into getValuesSV. With a descending block lo=docIds[0] is the max and hi=docIds[length-1] the min, so lo>hi: the dense guard (long)hi-lo+1==length goes negative and drops to the sparse branch, where advanceIfNeeded(lo) plus the `if (docId > hi) break` writes nothing — silent null/default for every row that has the path.
One caveat on the suggested "scatter with true min/max" fix: it repairs the sparse branch (docIdToPos is order-independent) but does NOT fix the dense fast-path. A descending block is also gap-free, so with min/max it still satisfies the dense guard and then writes values[docId - lo], which assumes docIds[i] == lo + i — for [5,4,3,2,1] that lands doc 5's value at index 4 instead of 0. So the dense branch should stay ascending-specific (e.g. also require docIds[0] <= docIds[length-1]) and fall back to sparse/mask otherwise.
getValuesMV was left on the mask path, so only the SV fast-path regressed; a regression test projecting json_extract_index under ORDER BY <sortedCol> DESC would pin it directly (the current JsonIndexTest / JsonExtractIndexTransformFunctionTest only exercise ascending blocks, which is why CI stays green). The flattenedToDocIds / convertFlattenedDocIdsToDocIds RoaringBitmapWriter changes look correct — getDocId is monotonic over ascending flattened ids, so ordered append is valid there.
--
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] Optimize ImmutableJsonIndexReader hot paths for json_extract_index and json_match [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #18891:
URL: https://github.com/apache/pinot/pull/18891#issuecomment-4843999372
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/18891?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 `42.85714%` with `28 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 56.96%. Comparing base ([`2103aa2`](https://app.codecov.io/gh/apache/pinot/commit/2103aa27006bcfe63490623063de952cb424c46f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`89d3357`](https://app.codecov.io/gh/apache/pinot/commit/89d335783209b5f8c4001780da4c40835b934da3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/18891?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...t/index/readers/json/ImmutableJsonIndexReader.java](https://app.codecov.io/gh/apache/pinot/pull/18891?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Fsegment%2Findex%2Freaders%2Fjson%2FImmutableJsonIndexReader.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L3JlYWRlcnMvanNvbi9JbW11dGFibGVKc29uSW5kZXhSZWFkZXIuamF2YQ==) | 42.85% | [25 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18891?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 (2103aa2) and HEAD (89d3357). Click for more details.
>
> <details><summary>HEAD has 13 uploads less than BASE</summary>
>
>| Flag | BASE (2103aa2) | HEAD (89d3357) |
>|------|------|------|
>|java-21|5|2|
>|temurin|5|2|
>|integration|3|0|
>|integration1|1|0|
>|unittests2|1|0|
>|custom-integration1|1|0|
>|integration2|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #18891 +/- ##
============================================
- Coverage 64.77% 56.96% -7.82%
+ Complexity 1318 1 -1317
============================================
Files 3393 2604 -789
Lines 211559 152959 -58600
Branches 33283 24883 -8400
============================================
- Hits 137029 87126 -49903
+ Misses 63458 58407 -5051
+ Partials 11072 7426 -3646
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/18891/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/18891/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/18891/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/18891/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/18891/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/18891/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.96% <42.85%> (-7.82%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/18891/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.96% <42.85%> (-7.82%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/18891/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.96% <42.85%> (-7.81%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/18891/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.96% <42.85%> (-0.03%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/18891/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 Harness](https://app.codecov.io/gh/apache/pinot/pull/18891?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] Optimize ImmutableJsonIndexReader hot paths for json_extract_index and json_match [pinot]
Posted by "arunkumarucet (via GitHub)" <gi...@apache.org>.
arunkumarucet commented on PR #18891:
URL: https://github.com/apache/pinot/pull/18891#issuecomment-5042126410
@Vamsi-klu thanks for the detailed trace — the `ReverseDocIdSetOperator.getNextBlock()` / `getReverseIntIterator()` path is exactly the reproducer, and your caveat about the dense fast path was spot on. Addressed in a7c1164.
Specifically to your points:
- **Dense path must stay ascending-specific.** Agreed — a "scatter with true min/max" fix repairs the sparse branch (`docIdToPos` is order-independent) but a descending block is still gap-free, so it would re-enter the dense guard and mis-scatter via `docId - lo`. The ascending fast path now additionally requires `first <= last`. Rather than let descending blocks fall back to the hash-map branch (a per-block `Int2IntOpenHashMap` alloc on a hot path), I added a **symmetric descending fast path** that writes `values[first - docId]` over `[last, first]`, so `ORDER BY ... DESC` keeps the allocation-free path too.
- **Sparse fallback** now computes the block's actual min/max, so its scan bounds are correct regardless of ordering.
- **`getValuesMV`** — confirmed it was left on the mask path and never regressed; no change there.
- **Regression coverage** — added your suggested end-to-end case: a `JsonPathTest` query projecting `json_extract_index(...)` under `ORDER BY <sortedColumn> DESC` on a genuinely sorted column (with `allowReverseOrder=true`), asserting each row's value rather than degrading to null. It throws `Illegal Json Path` against the pre-fix reader and passes now. Also added a reader-level test with hand-built dense `[2,1,0]` and sparse `[2,0]` descending blocks.
- **`flattenedToDocIds` / `convertFlattenedDocIdsToDocIds`** — thanks for confirming the ordered `RoaringBitmapWriter` change is valid; left as is.
--
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