Skip to content

fix: accept fractional prepared numeric overloads - #27556

Open
daviszhen wants to merge 9 commits into
matrixorigin:mainfrom
daviszhen:fix-issue-27294-prepared-overloads
Open

fix: accept fractional prepared numeric overloads#27556
daviszhen wants to merge 9 commits into
matrixorigin:mainfrom
daviszhen:fix-issue-27294-prepared-overloads

Conversation

@daviszhen

Copy link
Copy Markdown
Contributor

Fixes #27294

Server-side prepared ABS(?) and SLEEP(?) were bound to the first integer overload while the parameter marker was still represented as text. Fractional and decimal-looking values then failed during the cached implicit integer cast.

This change gives only parameter-bearing ABS/SLEEP expressions a stable DOUBLE binding domain during PREPARE. It leaves ordinary column calls and other prepared plans on their existing paths, so execution can reuse the cached compile without the broad per-execution plan specialization that caused the TPCC regression.

Regression coverage:

  • planner tests for direct and nested parameter expressions, plus an ordinary integer-column control;
  • real COM_STMT_PREPARE/COM_STMT_EXECUTE coverage for integer, float, textual fractional values and statement reuse.

Validation:

  • go test ./pkg/sql/plan -run 'TestPreparedScalarNumericOverloadsUseDoubleDomain|TestPreparedNumericContextUses(InsertValuesTarget|InsertSelectTarget|UpdateTarget)$' -count=1
  • go test ./pkg/tests/issues -run '^TestIssue27294PreparedNumericOverloads$' -count=1

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

@aunjgr aunjgr 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.

Forcing prepared ABS to FLOAT64 is not semantically safe. A binary-protocol INT64 value of -9007199254740993 must produce the exact integer 9007199254740993; the new cast rounds it to 9007199254740992. It also changes the integer-domain behavior at MinInt64.

The recursive containsPreparedParamExpr makes the issue broader: abs(case when ? then bigint_col else bigint_col end) converts ordinary BIGINT column values to FLOAT64 even though the parameter only controls the branch.

SLEEP can reasonably have a stable DOUBLE input, but polymorphic ABS must preserve integer, decimal, and floating domains, likely through runtime type-category specialization rather than one approximate universal type. Please add binary-protocol regressions beyond 2^53 and for a parameter nested in a CASE around BIGINT data.

@aptend aptend 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.

Deep-reviewed exact head d34f36df564fd80d380d27575b48ded623a6f640, including the complete diff, linked issue, commit history, all existing reviews/comments, and thread state. The focused integration test, the complete pkg/sql/plan tests, repeated focused planner tests, race tests, and vet pass, but two protocol-level counterexamples expose blocking correctness gaps. Temporary counterexample tests were removed and the exact-head worktree is clean.

Comment thread pkg/sql/plan/base_binder.go Outdated
// prepared parameter has TEXT transport type at PREPARE time, so letting
// the generic overload resolver choose an integer cast makes valid binary
// executions such as ABS(-1.5) and SLEEP(0.01) fail before the function can
// see the value. Use DOUBLE as the stable deferred domain; integer values

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.

[P1] Preserve exact integer semantics instead of forcing every prepared ABS argument through DOUBLE. The next sentence is false for integers wider than 53 bits. On this exact head, a real COM_STMT prepared SELECT ABS(?) executed with int64(-9007199254740993) returns 9.007199254740992e+15, losing one, rather than the exact 9007199254740993. Because containsPreparedParamExpr is recursive, the cast also covers non-parameter data: ABS(CASE WHEN ? THEN bigint_col ELSE bigint_col END) converts the BIGINT column to DOUBLE merely because the condition is parameterized. This silently corrupts valid wide BIGINT results and can also change the function result type. Please retain the bound numeric category/exactness (or safely specialize the overload) without converting the whole parameter-bearing expression to FLOAT64.

Comment thread pkg/sql/plan/base_binder.go Outdated
return e.Else != nil && containsPreparedParamExpr(e.Else)
case *tree.Tuple:
return containsPreparedParamExprs(e.Exprs)
default:

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.

