gortiz opened a new pull request, #19612:
URL: https://github.com/apache/pinot/pull/19612
`RealtimeNgramFilteringIndex.getDocIds` has two defects.
### 1. Leaked read lock
```java
_readLock.lock();
Iterable<String> ngrams = generateNgrams(searchQuery);
if (!ngrams.iterator().hasNext()) {
return null; // <-- returns while still holding the read lock
}
...
try { ... } finally { _readLock.unlock(); }
```
The lock is taken *before* the `try`, so the early return taken when the
search query is shorter than `minNgramLength` never releases it. A
`ReentrantReadWriteLock` read hold blocks every later write lock acquisition,
so a single such query permanently stalls ingestion for that column.
`generateNgrams` throwing would leak the lock in the same way.
### 2. `IndexOutOfBoundsException` for an n-gram that was never indexed
`_ngramToDictIdMapping` is created with `defaultReturnValue(-1)`, and that
`-1` was passed straight to `RealtimeInvertedIndex.getDocIds`, whose bounds
check only guards the upper end (`if (_bitmaps.size() <= dictId)`). Searching
for any n-gram absent from the index threw:
```
java.lang.IndexOutOfBoundsException: Index -1 out of bounds for length 29
at java.base/java.util.ArrayList.get(ArrayList.java:428)
at ...RealtimeInvertedIndex.getDocIds(RealtimeInvertedIndex.java:89)
at
...RealtimeNgramFilteringIndex.getDocIds(RealtimeNgramFilteringIndex.java:121)
```
## Fix
- Generate the n-grams before taking the read lock (the method only reads
the query string and the immutable length settings), then wrap the whole
critical section in `try`/`finally`.
- Treat an unknown n-gram as an empty intersection and return an empty
bitmap.
- Drop the intermediate `ArrayList` in favour of intersecting as we go,
which also lets the loop exit as soon as the result is empty.
The `null` return for "no n-grams generated" is unchanged, so `testQueries`
still passes as written.
## Testing
Two regression tests added to `RealtimeNgramFilteringIndexTest`. Both fail
on the current implementation:
```
[ERROR]
RealtimeNgramFilteringIndexTest.testQueryWithNgramThatWasNeverIndexed:87
» IndexOutOfBounds Index -1 out of bounds for length 29
[ERROR]
RealtimeNgramFilteringIndexTest.testReadLockReleasedWhenNoNgramIsGenerated:114
Writer thread is blocked by a leaked read lock expected [true] but
found [false]
```
and pass with it (`Tests run: 3, Failures: 0, Errors: 0`).
This index is not wired into the query path yet (it was added in #16364 as
"ngram index part 1"; only the unit test and `BenchmarkNgramFilteringIndex` use
it today), so there is no user-visible behaviour change to flag.
🤖 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: [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]
