You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "SammyUrfen (via GitHub)" <gi...@apache.org> on 2026/09/03 10:27:17 UTC

[PR] chore: remove the row-based query path from measure, stream and trace [skywalking-banyandb]

SammyUrfen opened a new pull request, #1326:
URL: https://github.com/apache/skywalking-banyandb/pull/1326

   Resolves apache/skywalking#13998.
   
   Vectorized query has been the default for measure, stream and trace since 0.11. This removes the row path it replaced: dispatch branches, plan nodes, storage-layer egress, and the tests that existed only to compare the two.
   
   **-7,194 lines** (10,600 deleted, 3,406 added). Non-test Go: -4,886.
   
   | Area | Net | What went |
   | --- | ---: | --- |
   | measure | -2,535 | row query planner, row distributed plan, row fallback in `dquery` |
   | stream | -1,215 | row execution path in dispatch, storage-layer egress (`query_by_ts.go`, `query_by_idx.go`, `tsResult` / `idxResult`) |
   | trace | -800 | row query dispatch, SIDX row streaming pipeline |
   | shared | -267 | `model.StreamResult` / `StreamQueryResult` / `MergeStreamResults` |
   | other | -69 | docs, manifests, harness |
   
   Before the stream storage layer was deleted, `(*stream).Query` and `logicalstream.BuildElementsFromStreamResult` were checked for callers outside tests; there are none. Everything live goes `QueryVectorized → queryVectorized → streamVecScan` over `blockCursor` / `block_scanner.go`, which never touches the row result types.
   
   ## Behaviour changes
   
   Both are in `docs/operation/upgrade.md` under 0.12.
   
   **Stream multi-segment timestamp-order queries can come back under-filled.** A stream query ordered by timestamp, carrying a criteria filter, and spanning more than one segment may return fewer elements than 0.11.x did. The row scan resumed segment by segment — after its first `maxElementSize` batch it kept pulling from later segments until the limit was filled. The vectorized scan doesn't see segment boundaries, so it caps at `maxElementSize` and stops. Results are correct and correctly ordered, just short of the limit. Index-order queries are unaffected; their merge stays uncapped. Clients needing the remainder can narrow the time range or page forward.
   
   **Index-order queries whose ordering tag isn't projected** now stay on the vec path and egress via proto. The scan projects that tag internally to derive its sort key and strips it before egress; the frame egress rebuilds its projection from the batch schema and would leak it. Results unchanged, no raw-wire fast path for that shape.
   
   **Stream slow-query logging is gone.** It lived only on the row path in `banyand/query/processor.go` after `se.Execute`, and vec dispatch returned before reaching it — so at the shipped default it was already not logging for stream. Not a regression, but measure and trace still have it (`:685`, `:848`).
   
   ## Flags
   
   `--measure-vectorized-enabled`, `--stream-vectorized-enabled` and `--trace-vectorized-enabled` stay registered as no-ops. `=false` now fails fast at startup with an error naming this issue; `=true` and omission are accepted. Removing the `BoolVar`s outright breaks node startup wherever the flag is still passed: CI, e2e, soak and the surviving vec integration tests.
   
   ## Tests
   
   Test Go is down 4,087 lines. About 3,300 of that is tests whose subject no longer exists: `model_test.go` (1,245), `trace/streaming_pipeline_test.go` (838), `stream/query_by_idx_test.go` (325) and `query_by_ts_test.go` (266), `stream/benchmark_test.go` (203), and the row measure plan tests (424).
   
   The stream parity tests that compared vec against row were converted to golden fixtures captured from the row path before it was deleted — 1,921 lines of YAML under `banyand/stream/testdata/`. Counting those, effective coverage delta is about -2,166.
   
   `Parity_Order`, `Parity_IndexOrder`, `Parity_Projection` and `Parity_OffsetLimit` went without direct replacements; the 16 `gen_feat_*` goldens in `test/cases/stream/data/want/` cover the same shapes end-to-end. `Parity_Fallback_RowPathCorrect` went with its subject. `banyand/trace/vectorized_bench_test.go` was the row-vs-vec A/B — its vec half measures nothing querybench doesn't.
   
   None of the four captured fixtures is a multi-segment timestamp-order query, so there's no golden for the one case where vec deliberately differs.
   
   The three `test/integration/{standalone,distributed}/query/vectorized_*_test.go` Describes each still boot a second standalone cluster to replay tables `round2.go` already registers against the default one; their counter assertions were kept as they are.
   
   `test/integration/distributed/querybench/` was a row-vs-vec harness. With no engine accepting `mode=row`, the mode dimension collapses to vec-only and `DQB_MODE=row` fails validation with a message naming this issue. Absolute latency and QPS measurement is unchanged; the ratio table is gone.
   
   ## Measurements
   
   `origin/main` and this branch built back-to-back on one machine, `go clean -cache` before each.
   
   Every binary shrank, **-6.81 MB total (-1.35%)**: banyand-backup -1.49%, banyand-restore -1.49%, banyand-server -1.61%, fodc-agent -2.18%, fodc-proxy -2.06%, banyand-lifecycle -0.87%, banyand-migration -0.86%, bydbctl-cli -0.67%. Total 505,891,527 → 499,083,703.
   
   Build wall time is flat: 87.30 s → 86.42 s, single sample each.
   
   ## Test plan
   
   `make generate`, `build`, `lint`, `check`, `license-check` and `pre-push` all pass. `make test-ci` passes across `./banyand/...` (42 suites), `./bydbctl/...` (14), `./pkg/...` (58), `./fodc/...` (22), `./test/integration/standalone/...` (15) and `./test/integration/distributed/...` (13).
   
   Two pre-existing failures, both reproduced on a clean `origin/main` worktree:
   
   - `make test-race` fails `banyand/trace` *Schema Change → querying data should return null for type-mismatched tags* on a 10 s Ginkgo timeout. Same failure on main at 12.5 s; passes under `make test-ci` on both.
   - `make lint-rawgo` fails with 140 violations here against 143 on main. One baseline entry pointed at `banyand/stream/query_by_idx.go:139`, which this PR deletes. That line was removed by hand rather than regenerating the file, so the baseline only shrinks (263 → 262).
   
   ---
   
   - [x] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Resolves apache/skywalking#13998.
   - [x] Update the [`CHANGES` log](https://github.com/apache/skywalking-banyandb/blob/main/CHANGES.md).
   


-- 
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] chore: remove the row-based query path from measure, stream and trace [skywalking-banyandb]

Posted by "hanahmily (via GitHub)" <gi...@apache.org>.
hanahmily commented on PR #1326:
URL: https://github.com/apache/skywalking-banyandb/pull/1326#issuecomment-5563000311

   I believe the practical way is to first delete all row-based implementations in the query package(pkg/query), then fix all the compile failures. This will ensure that row-based implementations are totally removed. 


-- 
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] chore: remove the row-based query path from measure, stream and trace [skywalking-banyandb]

Posted by "hanahmily (via GitHub)" <gi...@apache.org>.
hanahmily commented on PR #1326:
URL: https://github.com/apache/skywalking-banyandb/pull/1326#issuecomment-5617136949

   We don't need the lint guard in this PR; human review is sufficient to prevent reintroducing the row path. Please remove the guard and its mentions from the PR description. Also, please resolve the conflicts with main after #1331 and rerun CI.


-- 
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] chore: remove the row-based query path from measure, stream and trace [skywalking-banyandb]

Posted by "hanahmily (via GitHub)" <gi...@apache.org>.
hanahmily merged PR #1326:
URL: https://github.com/apache/skywalking-banyandb/pull/1326


-- 
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] chore: remove the row-based query path from measure, stream and trace [skywalking-banyandb]

Posted by "SammyUrfen (via GitHub)" <gi...@apache.org>.
SammyUrfen commented on PR #1326:
URL: https://github.com/apache/skywalking-banyandb/pull/1326#issuecomment-5585941542

   Updated the description: both references are `Related to` now, and it says what this covers and what it does not. The lint guard is in `scripts/lint/rowcallsites/` and runs as part of `make lint`. It counts sites per file instead of by line number so unrelated edits do not break the build, and the four that remain each carry a note on why. The multi-block Top test is `banyand/measure/query_batch_top_test.go`; if the row fallback is forced the test fails, so it checks the path and not only the result. A stream query the vec engine cannot plan now reports the reason, and I fixed a case where it reported the wrong one for multi-group plans.
   
   I also corrected two things in the description. The binary size numbers were wrong, so I rebuilt both branches from a clean cache and used the measured values. The slow query line references were wrong too: both of them are measure, not measure and trace. The four seams are unchanged.
   


