Skip to content

fix(colexec): short-circuit flow-control expressions#25742

Open
LeftHandCold wants to merge 8 commits into
matrixorigin:mainfrom
LeftHandCold:agent/fix-flow-control-short-circuit
Open

fix(colexec): short-circuit flow-control expressions#25742
LeftHandCold wants to merge 8 commits into
matrixorigin:mainfrom
LeftHandCold:agent/fix-flow-control-short-circuit

Conversation

@LeftHandCold

@LeftHandCold LeftHandCold commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • BUG
  • Improvement
  • Feature
  • Documentation
  • Test and CI
  • Code Refactoring
  • Performance
  • Build and Development Tool
  • Other (please describe)

Which issue(s) this PR fixes:

Fixes #25314

What this PR does / why we need it:

MatrixOne eagerly evaluated children of SQL flow-control expressions. An error from a branch that SQL semantics should skip could therefore fail the statement, for example IF(0, CAST('bad' AS SIGNED), 7) or IF(FALSE, @missing_user_variable, 'ok').

This PR:

  • keeps IF, CASE, and COALESCE branch selection lazy for every row;
  • makes all-false masks suppress function, parameter, and variable execution, while keeping skipped placeholders separate from resolved-value caches across executor reuse;
  • evaluates partially selected generic functions on compacted selected rows and scatters typed results back, so legacy kernels cannot fail or perform side effects on masked rows;
  • preserves CASE first-match semantics, COALESCE left-to-right semantics, runtime result metadata, and reusable executor state;
  • restores constant folding with lazy-aware dependency traversal: only conditions and the selected constant branch are folded, while skipped expressions remain unevaluated;
  • keeps selection and compact/scatter buffers bounded by the executor batch-size high-water mark.

No goroutines, waits, or independently owned external resources are introduced. Temporary folding placeholders and reusable vectors have explicit executor ownership and are released by Free.

Validation:

Executed locally on macOS with the repository CGo libraries configured:

go test -run '^TestFlowControlShortCircuitInvalidCast$' -count=20 -timeout 180s ./pkg/sql/colexec
go test -race -run '^TestFlowControlShortCircuitInvalidCast$' -count=5 -timeout 300s ./pkg/sql/colexec
go test -count=1 -timeout 300s ./pkg/sql/colexec ./pkg/sql/plan/function ./pkg/sql/colexec/projection
go test -run '^$' -count=1 -timeout 300s ./pkg/sql/compile
go vet ./pkg/sql/colexec ./pkg/sql/plan/function
go build ./pkg/sql/colexec/... ./pkg/sql/plan/function
git diff --check

The retained 8192-row constant-flow benchmark reports 3.91-3.93 ns/op, 0 B/op, and 0 allocs/op after warm-up. The same expression on the previous PR head was about 42 us/op with 4 allocs/op locally.

Regression coverage includes constant and row-dependent IF/CASE/COALESCE/IFNULL paths, direct variable and prepared-parameter leaves, selected errors, executor reuse and prepared-statement reset, no-ELSE/all-NULL folding, partial batches, runtime metadata, bounded buffers, and variable-length results. A distributed SQL case covers the four original issue expressions.

@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There is a deterministic reuse panic in the new lazy branch path.

EvalIff and EvalCase retain selectList1/selectList2 at the largest batch size ever seen, but pass the entire retained slices to child executors (for example, lines 540/544 and 562/578) instead of [:rowCount]. The new all-rows-skipped fast path makes this unsafe: a function child skipped for a large batch returns from makeNullResult before initializing its own FunctionSelectList buffer. If the same expression executor is then reused for a smaller batch and that branch becomes selected, the child allocates its selection buffer to the smaller rowCount, while the final loop at lines 703-704 iterates the parent's longer retained mask and writes past the child buffer.

A deterministic reproduction is to reuse one non-foldable IF(condition_column, CAST(value_column AS SIGNED), 7) executor: first evaluate N rows with the condition false for every row, then evaluate M<N rows with the condition true. The second evaluation panics with an index-out-of-range. CASE has the same retained-mask forwarding pattern. Before this PR, the eagerly evaluated child initialized its mask on the first large batch, so the new early return is what exposes the stale-shape mismatch.

