rohityadav1993 opened a new issue, #19923:
URL: https://github.com/apache/pinot/issues/19923
Part of #18667. Follow-up to #19120.
## Context
In #19120, `StreamingSelectionOrderByOperator` was added as the per-segment
leaf of the streaming selection
ORDER BY combine. Rows are emitted in runs. A run is a group of rows with
equal values in the sorted prefix.
When a selected column is not in the ORDER BY, the leaf works in two phases.
```mermaid
flowchart LR
G["Planner gate<br/>InstancePlanMakerImplV2<br/>(AUTO, direction)"] --> L
subgraph L["StreamingSelectionOrderByOperator (one per segment)"]
S["Scan in sorted order<br/>(reversed for DESC)"] --> R["Cut a
run<br/>sort run by unsorted tail"]
R --> F["Phase two<br/>fetchNonOrderByColumns()"]
end
F --> C["Streaming combine<br/>k-way merge"]
G -. "A3" .- S
S -. "A4" .- S
F -. "A1, A2" .- F
```
The items below were found in review and were deferred from #19120. Items A1
and A2 change the same code, so
one PR is recommended for both.
## A1. Batch small runs before the phase-two fetch
Source: review comment
https://github.com/apache/pinot/pull/19120#discussion_r4251361251
One-row runs are produced when the leading sorted key is (almost) unique and
an unsorted ORDER BY tail follows.
Phase two is then done once per row.
```
Today (one-row runs):
run 1 -> build projection, fetcher, reader contexts, transform tree,
bitmap -> fetch 1 row -> close
run 2 -> build ...
-> fetch 1 row -> close
...
compressed raw column: the same chunk is decompressed again for each row
Proposed:
run 1 + run 2 + ... + run k (bounded batch) -> build once -> fetch k rows
-> close
```
- The output block size of the combine does not amortize this cost, because
the cost is in the leaf.
- Since #19120 (review item P2a), runs are cut on the whole sorted prefix.
More queries reach this case, for
example `ORDER BY day, ts` where both columns are sorted and `ts` is
almost unique.
- Expected change: consecutive small runs are coalesced into bounded leaf
blocks before phase two, so the reader
contexts are reused within each batch.
## A2. Share the selected-row projection logic with
`SelectionOrderByOperator`
Source: review body note 2,
https://github.com/apache/pinot/pull/19120#pullrequestreview-5494854939
The phase-two path duplicates logic from `SelectionOrderByOperator`:
| Step | `SelectionOrderByOperator` | `StreamingSelectionOrderByOperator` |
|---|---|---|
| Sort doc IDs | yes | duplicated |
| Fetch non-ORDER-BY columns | yes | duplicated |
| Restore nulls | yes | duplicated |
| Build the output `DataSchema` | yes | duplicated |
| Resumable cursor, exhaustion | no | streaming only |
- Expected change: a focused helper is extracted, with explicit buffer
ownership.
- Inheritance from the one-shot operator is not wanted: the two execution
contracts are different.
- The A1 batch is expected to be built on this helper.
## A3. `allowReverseOrder=true` does not guarantee a reversed scan
Source: in-code TODO in `InstancePlanMakerImplV2` (cites
https://github.com/apache/pinot/pull/19120#discussion_r3871713989)
```mermaid
flowchart LR
Q["DESC leading ORDER BY<br/>allowReverseOrder=true"] --> A["AUTO resolves
ON"]
A --> P["getSortedByProject()"]
P -->|"doc ID set cannot be reversed:<br/>failure is swallowed"|
U["unreversed operator returned"]
U --> D["leaf gate declines"]
D --> M["merge runs over materialized children"]
```
- Result: AUTO can resolve ON while no leaf streams. The result is correct;
only the expected benefit is lost.
- Expected change: a predicate is added that tells, per segment and before
the plan is built, if the segment can
be scanned in the query's direction. AUTO is then resolved against that
predicate.
- This case is not tested today.
## A4. DESC with null handling: leftover reverse-bitmap cost
Source: residual R3 of #19120 (partly fixed there)
- Fixed in #19120: segments flagged non-null no longer build the DESC
reverse bitmap up front.
- Not fixed:
- A segment that is not flagged non-null is still force-activated. The
reverse bitmap is built for it even when
its non-null range is outside the limit.
- A null value on the merge frontier cannot be compared by the pruning
check (`sortsBeyond`). So a null-bearing
segment can keep later flagged segments from being skipped.
- Expected change: the force-activation is limited, or the null case is made
comparable for pruning. A test that
counts built reverse bitmaps is recommended.
## Out of scope
Combine-side follow-ups are tracked in #<ISSUE-B>.
--
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]