-- 
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] chore: remove the row-based query path from measure, stream and trace [skywalking-banyandb]

Posted by "SammyUrfen (via GitHub)" <gi...@apache.org>.
SammyUrfen commented on PR #1326:
URL: https://github.com/apache/skywalking-banyandb/pull/1326#issuecomment-5628837708

   The lint guard and its mentions in the PR description have been fully removed. The conflicts resulting from the #1331 merge have also been reconciled, and the CI is green again locally.


-- 
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] chore: remove the row-based query path from measure, stream and trace [skywalking-banyandb]

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1326:
URL: https://github.com/apache/skywalking-banyandb/pull/1326#issuecomment-5524700943

   ## [Codecov](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/1326?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 59.94%. 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 ([`b05fafb`](https://app.codecov.io/gh/apache/skywalking-banyandb/commit/b05fafb40515134d1486fa47996c3b687f111740?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
   :warning: Report is 415 commits behind head on main.
   
   <details><summary>Additional details and impacted files</summary>
   
   
   
   ```diff
   @@             Coverage Diff             @@
   ##             main    #1326       +/-   ##
   ===========================================
   + Coverage   45.97%   59.94%   +13.96%     
   ===========================================
     Files         328       35      -293     
     Lines       55505     6259    -49246     
   ===========================================
   - Hits        25520     3752    -21768     
   + Misses      27909     2285    -25624     
   + Partials     2076      222     -1854     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/1326/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/1326/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `59.94% <ø> (?)` | |
   
   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/1326?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] chore: remove the row-based query path from measure, stream and trace [skywalking-banyandb]

Posted by "hanahmily (via GitHub)" <gi...@apache.org>.
hanahmily commented on code in PR #1326:
URL: https://github.com/apache/skywalking-banyandb/pull/1326#discussion_r3945560821


##########
banyand/query/processor.go:
##########
@@ -233,7 +213,7 @@ func (p *streamQueryProcessor) tryStreamVecDispatch(ctx context.Context, plan lo
 	// so the send path passes it through and the liaison decodes it. The liaison
 	// applies the global offset/limit slice, so the data node emits the whole
 	// (already per-node-capped) batch set — no slice here.
-	if !hasFilter && streamVecEmitAsFrame(p.distributed, data.StreamWireModeRaw(), traced) {
+	if !hasFilter && !vecExec.HidesOrderTag() && streamVecEmitAsFrame(p.distributed, data.StreamWireModeRaw(), traced) {

Review Comment:
   **[P1] This gate leaves criteria and hidden-order-tag queries on an element/row path.** Below this branch, batches are materialized with `BuildElementsFromBatches`, criteria are evaluated one `Element` at a time, and hidden tags are stripped per element. Please implement filtering/projection on `RecordBatch` columns and materialize protobuf only at the external API boundary.



##########
banyand/query/processor.go:
##########
@@ -452,107 +425,65 @@ func executeMeasurePlan(
 	mctx *measureExecutionContext,
 	emitPartial bool,
 ) (executor.MIterator, string, error) {
-	if mit, planStr, handled, dispatchErr := tryVecDispatch(ctx, queryCriteria, mctx, emitPartial); dispatchErr != nil {
+	mit, planStr, dispatchErr := dispatchMeasure(ctx, queryCriteria, mctx, emitPartial)
+	if dispatchErr != nil {
 		return nil, "", fmt.Errorf("fail to dispatch the query request for measure %s: %w", queryCriteria.GetName(), dispatchErr)
-	} else if handled {
-		if e := mctx.ml.Debug(); e.Enabled() {
-			e.Str("plan", planStr).Msg("vec query plan")
-		}
-		return mit, planStr, nil
-	}
-
-	// nolint:staticcheck // SA1019 — row-path Analyze is the only production path until G8 ships.
-	plan, planErr := logical_measure.Analyze(queryCriteria, mctx.metadata, mctx.schemas, mctx.ecc, emitPartial)
-	if planErr != nil {
-		return nil, "", fmt.Errorf("fail to analyze the query request for measure %s: %w", queryCriteria.GetName(), planErr)
 	}
 	if e := mctx.ml.Debug(); e.Enabled() {
-		e.Str("plan", plan.String()).Msg("query plan")
-	}
-	mIterator, execErr := plan.(executor.MeasureExecutable).Execute(ctx)
-	if execErr != nil {
-		mctx.ml.Error().Err(execErr).RawJSON("req", logger.Proto(queryCriteria)).Msg("fail to query")
-		return nil, "", fmt.Errorf("fail to execute the query plan for measure %s: %w", queryCriteria.GetName(), execErr)
+		e.Str("plan", planStr).Msg("vec query plan")
 	}
-	return mIterator, plan.String(), nil
+	return mit, planStr, nil
 }
 
-// tryVecDispatch is the thin adapter from measureExecutionContext into
+// dispatchMeasure is the thin adapter from measureExecutionContext into
 // the vec dispatch inputs. Multi-measure queries (len(ecc) > 1) are
-// handled per-group: each group is dispatched to vec individually and
-// the per-group iterators are merged through the row path's exact
-// cross-group ordering via logical_measure.MergeGroupMIterators (G9f.1,
-// reusing the row sortableDataPoints + sortedMIterator stack so the
-// cross-group order and version dedup are reproduced by construction,
-// not by parallel reimplementation). If any group's executor context is
-// not vec-capable, or any group's dispatch declines to handle the
-// request, the whole multi-measure request falls through to the row
-// path — mixing vec for some groups with row for others would produce a
-// merged result the row path cannot validate.
+// handled per-group: each group is dispatched individually and the
+// per-group iterators are merged through the shared cross-group ordering
+// stack in logical_measure.MergeGroupMIterators (G9f.1, reusing
+// sortableDataPoints + sortedMIterator so the cross-group order and
+// version dedup are reproduced by construction, not by parallel
+// reimplementation).
 //
 // Distributed Map-mode GroupBy+Agg (emitPartial=true) routes to vec via
 // AggModeMap (G9f.2): vecplan.Dispatch receives emitPartial and the
 // BatchAggregation operator emits typed-column partials. As of G9f.4 the
 // data-node side handles distributed Top-over-Agg too — the vec plan emits
 // Scan → GroupByAgg(Map) → Top → Limit per node, BatchTop sorts on the
-// partial value column, and the row-path liaison's distributedPlan dedupes
-// + applies the global Top across the per-node partial top-Ns (mirrors the
-// row path's two-pass distributed Top approach; pushDownAgg is set whenever
-// Agg != nil, see measure_analyzer.go:179, and Top is applied AFTER
-// aggregation at :205-207).
-func tryVecDispatch(
+// partial value column, and the liaison's distributed plan dedupes and
+// applies the global Top across the per-node partial top-Ns.
+func dispatchMeasure(
 	ctx context.Context,
 	queryCriteria *measurev1.QueryRequest,
 	mctx *measureExecutionContext,
 	emitPartial bool,
-) (executor.MIterator, string, bool, error) {
-	// Empty execution context = no measures to query. Fall through to row
-	// in both flag states — the row path handles the degenerate empty
-	// case identically (returns an empty MIterator). This is NOT a vec
-	// fall-through under flag-on (we never entered vec); it is the
-	// "no work to do" exit.
-	if len(mctx.ecc) == 0 {
-		return nil, "", false, nil
-	}
+) (executor.MIterator, string, error) {
 	vecs := make([]vecExecutionContext, len(mctx.ecc))
-	flagOn := false
 	for groupIdx, ec := range mctx.ecc {
 		vec, ok := ec.(vecExecutionContext)
 		if !ok {
-			// A non-vec execution context can legitimately appear only on
-			// the flag-off rollback path (production storage satisfies
-			// vecExecutionContext). If ANY group's executor lacks the
-			// capability and the cluster is flag-on, that is a botched
-			// rollout and must fail loud — no proto/row fall-through.
-			if flagOn {
-				return nil, "", true, fmt.Errorf("vec dispatch: group %d execution context not vec-capable under flag-on (rollout skew?)", groupIdx)
-			}
-			return nil, "", false, nil
+			// Production storage always satisfies vecExecutionContext. A
+			// group that does not is a botched rollout and must fail loud.
+			return nil, "", fmt.Errorf("vec dispatch: group %d execution context not vec-capable (rollout skew?)", groupIdx)
 		}
 		vecs[groupIdx] = vec
-		// Detect flag-on as soon as the first vec config is observable.
-		// Per-process flag means every group's config agrees in practice.
-		if groupIdx == 0 && vec.VectorizedConfig().Enabled {
-			flagOn = true
-		}
 	}
 	if len(mctx.ecc) == 1 {
 		return vecplan.Dispatch(ctx, queryCriteria, mctx.metadata[0], vecs[0].GetSchema(),
 			mctx.schemas[0], vecs[0], vecs[0].VectorizedConfig(), emitPartial, false)
 	}
 	// Multi-measure projection validation: a tag/field is valid if it
-	// resolves in ANY group's schema (mirrors measure_analyzer.Analyze's
-	// mergeSchema(ss) union). Running the per-group validation inside
-	// Dispatch would reject the schema-evolution case where one group
-	// added a tag/field the others lack (multi_group_new_tag_field
-	// integration fixture). We pre-validate here against the union once,
-	// then skip the per-group validation inside Dispatch.
+	// resolves in ANY group's schema (mirrors the mergeSchema(ss) union).
+	// Running the per-group validation inside Dispatch would reject the
+	// schema-evolution case where one group added a tag/field the others
+	// lack (multi_group_new_tag_field integration fixture). We pre-validate
+	// here against the union once, then skip the per-group validation
+	// inside Dispatch.

Review Comment:
   **[P1] Union validation does not make schema-evolved multi-group aggregates executable.** The distributed path intersects projections per group, producing differently shaped partial frames, but `ReduceRawFrames/ReducePartialBatches` takes the first frame schema as canonical and rejects later structural mismatches. A projected tag present only in one group can therefore fail with `partial N schema mismatch`. Please normalize/null-fill to the merged schema before reduction and add an old-schema/new-schema GroupBy+SUM test.



##########
banyand/query/processor.go:
##########
@@ -452,107 +425,65 @@ func executeMeasurePlan(
 	mctx *measureExecutionContext,
 	emitPartial bool,
 ) (executor.MIterator, string, error) {
-	if mit, planStr, handled, dispatchErr := tryVecDispatch(ctx, queryCriteria, mctx, emitPartial); dispatchErr != nil {
+	mit, planStr, dispatchErr := dispatchMeasure(ctx, queryCriteria, mctx, emitPartial)
+	if dispatchErr != nil {
 		return nil, "", fmt.Errorf("fail to dispatch the query request for measure %s: %w", queryCriteria.GetName(), dispatchErr)
-	} else if handled {
-		if e := mctx.ml.Debug(); e.Enabled() {
-			e.Str("plan", planStr).Msg("vec query plan")
-		}
-		return mit, planStr, nil
-	}
-
-	// nolint:staticcheck // SA1019 — row-path Analyze is the only production path until G8 ships.
-	plan, planErr := logical_measure.Analyze(queryCriteria, mctx.metadata, mctx.schemas, mctx.ecc, emitPartial)
-	if planErr != nil {
-		return nil, "", fmt.Errorf("fail to analyze the query request for measure %s: %w", queryCriteria.GetName(), planErr)
 	}
 	if e := mctx.ml.Debug(); e.Enabled() {
-		e.Str("plan", plan.String()).Msg("query plan")
-	}
-	mIterator, execErr := plan.(executor.MeasureExecutable).Execute(ctx)
-	if execErr != nil {
-		mctx.ml.Error().Err(execErr).RawJSON("req", logger.Proto(queryCriteria)).Msg("fail to query")
-		return nil, "", fmt.Errorf("fail to execute the query plan for measure %s: %w", queryCriteria.GetName(), execErr)
+		e.Str("plan", planStr).Msg("vec query plan")
 	}
-	return mIterator, plan.String(), nil
+	return mit, planStr, nil
 }
 
-// tryVecDispatch is the thin adapter from measureExecutionContext into
+// dispatchMeasure is the thin adapter from measureExecutionContext into
 // the vec dispatch inputs. Multi-measure queries (len(ecc) > 1) are
-// handled per-group: each group is dispatched to vec individually and
-// the per-group iterators are merged through the row path's exact
-// cross-group ordering via logical_measure.MergeGroupMIterators (G9f.1,
-// reusing the row sortableDataPoints + sortedMIterator stack so the
-// cross-group order and version dedup are reproduced by construction,
-// not by parallel reimplementation). If any group's executor context is
-// not vec-capable, or any group's dispatch declines to handle the
-// request, the whole multi-measure request falls through to the row
-// path — mixing vec for some groups with row for others would produce a
-// merged result the row path cannot validate.
+// handled per-group: each group is dispatched individually and the
+// per-group iterators are merged through the shared cross-group ordering
+// stack in logical_measure.MergeGroupMIterators (G9f.1, reusing
+// sortableDataPoints + sortedMIterator so the cross-group order and
+// version dedup are reproduced by construction, not by parallel
+// reimplementation).

Review Comment:
   **[P1] Multi-group Measure still reconstructs the legacy row merger.** `MergeGroupMIterators` wraps the per-group outputs in `sortableDataPoints -> sortedMIterator`, operating on individual `InternalDataPoint` values; raw-frame egress later drains `Next/Current` and serializes those rows back into a frame. Please replace this with batch-native cross-group ordering/deduplication and remove the retained row stack.



##########
banyand/query/processor.go:
##########
@@ -152,52 +152,32 @@ func (p *streamQueryProcessor) Rev(ctx context.Context, message bus.Message) (re
 		}()
 	}
 
-	// Vec dispatch: attempt the native columnar path before the row execution.
-	// Gated on the engine flag (VectorizedConfig().Enabled). The plan must expose
-	// StreamVecExecutable (single localIndexScan, no tag filter / multi-group /
-	// skipping filter) — otherwise the assertion fails and we fall through to the
-	// row path below. When tracing is on we MUST return a proto QueryResponse (it
-	// carries common.v1.Trace); the frame emit is gated on tracer == nil.
-	if p.streamService.VectorizedConfig().Enabled {
-		if handled, vecResp := p.tryStreamVecDispatch(ctx, plan, queryCriteria, tracer != nil); handled {
-			resp = vecResp
-			se := plan.(executor.StreamExecutable)
-			se.Close()
-			return
-		}
-	}
-
-	se := plan.(executor.StreamExecutable)
-	defer se.Close()
-	entities, err := se.Execute(ctx)
-	if err != nil {
-		p.log.Error().Err(err).RawJSON("req", logger.Proto(queryCriteria)).Msg("fail to execute the query plan")
-		resp = bus.NewMessage(bus.MessageID(now), common.NewError("execute the query plan for stream %s: %v", queryCriteria.GetName(), err))
+	// Vec dispatch is the only stream execution path; the row path was removed in
+	// 0.12.0. Every analyzed plan shape must therefore be vec-eligible, so a decline

Review Comment:
   **[P1] The row path is not fully removed yet.** `banyand/measure/query_batch.go:88-115` still routes ordinary Top queries spanning multiple blocks through `qr.merge` (the row `MeasureResult`/postprocessor path) and then converts the result back into a batch. Please remove that runtime fallback before making this unconditional claim, and add a multi-block Top regression test that proves `PullBatch` stays batch-native.



##########
banyand/query/processor.go:
##########
@@ -452,107 +425,65 @@ func executeMeasurePlan(
 	mctx *measureExecutionContext,
 	emitPartial bool,
 ) (executor.MIterator, string, error) {
-	if mit, planStr, handled, dispatchErr := tryVecDispatch(ctx, queryCriteria, mctx, emitPartial); dispatchErr != nil {
+	mit, planStr, dispatchErr := dispatchMeasure(ctx, queryCriteria, mctx, emitPartial)
+	if dispatchErr != nil {
 		return nil, "", fmt.Errorf("fail to dispatch the query request for measure %s: %w", queryCriteria.GetName(), dispatchErr)
-	} else if handled {
-		if e := mctx.ml.Debug(); e.Enabled() {
-			e.Str("plan", planStr).Msg("vec query plan")
-		}
-		return mit, planStr, nil
-	}
-
-	// nolint:staticcheck // SA1019 — row-path Analyze is the only production path until G8 ships.
-	plan, planErr := logical_measure.Analyze(queryCriteria, mctx.metadata, mctx.schemas, mctx.ecc, emitPartial)
-	if planErr != nil {
-		return nil, "", fmt.Errorf("fail to analyze the query request for measure %s: %w", queryCriteria.GetName(), planErr)
 	}
 	if e := mctx.ml.Debug(); e.Enabled() {
-		e.Str("plan", plan.String()).Msg("query plan")
-	}
-	mIterator, execErr := plan.(executor.MeasureExecutable).Execute(ctx)
-	if execErr != nil {
-		mctx.ml.Error().Err(execErr).RawJSON("req", logger.Proto(queryCriteria)).Msg("fail to query")
-		return nil, "", fmt.Errorf("fail to execute the query plan for measure %s: %w", queryCriteria.GetName(), execErr)
+		e.Str("plan", planStr).Msg("vec query plan")
 	}
-	return mIterator, plan.String(), nil
+	return mit, planStr, nil
 }
 
-// tryVecDispatch is the thin adapter from measureExecutionContext into
+// dispatchMeasure is the thin adapter from measureExecutionContext into
 // the vec dispatch inputs. Multi-measure queries (len(ecc) > 1) are
-// handled per-group: each group is dispatched to vec individually and
-// the per-group iterators are merged through the row path's exact
-// cross-group ordering via logical_measure.MergeGroupMIterators (G9f.1,
-// reusing the row sortableDataPoints + sortedMIterator stack so the
-// cross-group order and version dedup are reproduced by construction,
-// not by parallel reimplementation). If any group's executor context is
-// not vec-capable, or any group's dispatch declines to handle the
-// request, the whole multi-measure request falls through to the row
-// path — mixing vec for some groups with row for others would produce a
-// merged result the row path cannot validate.
+// handled per-group: each group is dispatched individually and the
+// per-group iterators are merged through the shared cross-group ordering
+// stack in logical_measure.MergeGroupMIterators (G9f.1, reusing
+// sortableDataPoints + sortedMIterator so the cross-group order and
+// version dedup are reproduced by construction, not by parallel
+// reimplementation).
 //
 // Distributed Map-mode GroupBy+Agg (emitPartial=true) routes to vec via
 // AggModeMap (G9f.2): vecplan.Dispatch receives emitPartial and the
 // BatchAggregation operator emits typed-column partials. As of G9f.4 the
 // data-node side handles distributed Top-over-Agg too — the vec plan emits
 // Scan → GroupByAgg(Map) → Top → Limit per node, BatchTop sorts on the
-// partial value column, and the row-path liaison's distributedPlan dedupes
-// + applies the global Top across the per-node partial top-Ns (mirrors the
-// row path's two-pass distributed Top approach; pushDownAgg is set whenever
-// Agg != nil, see measure_analyzer.go:179, and Top is applied AFTER
-// aggregation at :205-207).
-func tryVecDispatch(
+// partial value column, and the liaison's distributed plan dedupes and
+// applies the global Top across the per-node partial top-Ns.

Review Comment:
   **[P1] Preserve GroupBy+Top operator semantics.** The new distributed reducer in `pkg/query/vectorized/measure/plan/distributed.go:725-760` (and multi-group `:818-835`) applies GroupBy-first and then Top, while the removed planner applied Top before GroupBy. When the ranking field varies within a group, this can select a different winning group. Please restore the prior ordering or explicitly specify the behavior change, and add single-/multi-group fixtures where the Top field varies within a group.



-- 
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] chore: remove the row-based query path from measure, stream and trace [skywalking-banyandb]

Posted by "SammyUrfen (via GitHub)" <gi...@apache.org>.
SammyUrfen commented on PR #1326:
URL: https://github.com/apache/skywalking-banyandb/pull/1326#issuecomment-5570358269

   Two things the PR body should have said:
   
   - Removing the flags removes the rollback lever, so the multi-segment under-fill is no longer avoidable by config. `upgrade.md` has the mechanism, the body doesn't.
   - A vec-declined stream plan is now a hard error instead of falling through to row. One shape still reaches it: an order-by on a tag that no longer resolves. I'll fix the error message and document it.
   
   Two corrections:
   
   - `query_batch.go:92` is gated on `topNQueryOptions`, set only when the measure is `_top_n_result` (`query.go:299`). `req.Top` goes `analyzer.go:145` -> `plan/top.go` -> `NewBatchTop` and never reaches `qr.merge`. I'll add the multi-block Top test. `qr.merge` stays a row call site either way, since `Pull()` is held by `topn_plan_localscan.go:201`.
   - `distributed.go:731`: `measure_analyzer.go:130` wraps GroupBy and `:146` wraps Top above it, so row was also GroupBy-then-Top. The divergence is that row GroupBy returned the whole group while `NewBatchGroupByFirst` keeps one. `query.proto:108-136` specifies no order between them.
   
   The other four are real. Five of the six sites are byte-identical to `v0.11.0` — this PR removes the row fallback under them rather than changing them. The exception is the criteria/hidden-order-tag gate, where `HidesOrderTag` is mine.
   
   Three ways to go, your call:
   
   **A.** Land this narrowed to the data-node engines, four seams as issues, plus a shrink-only ratchet in `make lint` pinning the four `.Pull()` sites — same shape as `lint-rawgo`. ~250 lines.
   
   **B.** All of `pkg/query` in this PR. 12-18k lines across four subsystems. `MIterator` still survives, since traced queries return proto.
   
   **C.** Columnar filtering/projection and the batch-native cross-group merge here, the schema-evolved reduce and the distributed coordinator as issues. ~1,200 lines, needs #1331 first.
   
   I'd go with A. Either way #1331 adds the columnar pre-merge tag filter for stream and should land first.
   


-- 
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] chore: remove the row-based query path from measure, stream and trace [skywalking-banyandb]

Posted by "hanahmily (via GitHub)" <gi...@apache.org>.
hanahmily commented on PR #1326:
URL: https://github.com/apache/skywalking-banyandb/pull/1326#issuecomment-5578747474

   Option A sounds good to me: keep this PR scoped to the data-node engines, track the remaining work in follow-up issues, and add the shrink-only lint guard.
   
   Please also replace both occurrences of `Resolves apache/skywalking#13998` in the PR description (the opening line and the checklist) with a non-closing reference such as `Related to apache/skywalking#13998`. With the narrower scope, this PR only completes part of the original issue, so merging it should not automatically close that issue. Please update the description to make the reduced scope and remaining work explicit.


-- 
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] chore: remove the row-based query path from measure, stream and trace [skywalking-banyandb]

Posted by "SammyUrfen (via GitHub)" <gi...@apache.org>.
SammyUrfen commented on PR #1326:
URL: https://github.com/apache/skywalking-banyandb/pull/1326#issuecomment-5689917849

   Conflicts resolved against main, in two commits so you can take them separately.
   
   The merge commit is mechanical, with one exception. `#1334` bounded the row scan allocations and this PR deletes both files it touched. The vectorized path already calls `query.Charge`, so the block budget stays. `banyand/stream/query_capacity.go` goes too, because `unused` fails `make lint` otherwise.
   
   The second commit drops the `engine` matrix axis from the e2e workflow. With one engine left both arms run the same code.
   
   Also stripped `--*-vectorized-enabled=false` from the new lifecycle and rover files.
   


-- 
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] chore: remove the row-based query path from measure, stream and trace [skywalking-banyandb]

Posted by "hanahmily (via GitHub)" <gi...@apache.org>.
hanahmily commented on PR #1326:
URL: https://github.com/apache/skywalking-banyandb/pull/1326#issuecomment-5681271598

   @SammyUrfen could you pls fix these conflicts?


-- 
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