Please slice every internal branch/condition mask passed to a child to the current rowCount (as EvalCoalesce already does with expr.selectList1[:rowCount]) and add a table-driven IF/CASE regression that flips the selected branch while shrinking the batch on the same executor. Also exercise a subsequent reuse to prove the executor remains healthy. This is deterministic and does not require sleeps or shape-specific production hooks.

I found no additional blocking resource/performance issue: selection buffers remain bounded by the historical maximum batch size, result/child ownership is released by Free, and there are no new waits or goroutines. Existing targeted tests repeated 20 times, the colexec/function/projection suites, go build, and go vet all pass; current CI is green, but none of those tests covers this generation/shape transition.

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The previous retained-mask reuse panic is fixed by slicing IF/CASE masks to the current row count, and the new shrinking-batch regression is deterministic. However, partial-row short-circuiting is still function-specific rather than an executor invariant.

A deterministic counterexample is:

IF(condition_column, CAST(value_column AS BOOL), FALSE)
condition = [false, true]
value     = ["bad", "true"]
expected  = [false, true]

On this head, evaluation fails with invalid input: 'bad' is not a valid bool expression. EvalIff correctly sends the true-branch mask [false, true], but this PR only teaches strToSigned to honor FunctionSelectList; strToBool (and many other fallible cast helpers) still parses skipped rows. CASE and COALESCE have the same underlying gap for mixed batches.

This is the same flow-control correctness contract, not an unrelated expansion: an unselected branch must not execute merely because its expression uses a cast overload other than varchar-to-signed. The all-rows-skipped fast path does not cover a mixed batch.

Please enforce skipped-row handling consistently at the central cast/executor boundary that owns this contract, rather than patching one conversion at a time. Add a deterministic table-driven partial-selection regression with representative fallible cast families plus a control proving that an invalid selected row still errors. The existing branch-flip/shrinking/reuse test should remain; no sleeps or short timing guards are needed.

I found no new leak, hang, or unbounded-growth issue. The masks remain bounded by the historical maximum batch size, ownership still closes through Free, and the focused test passes under -race.

@LeftHandCold
LeftHandCold requested a review from XuPeng-SH July 17, 2026 03:40
@LeftHandCold

Copy link
Copy Markdown
Contributor Author

Addressed review 4722460584 in commit 0e1ab37.

The finding is production-reachable, not a theoretical edge case. The compact path evaluated selected rows into a separate function result, then scattered only values and NULLs into the outer result. Runtime-refined metadata was left behind on the compact result. This made partial evaluation differ from full evaluation, for example:

  • FLOAT32(10,2) to FLOAT64 lost Width and Scale.
  • SYSDATE(3), when consumed by a nested CHAR cast under IF, used the declared timestamp scale 6 and produced a 26-character value instead of 23.

The fix now:

  • keeps the declared return type as the stable per-evaluation baseline;
  • restores that baseline before full, compact, all-NULL, and constant-fold evaluations, preventing metadata from a previous batch from leaking through executor reuse;
  • copies the complete runtime types.Type and vector binary marker from the compact result before scattering;
  • updates the reusable NULL placeholder to the same runtime metadata.

Regression coverage includes full-versus-partial FLOAT cast metadata, SYSDATE scale changes across repeated reuse, and the nested IF(SYSDATE(3) cast as CHAR) consumer case.

