Repository navigation
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.
Deep review of c62d79b729e1c366290c782cf18568b7b4ee4581 against 43c37847b16d37cfa8b9eef4235ba4388b5e25ce: APPROVE — no unresolved material blocker found. Reviewed directly without subagents.
The correction belongs in the existing prepared ROUND/TRUNCATE value binder. An outer result CAST previously supplied a narrow numeric context to the source parameter; digits could be discarded before the precision function ran. Saving, clearing and restoring the three inherited context fields closes that boundary for scalar and projected inputs. The scoped defer restores parent state on success, NULL fallback, binding error and nesting. Explicit input CASTs retain their own semantics. No second binder path, state machine, cache, storage or execution mechanism is introduced.
Traced the consumers through source-parameter binding, scalar/derived projection, integer precision binding, explicit result casting and prepared runtime cache admission. Text-value-dependent overloads still set ValueDependent and stay outside the type-only runtime cache. The changed predicate test is not weakened: it now also proves the executable explicit result CAST and parameter survive safe key lowering, with public +/-9999 versus 54321 result controls.
Reviewed all six changed files and their test costs separately. Production: +12/-0. Tests and BVT goldens: +298/-1, net +297. Documentation: 0. Typed-plan/context-restoration assertions, public SQL/binary results and BVT metadata have distinct purposes; existing fixtures are reused with minimal data and immediate statement/session cleanup. No new cluster, test framework or unnecessary production abstraction is added.
Independent QA used an untracked overlay on the current source and the existing one-CN integration fixture: 24/24 public checks passed (12 expression shapes through SQL PREPARE and binary prepare). Cases included both nesting orders of ROUND/TRUNCATE, scalar result and derived input, dynamic and negative precision, exact text beyond DOUBLE's integer range, a nested explicit input cast, sibling context restoration, and inner/outer ABS plus COALESCE. Expected results were asserted directly; normal run exited 0 (test 8.47s, package 11.994s, Darwin/arm64 Go 1.26.4 with provenance-verified CPU libraries). No tracked source was changed.
Reused current-head green build, Ubuntu UT, SCA, coverage, proxy/pessimistic BVT and CI Required evidence. Author evidence additionally records unfixed-base failures, complete owning planner/integration validation, invalid-text recovery, and normal mo-tester comparison twice. There are no outstanding inline/review comments; the only issue comment is Qodo's paused-review notice.
Runtime impact is confined to statement binding: local context save/restore and a defer. Numeric kernels, row loops, I/O and cache eligibility are unchanged. No equal-throughput benchmark claim is made. No storage, wire, authorization or shared concurrent lifecycle contract changes, so additional migration or concurrency stress would not prove a changed invariant here.
|
This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again. |
Summary
Fixes #29623.
Keep a prepared ROUND/TRUNCATE value in its own input domain. A surrounding output CAST must execute after the precision function, not implicitly round/narrow the parameter before the function sees it.
The latest verified main base is
43c37847b16d37cfa8b9eef4235ba4388b5e25ce. It contains #29508 and still reproduces the outer-DECIMAL wrong result found in nightly run 37215929355.1.461.5 / 1.51.5 / 1.4-1.46-1.5 / -1.5-1.5 / -1.4The defect also affects fresh text-first executions and SQL PREPARE; it is not dependent on a previous integer binding or a binary-only cache issue. #29508 fixed another input-domain variant, but its tests did not cover this outer DECIMAL composition. This PR does not claim #29508 was the original introducing change.
Minimal change and invariants
TRUNCATE(CAST(? AS DECIMAL(20,1)),1)still returns1.5for text1.46.Reuse the existing planner tests, SQL/binary integration fixture and prepared BVT. Coverage includes scalar/derived input, first text execution, typed rebinding, NULL, invalid text followed by recovery, explicit input controls and ROUND double rounding (
1.449, precision 2, output scale 1).The existing
explicit value castpredicate test is updated with stronger semantic checks. With the premature implicit input cast removed, ROUND runs before the user's explicit DECIMAL(4,0) result cast, whose existing saturation semantics yield 9999. The unchanged optimizer can therefore safely lower the complete peer expression. Tests assert that the explicit CAST, executable parameter and ValueDependent flag remain, and both public protocols return only the correct +/-9999 row, never the original 54321 row.Validation on host 50
Linux amd64, repository-supported Go/CGo toolchain, isolated worktree; no existing user MO service was modified.
./pkg/sql/plancomplete package: PASS, 8.067s../pkg/tests/sqlintegrationcomplete package: PASS, 135.576s. Final additional double-rounding/predicate controls were then validated with the complete owningTestPreparedNumericTemporalContracts: PASS, 12.796s.TestPreparedSpecializedDomainsconsumer regression: PASS, 16.295s.issue_29505_round_truncate.sqlwith normal mo-testermethod=runcomparison: 31/31 twice on the same CN, zero failures/ignored/abnormal commands; nogenrsor golden acceptance. Each pass had no remaining test database.go vet: PASS; configured incrementalgolangci-lint: 0 issues; gofmt andgit diff --check: PASS.Tests use the native provenance-checked
.agents/skills/mo-dev/scripts/mo-cgo-testwrapper. Local diagnostic/bridge sources, reports and documents are not part of the commit. Independent self-review found no remaining blocker.Risk is confined to statement binding and its context restoration, not per-row execution. Local validation does not substitute for the PR's CI or prove every unrelated workload unaffected.