You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "hanahmily (via GitHub)" <gi...@apache.org> on 2026/09/23 04:55:31 UTC
[PR] fix(query): bound the vectorized measure liaison reduce at the uint16 selection boundary [skywalking-banyandb]
hanahmily opened a new pull request, #1372:
URL: https://github.com/apache/skywalking-banyandb/pull/1372
## The defect
`RecordBatch.Selection` is `[]uint16`. `activeIndices` materializes a nil `Selection` as `[0, Len)` into that slice, and `BatchAggregation.Consume` drives its group loop from it. Past 65,536 rows the index wraps to `0`: early rows are folded into their group a second time and the rows past the wrap are never visited — **a silently wrong aggregate, with no error**.
This is reachable today, not theoretical. `DrainPipelineToFrame` coalesces a data node's entire pipeline output into one frame, and `ReduceRawFrames` decodes it as a single batch. The `Top`-bearing distributed shapes set an unbounded per-node limit, so a node producing more than 65,535 groups hits it now.
## The fix
- **Cap `BatchSize` at `MaxUint16`** in config validation. Every operator paginates its *output* at `batchSize`, so a configured value above the bound would reopen the same wrap from the emit side. `measure` and `trace` gain the ceiling; `stream` already had it, and is the model both follow.
- **Make `activeIndices` return an error** instead of returning wrapped indices. This is the one chokepoint every caller shares, so asserting there catches a violation regardless of which caller or construction site introduced the oversized batch. Guarding at the construction sites was tried first and rejected: `frame.Decode` and the multi-group reshape legitimately hold batches that large.
- **Consume oversized batches in bounded chunks** (`vectorized.ConsumeChunked`), copied into a reused scratch batch. Applied to the liaison's aggregation reduce, `ApplyTopToReduce`, and the raw `Top` / raw `GroupBy` passes.
Widening `Selection` to `[]uint32` was considered and rejected. The value is the **bounded buffer**, not the raised ceiling: widening would let the liaison materialize an arbitrarily large working set and merely survive indexing it, whereas chunking keeps the consumer's per-step footprint proportional to `batchSize` regardless of what a node sent. It would also touch every `activeIndices` caller and double the selection vector for the overwhelming majority of queries that never exceed a thousand rows.
## Scope notes
Only the **aggregation reduce** was reachable with an oversized batch. The raw `Top` and raw `GroupBy` passes are fed by `mergeDistributedRows`, whose emitter already pages at `batchSize` — chunking them is hardening, and the changelog says so rather than claiming a live fix. The `trace` ceiling is a real gap though: `--trace-vectorized-batch-size` is operator-settable, so a value above the bound silently wrapped. **A node configured above the bound will now fail to start rather than return wrong results.**
`ConsumeChunked` lives in `pkg/query/vectorized` beside the `RecordBatch`, `Column` and `BreakerOperator` definitions it is written against, since the exposure belongs to `RecordBatch` itself rather than to measure.
## Verification
Every fix is gated by a test that fails without it, verified by reverting the implementation and re-running — not assumed:
- `ReducePartialBatches` over a 65,537-row partial: wrong sums before, correct after, and identical to the same data split across smaller partials.
- `applyBatchTopToRows` / `applyBatchGroupByFirstToRows` / `ApplyTopToReduce` at the same boundary.
- `activeIndices` boundary units: safe at 65,536, errors at 65,537, non-nil `Selection` unaffected.
- Config validation at and above `MaxUint16` for both `measure` and `trace`.
The boundary is worth stating precisely, because it is easy to get wrong: `Len == 65536` is **safe** (indices `0..65535` all fit in a `uint16`); `Len == 65537` is the first wrapping size.
`make pre-push` passes, including `generate` and `generate-test-cases` producing no diff.
## Follow-up
This is the prerequisite for a larger change to `LIMIT`/`OFFSET` with aggregation on the distributed path, where the per-node window currently truncates partial aggregates before the liaison reduces them. The design for that is in a companion docs PR; this lands first and independently, because it fixes a defect that exists today on the `Top` paths regardless of that work.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(query): bound the vectorized measure liaison reduce at the uint16 selection boundary [skywalking-banyandb]
Posted by "hanahmily (via GitHub)" <gi...@apache.org>.
hanahmily merged PR #1372:
URL: https://github.com/apache/skywalking-banyandb/pull/1372
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(query): bound the vectorized measure liaison reduce at the uint16 selection boundary [skywalking-banyandb]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1372:
URL: https://github.com/apache/skywalking-banyandb/pull/1372#issuecomment-5789366854
## [Codecov](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/1372?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 60.13%. Comparing base ([`3530dd9`](https://app.codecov.io/gh/apache/skywalking-banyandb/commit/3530dd9b5cefaa4fef60eec91ecbfbdfcaf243c5?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`0a4ddad`](https://app.codecov.io/gh/apache/skywalking-banyandb/commit/0a4ddad13a409ab06902e9f8f4fd6e9ae88e11dd?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 449 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #1372 +/- ##
===========================================
+ Coverage 45.97% 60.13% +14.15%
===========================================
Files 328 35 -293
Lines 55505 6283 -49222
===========================================
- Hits 25520 3778 -21742
+ Misses 27909 2282 -25627
+ Partials 2076 223 -1853
```
| [Flag](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/1372/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [fodc](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/1372/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `60.13% <ø> (?)` | |
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/skywalking-banyandb/pull/1372?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>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org