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/19 00:08:18 UTC
[PR] feat(query): add COUNT_DISTINCT push-down for measure queries [skywalking-banyandb]
hanahmily opened a new pull request, #1365:
URL: https://github.com/apache/skywalking-banyandb/pull/1365
## Summary
Implements apache/skywalking#14090 (Part 3 of the tag-aggregation-and-time-bucketing design, `docs/design/0.12.0/tag-aggregation/README.md`, merged in #1360): an exact, budget-enforced `COUNT_DISTINCT` aggregation function for measure queries, standalone and distributed.
- **Single-node**: a new `pkg/query/aggregation.Distinct` set primitive, wired into the vectorized `BatchAggregation` engine (`AggCountDistinct`), budget-charged per distinct value.
- **Distributed push-down**: "local distinct, global SUM" — each data node computes an exact local distinct count per group and emits it as an ordinary scalar partial; the liaison reduces via the existing `AggSum`/`AggReduceSpec` path unchanged. A new decomposability check (`validateCountDistinctPushdown`, design §7.4) rejects any request where a routing tag (the measure's `sharding_key`, else `entity`) isn't covered by either the GroupBy keys or the Agg target — applied uniformly in standalone and distributed analysis.
- **Multi-stage rejection**: a `RequiresSingleStage()` signal plumbed from the vectorized plan through to `banyand/dquery`, since distinct counts aren't additive across lifecycle stages.
- **Multi-group rejection**: shard ids are scoped per measure group, not globally unique, so cross-group replica-dedup can't distinguish a genuine duplicate from an unrelated partial — multi-group `COUNT_DISTINCT` requests are rejected outright.
- **Test-coverage closeout**: unit and integration coverage across the full tag-role matrix — target/GroupBy covering a single-tag entity, a genuinely composite (multi-tag) entity where each routing component is covered by a *different* branch, and an `index_mode`/`IndexRule`-bound tag, proving indexedness never affects the decomposability decision. Also closes a gap the design doc itself flagged: the layer-3 pairwise test generator hard-coded the five pre-existing aggregation functions and would have silently under-covered `COUNT_DISTINCT` forever.
Two real bugs were found and fixed via adversarial review during this branch's own development (see commit history): a missing shard-id guard in the bucketed map-mode path, and the cross-group shard-id collision above.
## Test plan
- [x] `go test -race ./pkg/query/vectorized/... ./pkg/query/aggregation/... ./banyand/measure/... ./banyand/dquery/...`
- [x] `go test ./test/integration/standalone/query/...` (full suite, green)
- [x] `go test ./test/integration/distributed/query/...` (full suite, green — 2-data-node + liaison cluster, exercises real cross-shard SUM-reduce)
- [x] `make pre-push` (lint, license-check, generator idempotency, vuln-check)
- [x] `codex review --base origin/main` over the full branch diff — no actionable findings on the final pass (two earlier findings fixed within the branch)
--
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] feat(query): add COUNT_DISTINCT push-down for measure queries [skywalking-banyandb]
Posted by "hanahmily (via GitHub)" <gi...@apache.org>.
hanahmily merged PR #1365:
URL: https://github.com/apache/skywalking-banyandb/pull/1365
--
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] feat(query): add COUNT_DISTINCT push-down for measure queries [skywalking-banyandb]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1365:
URL: https://github.com/apache/skywalking-banyandb/pull/1365#issuecomment-5737739057
## [Codecov](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/1365?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 `96.90722%` with `3 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 52.68%. 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 ([`fdf4f25`](https://app.codecov.io/gh/apache/skywalking-banyandb/commit/fdf4f259431f545c5246ebe341def76f5ea705b4?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 444 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/1365?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [pkg/query/vectorized/measure/aggregation.go](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/1365?src=pr&el=tree&filepath=pkg%2Fquery%2Fvectorized%2Fmeasure%2Faggregation.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL3F1ZXJ5L3ZlY3Rvcml6ZWQvbWVhc3VyZS9hZ2dyZWdhdGlvbi5nbw==) | 93.93% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/1365?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [pkg/query/vectorized/measure/plan/distributed.go](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/1365?src=pr&el=tree&filepath=pkg%2Fquery%2Fvectorized%2Fmeasure%2Fplan%2Fdistributed.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL3F1ZXJ5L3ZlY3Rvcml6ZWQvbWVhc3VyZS9wbGFuL2Rpc3RyaWJ1dGVkLmdv) | 93.33% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/1365?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #1365 +/- ##
==========================================
+ Coverage 45.97% 52.68% +6.71%
==========================================
Files 328 296 -32
Lines 55505 30608 -24897
==========================================
- Hits 25520 16127 -9393
+ Misses 27909 12858 -15051
+ Partials 2076 1623 -453
```
| [Flag](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/1365/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/1365/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `60.00% <ø> (?)` | |
| [pkg](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/1365/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `50.79% <96.90%> (?)` | |
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/1365?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
Re: [PR] feat(query): add COUNT_DISTINCT push-down for measure queries [skywalking-banyandb]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #1365:
URL: https://github.com/apache/skywalking-banyandb/pull/1365#discussion_r4051875123
##########
pkg/query/vectorized/measure/aggregation.go:
##########
@@ -590,6 +614,29 @@ func (a *BatchAggregation) fold(b *vectorized.RecordBatch, rowIdx int, slot *agg
v = float64(col.(*vectorized.TypedColumn[int64]).Data()[rowIdx])
}
slot.floatMap.In(v)
+ return nil
+}
+
+// foldDistinct encodes col's value at rowIdx with appendKeyComponent (the
+// same per-type byte encoder BatchAggregation's own group key already
+// uses — it already covers every target type the §6 semantics matrix
+// admits) and inserts it into slot's distinct set. Unlike the fixed-size
+// numeric slots, a distinct set grows with cardinality (design §7.6), so
+// memory is charged per newly-added value here, not once per group in
+// Consume — refunded together with every other charge this operator holds
+// via the shared a.reserved counter in Close.
+func (a *BatchAggregation) foldDistinct(col vectorized.Column, rowIdx int, slot *aggSlot) error {
+ var sb [64]byte
+ encoded := appendKeyComponent(sb[:0], col, rowIdx)
+ if !slot.distinct.In(encoded) {
+ return nil
+ }
+ charge := int64(len(encoded)) + distinctEntryOverhead
+ if reserveErr := a.tracker.Reserve(charge); reserveErr != nil {
+ return fmt.Errorf("aggregation memory budget exceeded: %w", reserveErr)
Review Comment:
`Distinct.In` mutates the set before the memory reservation is attempted. When `Reserve` fails, this newly encoded value remains in the backing map without any charge, so the operator can exceed the configured query budget (and a very large value is allocated before the guard rejects it). Check membership without mutation before reserving, or reserve/rollback through a distinct-set API that only commits the insertion after the charge succeeds.
--
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