[P1] Do not let an AST allowlist leave valid parameter-bearing forms on the broken integer-overload path. tree.Subquery reaches this default, so a real COM_STMT prepared SELECT ABS((SELECT ?)) executed with float64(-1.5) still fails with invalid argument cast to int, bad value -1.5 instead of returning 1.5. Thus the bug fixed by this PR remains reproducible whenever the marker is behind a scalar subquery (and other omitted wrappers have the same structural risk). Please detect parameters through valid scalar-expression forms, or remove the syntax-shape dependency from overload resolution.

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

Requesting changes on exact head d34f36df564fd80d380d27575b48ded623a6f640.

The direct fractional cases in #27294 are fixed and the cached-plan approach avoids per-execution recompilation, but two blocking correctness/closure problems remain.

  1. [P1 correctness] DOUBLE is not a lossless stable domain for polymorphic ABS.

    ABS has distinct int64, uint64, float64, decimal64, decimal128, and decimal256 overloads. The new preparedNumericFunctionTarget forces every parameter-derived ABS argument to float64, so exact integer/decimal executions silently change both value and result type. The source comment that integer values are losslessly accepted is false outside the 53-bit IEEE-754 integer range.

    I reproduced this through the real COM_STMT_PREPARE/COM_STMT_EXECUTE path on this head:

    select abs(?)       with INT64(-9007199254740993)
    expected: 9007199254740993
    actual:   9.007199254740992e+15
    

    select abs(? + 0) produces the same wrong result. This also changes the native overflow/error contract at MinInt64 and necessarily loses precision for DECIMAL/NEWDECIMAL values that the issue explicitly requires supporting. SLEEP can reasonably use a stable DOUBLE input because its output is not the numeric input; ABS cannot discard its integer/unsigned/decimal domain just to accept fractions.

    Please preserve or dispatch on the execute-time numeric category/precision without recompiling the whole statement. Add binary-protocol regressions around 2^53, MinInt64, uint64 boundaries, and high-precision decimals, checking exact value and result metadata as well as statement reuse.

  2. [P1 functional closure] Parameter detection misses scalar subqueries, so the original bug still reproduces.

    containsPreparedParamExpr is a second, hand-maintained AST walker and has no *tree.Subquery case. Consequently abs((select ?)) and the equivalent SLEEP shape never enter the new path, even though the existing numeric AST scanner explicitly supports scalar subqueries.

    A real protocol regression on this head:

    select abs((select ?))

    executing with DOUBLE(-1.5) still fails with the original error:

    invalid argument cast to int, bad value -1.5
    

    Please derive parameter presence from the canonical numeric scan/visitor rather than maintaining a partial expression-form list, and cover scalar-subquery operands for both ABS and SLEEP. Merely extending this walker while retaining the universal DOUBLE ABS cast would fix finding 2 but not finding 1.

Validation:

  • full pkg/sql/plan and pkg/tests/issues: PASS
  • focused prepared-numeric planner -race -count=10: PASS
  • three temporary COM_STMT counterexamples above: fail exactly as described; removed after verification

No runtime hot-path or resource-lifecycle regression was found in the changed code; the blockers are semantic precision and incomplete functional closure.

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

