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 self-authored head 8558baf1fbe5e6612011dd233b8bbacb96e88aff against base/merge-base 19e01d261b078c172585a3ca46da86eb7e446921. COMMENT only because GitHub does not permit author approval/request-changes. The local decision is REQUEST_CHANGES: one independently reproduced wrong-result regression remains, attached inline.
All six changed files and their numeric-proof, filter-normalization, block-filter and prepared-cache consumers were reviewed. This is an ordinary focused fix, exempt from a new feature design document. Reusing ConstantTranspose, canonicalRangeOp and the existing exact-integer/value-dependent binding proof is the right ownership direction. The two historical #29508 optimization comments (unary default precision and scalar-subquery comparisons beyond equality) are addressed. The new AND/OR traversal, however, also enables the old arithmetic equality transposition in places where it previously did not run; this is not safe for floating arithmetic.
Independent QA on the exact head:
- TestReviewNestedFloatTransposePreservesRows FAIL: the unchanged executable expression accepts 1e-17, while its rewritten form rejects it.
- TestReviewNestedFloatSQL FAIL through a real MySQL connection: the native nested predicate returns count 1; the explicit-cast control returns the correct count 2.
- The identical two probes PASS on a separate clean-production checkout of base 19e01d2 (real-client test 9.47s; package terminal status successful). Both worktrees built their own native artifacts with the repository make/wrapper workflow. No production fixes were made.
Implementation (+35 net lines), tests (+113) and documentation (0) were reviewed separately. The tests extend existing fixtures with small deterministic datasets, expected rows and executable domain controls; they do not cover the newly reachable approximate-arithmetic branch under OR. Preserve those tests and add the orthogonal counterexample/control at this normalization boundary. No new retained state, executor, storage or cancellation mechanism is introduced; the extra normalization work is during planning. Eligible exact ROUND/TRUNCATE bindings can retain native pruning, but that benefit cannot justify changing unrelated floating query results.
The author's owning-suite, lint, public-client and block-measurement evidence was read and its scope checked; it does not select this counterexample. The latest CI run 36682947815 is in progress, so this review does not claim current CI success or wait for it. Main-workspace files were unchanged. No subagents were used.
8558baf to
bf23ee0
Compare
XuPeng-SH
left a comment
There was a problem hiding this comment.
Follow-up review of head bf23ee041bc750bf86b2727f197648d12a469b0b against rebased main c628836143ff3f2d2265124ffd84ae5acebf0d6c: PASS, no unresolved material blocker. COMMENT is used for this self-authored PR.
The floating wrong-result finding at discussion_r4141993739 is repaired at the normalization owner: Boolean recursion now uses only direction normalization and cannot reach legacy arithmetic equality transposition. The existing root algebra boundary remains unchanged. Native comparison direction, prepared exact-value proofs and executable operand domains stay with their existing owners; redundant checks were removed. All six changed files, surrounding consumers and implementation/test increments were reviewed.
Actual requested model/effort: GPT-6.1 Sol xhigh for the design and final overall review, with a separate GPT-6.1 Sol xhigh independent QA agent. Design and overall review artifacts record reviewed fingerprint 8111d26a843a51f83e790cb147b3fa5b03cea27bcaa7b1dbc63775c108da7b9c; committing the reviewed content did not alter it.
Final local plan/filter/embed complete packages passed with matrixone_test, -short, -vet=all, -p 1, -count=1 (5.594s / 0.782s / 54.236s, terminal exit 0). Independent executor and real MySQL challenges passed for floating subtraction/cancellation, nested AND/OR, NULL, signed-overflow error codes, input ownership/idempotence, native PK peers, durable UPDATE/DELETE effects, and one binary prepared statement through type changes, invalid input and recovery. The permanent regression extends existing fixtures with four executor rows and two SQL rows; wider diagnostic probes remain outside the submitted suite.
No new persisted-block performance measurement or CI completion is claimed. The prior root-level algebra limit is outside this Boolean-traversal repair.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review of bf23ee041bc750bf86b2727f197648d12a469b0b against fresh base/merge-base c628836143ff3f2d2265124ffd84ae5acebf0d6c: PASS / no new material blocker found. This review used no subagents. COMMENT is necessary because the authenticated account authored this PR.
All six changed files, original comments, comparison overloads, scan normalization/native PK consumers, exact numeric proofs and prepared-cache admission were reviewed. The design keeps existing responsibility boundaries: unary default-zero recognition uses the existing precision proof; late scalar comparison rewriting uses existing exact-integer and direction owners; value-dependent plans remain excluded from the type-only cache. No new state machine, storage or execution path was introduced. The direction-only Boolean helper cannot enter legacy arithmetic equality transposition and preserves input ownership and idempotence.
Comment closure: both #29508 optimization comments (unary ROUND/TRUNCATE and scalar comparisons beyond equality) are implemented and covered. The #29525 floating wrong-result finding at discussion_r4141993739 is repaired and its thread is resolved. No additional unresolved technical comment was found.
New public QA on this exact head: 23 prepared rebind cells PASS, using 12 rows and the existing 1CN fixture owner through real binary MySQL protocol. Challenges cover TINYINT PK out-of-range/fractional peers; scientific, malformed, huge-exponent and suffixed text; NULL and byte-text recovery; negative BIGINT neighbors around 2^53; scalar ROUND precision changes through zero/nonzero/negative/NULL/invalid/textual-zero and recovery. A separately executable DECIMAL column control establishes results/error equivalence, with independent expected IDs on ordinary/boundary/recovery cells. Terminal native-wrapper run passed with matrixone_test, -short -vet=all -p 1 -count=1: package 11.373s, fixture-inclusive test 9.43s; case bodies <=0.01s. Temporary QA source was preserved outside the PR and removed after byte verification.
Qualifying exact-head owning-suite and previous executor/public/DML evidence were verified and reused rather than duplicated: complete plan/filter/embed PASS (5.594s/0.782s/54.236s), including floating +/-/deep cancellation, NULL, signed-overflow error preservation, immutable/idempotent normalization, actual UPDATE/DELETE and prepared invalid-input recovery. Relevant tracked fingerprint remains 8111d26a843a51f83e790cb147b3fa5b03cea27bcaa7b1dbc63775c108da7b9c.
Implementation (+46 net lines), tests (+151) and docs (0) were assessed separately. Small complementary typed/executor/public oracles extend existing fixtures; no new submitted topology, retained state or heavy test fixture was added. Extra work stays in planning and only changed Boolean ancestors are rebuilt. No new physical block-count or measured speedup claim is made. Pre-existing root arithmetic transposition is outside this focused increment; this review does not certify its general soundness.
Fresh CI run 36687503001, attempt 2 completed successfully at this head, including required gate, Linux UT, coverage UT, SCA, shared build, proxy and pessimistic BVT and coverage merge. CI was inspected, not awaited. Production source and the user's unrelated workspace edits were unchanged.
What type of PR is this?
Which issue(s) this PR fixes:
issue #29512
What this PR does / why we need it:
Prepared ROUND/TRUNCATE comparisons can miss native block filtering, and reversed native ranges can return wrong rows when scan consumers interpret the operator as column-first. For BIGINT keys 54320/54321/54322,
ROUND(?) <= idwith integer 54321 could return only 54321 instead of 54321 and 54322.ConstantTranspose,canonicalRangeOp, and the existing scan-invariant guard. Preserve executable peers and their domains; copy Boolean arguments only when a child changes.v=1e-17,v+1e0=1e0is true under IEEE rounding, whilev=1e0-1e0is false. The former traversal incorrectly lost this row under OR. The repair also preserves arithmetic overflow errors.This is a follow-up to PR #29508 and its review findings, and addresses the floating-point wrong-result finding in this PR.
The existing owners continue to handle explicit casts, precision, exact text, target width, NULL, large-integer fallback, and value-dependent cache protection. No retained state, storage, execution path, or lifecycle mechanism is added. Work is confined to planning.
Tests extend the existing transpose suite and one-CN prepared fixture. The new permanent regression uses four executor rows including NULL and two SQL rows, with exact results and an executable cast control. Full PR versus rebased main: production +46 net lines, tests +151, documentation 0; this repair contributes production +11, tests +38. Independent QA probes are preserved as external evidence rather than added to the permanent suite.
Validation at
bf23ee041bc750bf86b2727f197648d12a469b0b, rebased onto mainc628836143ff3f2d2265124ffd84ae5acebf0d6c:matrixone_test,-short,-vet=all,-p 1,-count=1(5.594s / 0.782s / 54.236s).Native-filter eligibility remains covered by the existing planner assertions. The prior persisted-block measurements were made at the earlier head; no new block-read performance run is claimed for this repair.
<=>and!=/<>retain semantic coverage; native pruning claims apply to registered equality/range predicates. Volatile peers are excluded from native direction normalization. CI has not been awaited.