Repository navigation
fix: preserve external CSV integer values and whitespace records - #30262
iamlinjunhong wants to merge 8 commits into
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? |
|
mo-bot review · Commit: Review completed. Read the assessment. |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-local · review
Commit: e3bbdab3033396408c91da8cb432d2647f25f927
Assessment: request_changes
Reviewed the complete 12-file delta. Reusing the existing decimal parser and CSV record owner is sound, but the promised range-check-after-rounding contract remains incomplete.
Model: gpt-6-astra / xhigh
Session: 01a13dcf-1517-7f90-b3f9-c5f9f9e7031b · slot 0 · generation 1
- P2
pkg/sql/colexec/external/external.go:1624: Read unquoted 1000e-3 into TINYINT, or 1000000000000000000000e-21 into BIGINT.
Impact: The initial strconv parser reports overflow before reaching the shrinking exponent. Both helpers return immediately, rejecting valid values that should become 1. Queries fail or produce NULL error rows. Allow failed unquoted external-CSV integer parses through exact decimal validation, then check destination bounds.
Evidence: Six materialization counterexamples failed although ParseDecimalInteger returned the expected values. A CSV reader probe also rejected 42949672960e-10 instead of producing 4. The frozen base reproduces this rejection: it is an incomplete fix, not a new regression. A diagnostic overlay removing only the two early ErrRange guards passed these cases and existing conversion/rollback controls.
Validation:
-
Frozen matching-head CI reports successful build, UT, SCA, coverage and multi-CN BVT checks.
-
Local preflight and native compatibility verification passed. Selected existing whitespace, rounding, quoting, LOAD/JSONLINE/SET and rollback regressions passed.
-
Candidate and frozen-base boundary probes failed as described; the diagnostic counterfactual passed. Its success is not candidate validation.
-
Seven command/source/exit receipts were reconciled with logs and overlay hashes in /home/xupeng/.local/state/mo-bot/jobs/e48059dacb29eaaf2f592c6d/attempt-1/review-evidence-audit.json. Source remained unchanged.
Limitations:
-
Standalone issue text and raw author artifacts were unavailable. Public BVT evidence was reused from the frozen snapshot; no fresh frontend run was performed.
-
No throughput benchmark was run. Integer fast paths remain; decimal fallback adds bounded work for exactness, while whitespace parsing removes a TrimSpace scan.
aptend
left a comment
aptend
left a comment
There was a problem hiding this comment.
Deep review of exact head e3bbdab3033396408c91da8cb432d2647f25f927, base/merge-base 501c85ecdd6eee0195172743c6d8aaf125cb252d: REQUEST_CHANGES for one verified P2 incomplete-fix case (inline). No other blocker found.
Independently read all 12 changed files, both linked issues30250/30252, all current review/inline/discussion history and thread state; another reviewer's matching report did not replace verification. This is an ordinary existing-contract repair, so no feature-design approval requirement applies.
Change map: exact external-CSV integer fallback and existing assignment-parser consumer (R2); shared record/CRLF/EOF/line-number handling and LOAD consumers (R2); partial-row tolerance rollback, UT/BVT fixtures (R2). Checked every integer width/MEDIUMINT, quoted/NULL/SET/non-CSV/LOAD controls, rounding/overflow/malformed/nonfinite values, blank-vs-whitespace records, physical line metadata and teardown. The shared formatter/parser reuse is sound; the remaining early integer ErrRange gate is the specific incomplete closure. Work/temporary text is bounded by input length and destination digits, no new shared state, background resource, I/O or synchronization owner. No additional lifecycle/race/restart mechanism is introduced.
Independent diagnostic at reviewed SHA, Go1.27.1/Linux/amd64, native artifacts rebuilt through make cgo and repository CGO wrapper:
- Four materialization counterexamples FAIL for the intended conversion reason; each exact-decimal helper and
1e0control PASS first. - Actual CSV reader/tolerant-row counterexample FAILS: expected non-NULL4, actual NULL/error row. Package terminal0.032s; no frontend SQL reproduction claimed.
- Existing rounding/scope/tolerance/whitespace/RecordLine/exact-decimal tests PASS in external/csvparser/function packages (0.069/0.035/0.031s).
- Probe SHA256
3dda7b954822fcf5a0414818a5c9ba730f0f40a01f4dc57afc5ef0a68b87dbd8. The base's identical pre-fallback ErrRange condition was source-compared; no independent baseline runtime run or production counterfactual patch was made.
Exact-head CI38693888707 is terminal SUCCESS (UT, SCA, coverage, shared build, both BVT jobs). Downloaded execution receipts confirm all three owning packages selected in successful CGO race groups and new external_csv_values.sql 19/19, 0 failures/ignored cases. Those valid passes do not exercise the inline counterexample. Source/result diff checks and gofmt pass; full whitespace check correctly flags spaces/tabs intentionally used as CSV fixture records, not an actionable formatting issue. No production edit, push, issue/label change or CI rerun; own probe/worktree removed after confirmed submission.
|
mo-bot review · Commit: Review completed. Read the assessment. |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-xupeng · review
Commit: 3e43b131ccd34153e9d9ecfabec797bce0f8a585
Assessment: approve
Reviewed the complete 18-file delta, affected callers, test quality and supplied comments. The existing decimal and record-framing owners resolve the prior rounding and JSONLINE concerns without duplicate execution paths. No actionable findings remain.
Model: gpt-6-astra / xhigh
Session: 01a14d34-4e51-79a3-a62d-6c9c16279dc8 · slot 0 · generation 0
Validation:
-
24 selected tests passed across five packages, covering numeric boundaries, conversion scope, whitespace framing, cancellation, rollback and prescan cleanup.
-
Authenticated SQL tests passed serial/parallel JSONLINE IGNORE cases with actual two-reader assertions. The committed SQL fixture passed two replays, each verifying 11 value/error oracles and database teardown.
-
Isolation preflight passed. Native recovery verified 190 matching build inputs, 46 artifact hashes and the original make cgo receipt. Source and formatting checks passed; source remained unchanged.
-
Command/source/exit receipts and retained logs were reconciled in /home/xupeng/.local/state/mo-bot/jobs/7f361ed372e6f3332a489312/attempt-1/review-evidence-audit.json.
-
Frozen exact-head CI metadata reports successful build, UT, SCA, coverage and multi-CN BVT checks.
Limitations:
-
Standalone linked-issue bodies and raw author/CI logs were not supplied; local_validation was empty.
-
Local validation used focused tests and authenticated native SQL. No fresh full-suite, race, throughput or complete mo-tester run was performed.
Merge Queue Status
This pull request spent 3 hours 49 minutes 25 seconds in the queue, with no time running CI. ReasonThe merge conditions cannot be satisfied due to failing checks
HintYou may have to fix your CI before adding the pull request to the queue again. Waiting for any of
All conditions
Tick the box to put this pull request back in the merge queue (same as
|
This branch is being deployed
5 participants
What type of PR is this?
Which issue(s) this PR fixes:
Please leave both issues open for reporter verification after merge.
What this PR does / why we need it:
CSV external-table integer assignment now preserves exact decimal values and checks the destination range after scaling and rounding. Oversized mantissas with shrinking exponents remain valid when their final assigned value fits. Ordinary integer inputs keep the existing strconv fast path; quoted-field, SET, LOAD and non-CSV conversion contracts remain separate.
Whitespace-only CSV records remain field data, including EOF records. JSONLINE retains its separate blank-record policy through one existing parser owner: an opt-in
CSVConfig.SkipWhitespaceRecordsapplies during record framing, shared byReadand allocation-freeScanRecord. Both JSONLINE constructors enable it; the execution reader's duplicate post-materialization filter is removed. Planner IGNORE pre-scan usesScanRecord, so serial and parallel import skip the same visible records and publish the same starting offset.Quoted whitespace and tab-separated malformed JSON records still reach normal validation. Physical line numbers, byte offsets, CRLF, EOF, headers, error rows and batch accounting retain their existing owners. The parser borrows the query context for cancellation checks while skipping/refilling; it adds no reader, cache, goroutine or cancellation owner. Existing pre-scan close handling is retained.
Rebased on official main
35253f2a95e88ec6537696015b6189442aff23d1. Conflict resolution retains both sides' tests, main's sealed-field CR protection, ScanRecord/lookahead handling, exact decimal warnings/select-list semantics and the complete LOAD CHECK IGNORE policy from #30355.Validation
Go 1.27.1, darwin/arm64, verified CPU native prerequisites:
csvparser,externalandfunctionnormal suites pass: 2,774 test roots plus fuzz seeds; one existing environment-dependent skip is explicit.-race -count=1run exercise every selected root across all five affected packages, including main CHECK policy/barrier controls.gpt-6.1-sol / xhighsolution/design and final overall review are retained with source fingerprints and test receipts.CSV keeps whitespace as data. JSONLINE whitespace scanning moves before field materialization; pre-scan no longer decodes fields. Work remains linear in consumed input, without a new shared execution path or state. No measured throughput improvement or general no-regression claim is made.
The distributed SQL fixtures remain in the PR; this update's public execution evidence comes from the authenticated native fixture, not a fresh full mo-tester run. New-head CI remains pending. No repository-wide stress, remote build or heavy benchmark was run.