perf: Descending vector cursors. - #19947
Conversation
This patch adds descending-order vector cursors, enabling faster execution for timeseries, timeBoundary, and scan queries that run in descending order.
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 1 |
| P3 | 0 |
| Total | 1 |
Reviewed 26 of 26 changed files.
Validation: git diff --check passed; no builds or tests were run.
This is an automated review by Codex GPT-5.6-Luna(max)
| * batches in internally-ascending order. This allows them to be simpler, as they do not need to have handling for | ||
| * both ascending and descending order. | ||
| */ | ||
| public class ReverseVectorColumnSelectorFactory implements VectorColumnSelectorFactory |
There was a problem hiding this comment.
[P2] Reverse factory bypasses virtual-column group-by handling
The wrapper inherits the generic makeGroupByVectorColumnSelector implementation instead of preserving the underlying factory's virtual-column specialization. A descending vectorized group-by using an ExpressionVirtualColumn can therefore reach the generic ARRAY processor and fail with UnsupportedOperationException instead of using the supported deferred expression selector. Add a reverse-aware group-by override and regression coverage.
There was a problem hiding this comment.
makeGroupByVectorColumnSelector won't be called anyway since groupBy doesn't use descending cursors. I'll add an explicit override that throws NotYetImplemented.
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 1 |
| P3 | 0 |
| Total | 1 |
Reviewed 26 of 26 changed files.
This is an automated review by Codex GPT-5.6-Luna(max)
| * it advances as long as the next value is larger than the given value, i.e., it is the reverse-order | ||
| * analog of the forward iterator's behavior. | ||
| */ | ||
| public static PeekableIntIterator getReverseBitmapOffsetIterator(ImmutableBitmap bitmapIndex) |
There was a problem hiding this comment.
[P2] Preserve the reverse-iterator binary signature
Changing the public method return type from IntIterator to PeekableIntIterator changes its JVM descriptor. Precompiled callers using the previous processing artifact can fail with NoSuchMethodError at runtime. Preserve the IntIterator-returning method and expose the peekable iterator under a new method name.
There was a problem hiding this comment.
I am not worried about this, this function is an internal API.
FrankChen021
left a comment
There was a problem hiding this comment.
Reviewed 26 of 26 changed files. The follow-up concern is handled: normal groupBy cursor specs do not request descending ordering, and the explicit NotYetImplemented guard addresses the original concern. No further inline reply is needed.
This is an automated review by Codex GPT-5.6-Luna(max)
This patch adds descending-order vector cursors, enabling faster execution for timeseries, timeBoundary, and scan queries that run in descending order.