fix(colexec): short-circuit flow-control expressions#25742
fix(colexec): short-circuit flow-control expressions#25742LeftHandCold wants to merge 8 commits into
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
XuPeng-SH
left a comment
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
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:
The fix now:
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:
|
XuPeng-SH
left a comment
There was a problem hiding this comment.
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.
-
Skipped leaf executors can still execute and fail.
EvalIff,EvalCase, andEvalCoalesceinvoke every child executor with a mask (for example lines 570, 610, and 636), but onlyFunctionExpressionExecutor.Evalinterprets an all-false mask.ParamExpressionExecutor.EvalandVarExpressionExecutor.Evalignore it (lines 294 and 369). A deterministic production-shaped counterexample isIF(FALSE, @missing_user_variable, 'ok'): the binder produces a directExpr_V, MatrixOne's user-variable resolver can returnuser 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 anExpressionExecutor-level invariant (or an equivalent common child-evaluation boundary), including leaf executors, with selected-error and reuse controls—not only aFunctionExpressionExecutorfast path. -
Flow-control constant folding is disabled wholesale, causing a large deterministic hot-path regression.
doFoldunconditionally returns for IF/CASE/COALESCE at lines 118-120. On the same Apple M4/CGo test setup, an 8192-row repeated evaluation of constantIF(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.
|
Addressed both blockers from review 4724034771 in commit 09fc871.
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:
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). |
gouhongshen
left a comment
There was a problem hiding this comment.
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).
What type of PR is this?
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)orIF(FALSE, @missing_user_variable, 'ok').This PR:
IF,CASE, andCOALESCEbranch selection lazy for every row;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:
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.