Validation completed:

  • focused regression test: pass
  • focused test with count=20: pass
  • focused race test: pass
  • full pkg/sql/colexec test suite: pass
  • go vet pkg/sql/colexec: pass
  • pkg/sql/compile compile-only check: pass

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The runtime-metadata fix in 0e1ab37ff1 closes the previous compact/scatter issue, and the generic function masking, CASE first-match state, reuse cleanup, and bounded buffers now look sound. Two merge blockers remain, both showing that the lazy-evaluation contract is still incomplete rather than needing another function-specific patch.

  1. Skipped leaf executors can still execute and fail. EvalIff, EvalCase, and EvalCoalesce invoke every child executor with a mask (for example lines 570, 610, and 636), but only FunctionExpressionExecutor.Eval interprets an all-false mask. ParamExpressionExecutor.Eval and VarExpressionExecutor.Eval ignore it (lines 294 and 369). A deterministic production-shaped counterexample is IF(FALSE, @missing_user_variable, 'ok'): the binder produces a direct Expr_V, MatrixOne's user-variable resolver can return user variable does not exist, and the unselected branch still fails the expression. I reproduced this on the current head with a resolver that returns an error and asserted that it must not be called; the test fails with that resolver error. The same hole exists for direct leaf children of CASE/COALESCE. Short-circuiting therefore needs to be an ExpressionExecutor-level invariant (or an equivalent common child-evaluation boundary), including leaf executors, with selected-error and reuse controls—not only a FunctionExpressionExecutor fast path.

  2. Flow-control constant folding is disabled wholesale, causing a large deterministic hot-path regression. doFold unconditionally returns for IF/CASE/COALESCE at lines 118-120. On the same Apple M4/CGo test setup, an 8192-row repeated evaluation of constant IF(TRUE, 7, 9) measured:

    • merge base: 9.3–11.2 ns/op, 8 B/op, 1 alloc/op
    • current head: 27.5–28.5 µs/op, 264 B/op, 5 allocs/op

    This is roughly 2,500–3,000x relative regression and changes a once-folded expression into per-batch/per-row work. The absolute per-batch cost is small, but it scales through scan/projection hot paths and also applies to constant CASE and COALESCE. Please preserve lazy correctness with lazy-aware folding: evaluate/fold only the selected constant branch(es), never the skipped expression, then cache the final constant result when the selected dependency graph is foldable. Add an invariant/unit check that valid constant flow-control becomes folded after lazy evaluation, plus a benchmark/allocation guard.

The current focused test passes under -race -count=5; full pkg/sql/colexec and pkg/sql/plan/function tests pass, all CI checks are green, and a merge-tree check against latest origin/main is conflict-free. Those tests do not cover the direct-leaf failure or retain a folding/performance oracle.

@LeftHandCold

Copy link
Copy Markdown
Contributor Author

Addressed both blockers from review 4724034771 in commit 09fc871.

  1. Direct parameter/variable leaves now honor an all-false selection mask before reading prepared parameters or invoking the variable resolver. The skipped typed-NULL cache is deliberately separate from the resolved-value cache, so a skipped generation cannot poison a later selected generation. Coverage includes IF selected-error/reuse transitions, CASE and COALESCE direct variable leaves, missing parameters, and skip -> selected parameter reuse.

  2. IF/CASE/COALESCE constant folding is now lazy-aware instead of disabled. Folding walks conditions left to right and folds only the selected result dependency (or the required COALESCE prefix), supplies temporary typed NULLs only to execute the existing final kernel, and caches the folded result. It covers selected errors, CASE without ELSE, all-NULL COALESCE, nested IFNULL rewrites, and prepared-statement ResetForNextQuery refolding.

I also re-reviewed the complete PR around ownership, partial-row compact/scatter behavior, runtime type metadata, CASE first-match state, shrinking-batch reuse, reset/free paths, buffer growth, and current main compatibility. One additional state bug found during self-review--reusing the actual parameter NULL cache as the skipped placeholder--was fixed before push.

Validation:

  • focused test: count=20
  • focused race test: count=5
  • full colexec, plan/function, and projection tests
  • sql/compile compile-only boundary
  • go vet and go build for the affected packages
  • git diff --check
  • latest main merge-tree: conflict-free

The retained 8192-row constant IF benchmark is 3.91-3.93 ns/op, 0 B/op, 0 allocs/op after warm-up (previous PR head was about 42 us/op and 4 allocs/op locally).

@LeftHandCold
LeftHandCold requested a review from XuPeng-SH July 18, 2026 04:05

@gouhongshen gouhongshen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Codex automated review

No correctness findings in the PR diff. Focused colexec tests were blocked by missing host C headers (xxhash.h, roaring.h, usearch.h).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working size/XL Denotes a PR that changes [1000, 1999] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: flow-control functions evaluate skipped branches instead of short-circuiting like MySQL

5 participants