Skip to content

fix: Reduce usage of skipVectorize and fix revealed bugs. - #20466

Open
gianm wants to merge 2 commits into
apache:masterfrom
gianm:tests-skip-vectorize
Open

gianm wants to merge 2 commits into
apache:masterfrom
gianm:tests-skip-vectorize

Conversation

@gianm

@gianm gianm commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

The skipVectorize parameter is inherently susceptible to masking bugs, because it skips vectorization tests completely rather than asserting some specific problem. This patch moves most usages of skipVectorize to cannotVectorize, where appropriate, or else deals with them some other way.

Production changes:

  1. The native Scan and TopN implementations are updated to always throw
    when "vectorize: force" is used. The same change is made to the MSQ
    Scan implementation when it runs over an input channel. These changes
    aid in writing test cases and are also arguably more correct behavior.

  2. Subquery errors were lost in ClientQuerySegmentWalker when
    maxSubqueryBytes was set. This is because the groupBy, topN, and
    timeseries toolchests would run the subquery in resultsAsFrames, and
    all exceptions were caught and suppressed. Now, the code is
    restructured to avoid catching errors unnecessarily.

  3. Scan subquery results were not previously closed properly, due to
    a closeable iterator being passed to Sequences.simple. This is fixed
    by migrating the Iterator to a Sequence itself.

  4. Decoupled + preplanned Dart queries ignored query context when
    building stages. In the context of this patch, this had caused
    the "vectorize" context parameter to be lost.

As a result of (4), previously decoupled Dart tests were never actually testing vectorization. Fixing the bug revealed that many of them cannot vectorize due to adding extra stages beyond what is necessary. To avoid needing to deal with that, the decoupled Dart tests are updated to explicitly skip vectorization.

@github-actions github-actions Bot added Area - Documentation Area - Batch Ingestion Area - Querying Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 labels Oct 1, 2026
The skipVectorize parameter is inherently susceptible to masking bugs,
because it skips vectorization tests completely rather than asserting
some specific problem. This patch moves most usages of skipVectorize to
cannotVectorize, where appropriate, or else deals with them some other
way.

Production changes:

1) The native Scan and TopN implementations are updated to always throw
   when "vectorize: force" is used. The same change is made to the MSQ
   Scan implementation when it runs over an input channel. These changes
   aid in writing test cases and are also arguably more correct behavior.

2) Subquery errors were lost in ClientQuerySegmentWalker when
   maxSubqueryBytes was set. This is because the groupBy, topN, and
   timeseries toolchests would run the subquery in resultsAsFrames, and
   all exceptions were caught and suppressed. Now, the code is
   restructured to avoid catching errors unnecessarily.

3) Scan subquery results were not previously closed properly, due to
   a closeable iterator being passed to Sequences.simple. This is fixed
   by migrating the Iterator to a Sequence itself.

4) Decoupled + preplanned Dart queries ignored query context when
   building stages. In the context of this patch, this had caused
   the "vectorize" context parameter to be lost.

As a result of (3), previously decoupled Dart tests were never actually
testing vectorization. Fixing the bug revealed that many of them
cannot vectorize due to adding extra stages beyond what is necessary.
To avoid needing to deal with that, the decoupled Dart tests are updated
to explicitly skip vectorization.
@gianm
gianm force-pushed the tests-skip-vectorize branch from fcacaad to 1f01577 Compare October 1, 2026 07:13

@FrankChen021 FrankChen021 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

Recommendation: merge the PR; no high-confidence findings warrant blocking or an inline review comment.

Concrete merge implication: I found no PR-caused correctness, regression, compatibility, lifecycle/concurrency, security, data-loss, or missing-test risk that should prevent merging. The changed behavior is internally consistent with the documented forced-vectorization semantics, query-context propagation, subquery error propagation, and scan-frame resource cleanup.

Coverage: reviewed all 70 changed files in the complete merge-base diff, including the processing, multi-stage-query, server, documentation, and test/resource changes, plus relevant surrounding implementations and tests in the supplied worktree.

Validation: static inspection of the supplied packet, diff, AGENTS.md, and worktree code; git diff --check f45885a53b1a16d6930caa30b4077d05e25a5a86..1f015774d26a80644f88c9e485c35408010ed7c2 passed. No builds, tests, installs, or formatters were run per the review instructions.


This is an automated review by Codex GPT-5.6-Luna(max)

@FrankChen021 FrankChen021 added the Upgrade note Behavior change that requires an upgrade note label Oct 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area - Batch Ingestion Area - Documentation Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 Area - Querying Upgrade note Behavior change that requires an upgrade note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants