fix: fold variable expressions before remote run - #24800
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? |
There was a problem hiding this comment.
Pull request overview
This PR addresses a remote-run failure mode where variable expressions (plan.Expr_V) can panic (or now error) when executed on a reconstructed remote process.Process that does not carry a ResolveVariableFunc. It adds scope-level detection to force local execution for pipelines containing variable expressions, and makes variable-expression evaluation defensive by returning an error instead of panicking.
Changes:
- Convert remote-run scopes to local run when the scope contains variable expressions.
- Make
VarExpressionExecutor.Evalreturn a normal internal error whenResolveVariableFuncis not set. - Add unit tests for scope variable-expression detection and nil resolver behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/sql/compile/scope.go | Falls back to local MergeRun when a remote-run scope is detected to contain variable expressions. |
| pkg/sql/compile/remote_expr.go | Adds reflection-based detection for plan.Expr_V within Scope/Source/operator trees. |
| pkg/sql/compile/remote_expr_test.go | Adds unit tests for scope variable-expression detection. |
| pkg/sql/colexec/evalExpression.go | Adds nil check around proc.GetResolveVariableFunc() to avoid panic. |
| pkg/sql/colexec/evalExpression_test.go | Adds coverage for nil ResolveVariableFunc behavior. |
2738588 to
3f20b3f
Compare
3f20b3f to
75c995b
Compare
978f06a to
d0118b1
Compare
d0118b1 to
374c84d
Compare
|
已按后续 review 调整:RemoteRun 前不再切 MergeRun,而是在 source CN 对 |
aunjgr
left a comment
There was a problem hiding this comment.
Practical fix for remote CN variable resolution. Same change across three branches (#24800 main, #24801 4.0-dev, #24802 3.0-dev).
Approach: Before sending a scope to a remote CN, walk the operator tree via reflection to find VarRef expressions, evaluate them locally (where the session variable context exists), and replace with constants. Uses scopeContainsVarExpr as a fast-path guard so the reflection-based folding only runs when needed.
remote_expr.go (490 new lines): reflection-based traversal of operator structs looking for *plan.Expr fields ([]*plan.Expr, []plan.Expr, and operator-base fields are handled). Two custom interfaces for operators that need special treatment:
argExpressionsGetter/RewriterforAggFuncExecExpression(aggregate arg expressions)lockRowsExpressionsGetter/RewriterforLockOp(lock row filter expressions)
Other changes:
evalExpression.go: nil guard onresolveVariableFunc— returns a clear error instead of nil-deref on remote CNaggexec/types.go: implementsRewriteArgExpressionsforAggFuncExecExpressionlockop/lock_op.go: implementsGetLockRowsExpressions/RewriteLockRowsExpressionsforLockOpscope.go: removes deadVarExprfield fromScope(unused, confirmed by grep ofs.VarExpr)
Reflection safety: only plan.Expr types are matched (via planExprPtrType); the walk handles pointers, slices, and operator-base embedding. replaceVarRefExpr only fires on actual VarRef nodes, so nothing gets accidentally constant-folded.
LGTM. Please also merge #24801 and #24802 (same fix, different target branches).
Fixes #24799.
Summary
@@variable expressions on the source CN before a scope is shipped to a remote CN, preserving the remote execution pathTesting
GOCACHE=/private/tmp/mo-gocache-var-expr-main-copy GOFLAGS=-mod=mod go test ./pkg/sql/colexec ./pkg/sql/compile -run 'TestVarExpressionExecutorWithoutResolveVariableFunc|TestScopeContainsVarExpr|TestFoldVarExprs' -count=1is blocked in this local main checkout by existing usearch cgo build errors:could not determine what C.usearch_* refers to.