Deep-reviewed exact head 6d1104f6fccb904561c80aa15d405d6290913895. The previous direct BIGINT and scalar-subquery counterexamples are partially addressed, but blocking correctness and performance gaps remain.

  1. [P1 correctness] DECIMAL protocol values still silently go through DOUBLE. binaryProtocolPrepareParamKind correctly labels MYSQL_TYPE_DECIMAL/NEWDECIMAL as PrepareParamDecimal, but allIntegerParamRefs / typedIntegerParamExpr specialize only PrepareParamInteger. Therefore ABS(?) with a high-precision setBigDecimal value keeps the prepare-time FLOAT64 cast, loses precision/scale, and returns DOUBLE metadata. This is explicitly in #27294 scope. The protocol test added here does not bind an actual decimal value or assert exact decimal value/type. Please rebind and preserve decimal64/128/256 semantics, and add values beyond 53-bit precision plus metadata checks.

  2. [P1 correctness] Exact integer rebinding is only effective for the direct marker shape. For ABS(? + 0) (already identified as an affected form), the marked source is the arithmetic expression. rebindPreparedIntegerExpr recursively rebuilds its existing implicit cast(? AS DOUBLE) via BindFuncExprImplByPlanExpr("cast", ...), so a wide integer such as -9007199254740993 is still rounded before ABS. The new wide-integer regression covers only ABS(?); add real COM_STMT regressions for nested arithmetic/IF/CASE and ensure prepare-time fallback casts are removed/retyped throughout the numeric value path. Scalar-subquery integer exactness is also not covered: the outer ABS argument contains a subquery column rather than a ParamRef, so PreparedPlanHasDeferredNumericFunction may not trigger specialization.

  3. [P1 performance/design] This restores per-EXECUTE full-plan specialization and compilation. Every deferred ABS execution calls PreparedPlanHasDeferredNumericFunction, FillValuesOfParamsInPlan (full DeepCopyPlan + visit), sets retComp = nil, and recompiles instead of resetting the cached prepared topology. That contradicts the PR description and recreates the class of prepared-plan performance regression this approach was intended to avoid. Please keep runtime numeric category dispatch local/cached (or cache bounded typed variants), and provide compile-count plus steady-state latency/allocation evidence for repeated execution. Do not add another unconditional full-plan scan to all prepared executions without evidence.

The negative AuxId marker also reuses the executor memo-identity namespace; negative AuxIds are executed as memoized expressions, not merely planner-local annotations. Use explicit plan metadata or prove uniqueness/lifecycle instead of changing runtime expression semantics as a side effect.

Finally, this head is currently CONFLICTING with latest main and must be rebased/resolved before another exact-head review.

…pared-overloads

# Conflicts:
#	pkg/frontend/computation_wrapper.go
#	pkg/sql/plan/utils.go
#	pkg/sql/plan/visit_plan_rule.go
@daviszhen

Copy link
Copy Markdown
Contributor Author

Implemented the follow-up on the latest head d8bf4cc0216bc91ab80666214ffed46d696de5f9 (including the current main merge).

Addressed the requested blockers:

  • Replaced the negative AuxId marker with explicit Expr protobuf metadata (PreparedNumericFallback, parameter position, and scalar-subquery source node/column identity). The metadata is copied, flattened, serialized, and restored explicitly; a protobuf round-trip regression verifies it and confirms AuxId remains untouched.
  • Added execute-time numeric-category rebinding for signed/unsigned integer and DECIMAL/NEWDECIMAL values. Integer literals retain exact int64/uint64 semantics (including values beyond 2^53 and the native MinInt64 overflow contract); DECIMAL64/128/256 paths retain exact value, scale, and result metadata.
  • Rebound nested numeric value expressions (? + 0, IF/CASE result branches, and scalar-subquery projections) so provisional prepare-time DOUBLE casts are removed/retyped without changing scalar-subquery filtering/LIMIT/empty-result semantics. Condition-only CASE/IF parameters do not widen ordinary BIGINT data.
  • Moved deferred-overload detection to one-time prepared-plan metadata and added a bounded one-entry runtime compile cache keyed by semantic parameter category. Repeated executions in the same category reuse the plan/compile; category replacement is committed only after successful compilation, with process parameter ownership preserved during eviction.

Validation completed:

  • go test ./pkg/sql/plan ./pkg/frontend ./pkg/tests/issues -count=1
  • focused -race planner/frontend tests
  • go vet ./pkg/sql/plan ./pkg/frontend ./pkg/tests/issues
  • git diff --check

make static-check was intentionally not run. Please re-review the updated head.

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

Labels

size/L Denotes a PR that changes [500,999] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Server-side prepared overloaded functions reject valid non-integer bound values

5 participants