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? |
9015951 to
4c4323b
Compare
|
mo-bot review · Commit: Review completed. Read the assessment. |
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-local · review
Commit: 4c4323b55476267ca7a6b2cb256a8d8152759644
Assessment: request_changes
The shared conversion helper correctly centralizes bounds validation without new state. Name resolution has a mode-2 regression: CREATE preserves spelling, but the new helper lowercases it.
Model: gpt-6.1-sol / max
Session: 01a1a004-90c3-73b1-acc9-3d4636452003 · slot 0 · generation 0
- P2
pkg/sql/plan/function/seqfunc.go:341: A session using lower_case_table_names=2 creates MySeq and then calls nextval('MySeq'), currval('MySeq') or setval('MySeq', ...).
Impact: CREATE stores MySeq, but this helper requests myseq, so the functions cannot resolve the created mixed-case sequence. Match CREATE's mode-1-only folding and correct the new mode-2 test expectation.
Evidence: mysql_lexer.go:642 GetDbOrTblName folds only mode 1; the table_name grammar and buildCreateSequence retain its result. Catalog lookup compares names exactly (cache/catalog.go:500). The component probe reproduces MySeq versus myseq in mode 2, while modes 0/1 agree. sequence_relation_name_policy_test.go:52 asserts the incorrect lowercase result. Evidence: mode-two-source-proof.json and case-policy-probe.jsonl.
Validation:
-
Reviewed all 10 changed paths, both discussion entries and linked issues #31908/#31909; traced DDL rollback and relation/session identity.
-
Preflight passed: checks/1793828542927548911-2.json; preflight.log.
-
Range helpers passed 18,720 independent math/big comparisons and baseline controls: checks/1793828623125360417-2.json; sequence-range-probe.jsonl.
-
Policy probe reproduced the mode-2 mismatch: checks/1793829032993920439-2.json; case-policy-probe.jsonl.
-
Source/fixture audit passed, including 38+17 SQL/result statements and the corrected legacy golden: checks/1793828795570654656-2.json; source-audit-v2.log. Reconciliation: validation-evidence.json.
-
Frozen-head CI reports successful build, unit tests, SCA and multi-CN BVT.
Limitations:
-
Local probes use extracted production bodies with documented harness adaptations; full native packages and public SQL suites were not rerun locally.
-
Detailed author logs were not supplied; reported package and 958-statement BVT runs carry older-base labels.
-
INCREMENT semantics remain unchanged; the bounds fix covers START/MINVALUE/MAXVALUE.
4c4323b to
4ecca8e
Compare
|
mo-bot review · Commit: Review completed. Read the assessment. |
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-local · review
Commit: 4ecca8ecdc6c8867d01279c7cef1c9600c10edcc
Assessment: approve
Shared validation correctly rejects out-of-domain START/MIN/MAX values, and sequence lookup plus internal SQL now follow CREATE's identifier policy. The prior mode-2 finding is resolved; no actionable correctness or design findings remain.
Model: gpt-6.1-sol / max
Session: 01a1a004-90c3-73b1-acc9-3d4636452003 · slot 0 · generation 0
Validation:
-
Reviewed all 13 changed files, affected consumers, four discussion entries and both linked issues.
-
Extracted production helpers passed 12,648 independent math/big comparisons using Go 1.26.6; range-probe.log and checks/1793862414202087910-2.json.
-
Source audit passed: clean head, diff/gofmt checks and 55 SQL/result statements; source-audit.log and checks/1793862571019366748-2.json.
-
Reused supplied exact-head UT, static analysis, coverage and both BVT successes; author SQL evidence retains its reported source/binary labels.
Limitations:
- Go 1.27.1 bootstrap timed out. Local execution covered extracted helpers, not native packages or public SQL; those checks rely on supplied CI and attributed author evidence.
🟠 Testing · estimated merge 10:27 UTCWaiting for
Queue rule
So far
2 merge conditions met
Queue rule
|
What type of PR is this?
Which issue(s) this PR fixes:
Fixes #31908
Fixes #31909
What this PR does / why we need it:
Reject explicit sequence START/MINVALUE/MAXVALUE literals outside the declared signed or unsigned integer type before narrowing them. Errors retain the original value, rejected CREATE leaves no sequence, and rejected ALTER preserves the stored row and issued position. The six supported integer domains are covered; INCREMENT retains its existing int64 contract.
Resolve NEXTVAL/CURRVAL/SETVAL string arguments with the same case policy as CREATE SEQUENCE: mode 1 folds names, while modes 0 and 2 preserve their spelling. Sequence-owned internal SQL executes already-resolved physical identifiers without folding them again in mode 2, closing the real allocation UPDATE failure reported in review 5776394217. This is limited to the sequence executor environment; allocation identity, generation fences, permissions, caller/autonomous transaction ownership and session caches retain their existing behavior.
Update the legacy SMALLINT MAXVALUE 9999999 golden error to report the original out-of-domain value instead of a silently wrapped MAXVALUE -27009. The abnormal SQL statement remains in the comparison suite.
Validation:
CI and the requested re-review are pending on the new HEAD. Local validation does not claim predecessor-binary migration or multi-CN coverage.