Skip to content

feat(readers): normalise NUMERIC result values to float64 - #204

Merged
trakhimenok merged 3 commits into
mainfrom
pg-sql-05-value-normalisation
Oct 4, 2026
Merged

trakhimenok merged 3 commits into
mainfrom
pg-sql-05-value-normalisation

Conversation

@trakhimenok

@trakhimenok trakhimenok commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

SQL-05: NUMERIC result values as float64

pgx reports float64 as the scan type of a PostgreSQL NUMERIC column but delivers a string. Both readers now turn it into a float64.

What changes

  • normalizeValueByDatabaseType (reader_records.go) converts a string or []byte in a column whose database type name is NUMERIC. Accepted text: decimal digits with optional sign and fraction, and exactly NaN, Infinity, -Infinity. Anything else (including inf, hex floats, exponents, out-of-range values) is kept as text.
  • The records reader applies it to every column with that type name.
  • The recordset reader applies it only when the recordset column's Go type is float64, so a []byte or string column keeps exactly what it returned before.
  • A string in a float64 recordset column now returns an error naming the column and Go type (it used to panic in dalgo's strict value.(T)). The error never quotes the cell.

What does not change

  • DECIMAL (MySQL) and JSON/JSONB are not normalised: MySQL callers keep "13.80" and the records reader already stores []byte as string.
  • SQLite results are unchanged, including BLOBs in JSON/JSONB/NUMERIC columns (pinned by tests against real SQLite).

Known limits

  • A float64 holds about 15 significant digits; a NUMERIC(20,0) key is rounded, and ids above 2^53 can collide.
  • A NUMERIC NaN or infinity in a plain SELECT becomes a non-finite float64 (encoding/json refuses it); aggregate queries reject it.
  • Residual: in the records reader a SQLite BLOB holding decimal text in a bare NUMERIC column (x'3133') becomes float64; the driver cannot be told apart at this layer.
  • The Get path (getter.go) is not normalised; follow-up task.

@trakhimenok

Copy link
Copy Markdown
Contributor Author

[review r1 #204] independent adversarial review (Opus), head edca11e

Review of edca11e (round 1)

What I verified

  • Read the full diff and the surrounding code: reader_base.go, reader_records.go, reader_recordset.go, getter.go, the CI workflow. The repository has no AGENTS.md or CONTRIBUTING.
  • Read the driver sources the change depends on: pgx v5.10.0 stdlib/sql.go (NUMERIC: scan type float64, text delivered; JSON/JSONB: scan type string, []byte delivered), modernc sqlite v1.60.1 rows.go (scan type follows the first row's storage class; declared type upper-cased), go-sql-driver/mysql v1.10 fields.go, lib/pq rows.go, and dalgo v0.89.1 recordset/column_any.go:45 (value.(T), panics on a type mismatch).
  • Ran the eight new test functions in the author's worktree: pass. Checks at this head: Lint and Build & test (100% coverage floor) are green; both are the required checks. Main is green at the merge base.
  • The acceptance line holds for pgx: NUMERIC "13.86" gives float64 13.86 in both readers, and NUMERIC NaN is still rejected in aggregate queries.
  • No SQL text is built from values in this change. No exported interface changed.
  • Not executed: the blocker below is from code reading; I was not allowed to add a test.

Blocking

  1. reader_recordset.go:149 — normalisation ignores the recordset column's Go type, which is fixed from ScanType at lines 36-110. When the column is typed []byte, the converted value (float64 or string) matches none of the coercion branches (lines 169-194) and SetValueByIndex panics in dalgo's value.(T). Before this PR those rows returned []byte.
    • SQLite: a column declared JSON or JSONB whose first row is a BLOB (what jsonb() stores) now panics. So does any BLOB in a column declared NUMERIC or DECIMAL (parsable bytes become float64; unparsable bytes become string, reader_records.go:112).
    • lib/pq: NUMERIC, JSON and JSONB report scan type interface{}, which line 89-91 maps to a []byte column; every such row panics.
    • Where the column is typed string the value is silently rewritten: MySQL DECIMAL (scan type string, []byte delivered) turns "13.80" into "13.8" and "12345678901234567890.12" into "1.2345678901234567e+19".
    • Fix: in recordsetReader.Next take col and vt first and call the normaliser only when vt.Kind() == reflect.Float64; keep the finite check after it; drop JSON handling from this reader (string columns already convert []byte at lines 185-188). Add tests that fail today: sqlmock NUMERIC with scan type []byte(nil); sqlmock JSONB with scan type []byte(nil); sqlmock DECIMAL with scan type "" and value []byte("13.80") expecting "13.80"; real SQLite CREATE TABLE d (doc JSONB, n NUMERIC); INSERT INTO d VALUES (x'7B7D', x'3133') read through getRecordsetReader.

Serious

  1. reader_records.go:99 — the "DECIMAL" arm never matches PostgreSQL (pgx and lib/pq report NUMERIC). It only changes MySQL, where every decimal column reports DECIMAL, and SQLite columns declared bare DECIMAL. A MySQL DECIMAL in the records reader changes from the exact string "13.80" to float64 13.8. dalgo2mysql creates DECIMAL columns and openvaultdb-go's MySQL mount reads through ExecuteQueryToRecordsReader. This is undeclared and untested, and the doc comment says other drivers are not altered. Fix: remove "DECIMAL" (recommended), or keep it as a stated decision with a MySQL-shaped test in both readers.
  2. reader_recordset.go:173-178 — the new error quotes the raw cell value with %q, unbounded, into text that callers log. No other error in the package carries row data. The branch also fires for any string in any float64 column on any driver (for example a SQLite REAL column with a text row), not only NUMERIC as the comment says. Fix: name the column and %T only; correct the comment; the test at reader_value_normalisation_test.go:164 should assert the cell text is absent.

Minor

  1. reader_records.go:113-116 — the JSON/JSONB arm changes nothing: the records reader already converts every []byte (line 191) and the recordset reader already does so for string columns. TestRecordsReader_JSONBytesBecomeString (:90) and TestRecordsetReader_JSONBytesBecomeString (:183) pass on main without this PR.
  2. reader_records.go:109 — ParseFloat accepts inf, Infinity, NaN in any case and hex floats. A plain SELECT of a PostgreSQL NUMERIC NaN now yields a non-finite float64 that encoding/json refuses; SQLite text such as 'NaN' or '0x1p4' in a NUMERIC column changes type. Fix: accept decimal text plus exactly NaN, Infinity, -Infinity; document the non-aggregate case.
  3. reader_records.go:86-96, 198-203 — exactness loss beyond about 15-17 digits is not documented, and it applies to the identity column: a NUMERIC(20,0) key becomes a float64 record key, so ids above 2^53 can collide. Out-of-range values fail the whole row in the recordset reader.
  4. reader_value_normalisation_test.go:205-255 — the SQLite pin breaks its loops on any error (:226, :244), filters out the text row for the recordset reader (:235), never asserts the third value (:252), and covers only bare NUMERIC: not NUMERIC(10,2), DECIMAL, JSON, JSONB, and no BLOBs.
  5. getter.go:384-388 — the Get path is not normalised, so on PostgreSQL Get returns "13.86" and a query returns 13.86 for the same column. Follow-up task, not this PR.
  6. The PR body is empty; the deviations and the driver effects should be on the PR.

Declared deviations

  • Error instead of panic for a string in a float64 column: right in principle; see 3.
  • NaN/Inf via ParseFloat: acceptable with 5; the claim that SQLite cannot be told apart at this layer is not correct.
  • No nil-ColumnType guard: acceptable; pre-existing (the earlier failure is the index at reader_base.go:124).
  • Helper placed in reader_records.go; end2end is a separate module: fine.
  • Not declared: the MySQL DECIMAL change (2) and the []byte-column panic (1). The hand-off's statement that every other value passes through untouched and SQLite is unchanged does not hold for BLOBs.

VERDICT: blockers=1 majors=2 minors=6 land=no

@trakhimenok trakhimenok changed the title feat(readers): normalise NUMERIC and JSON result values by database type name feat(readers): normalise NUMERIC result values to float64 Oct 4, 2026
@trakhimenok

Copy link
Copy Markdown
Contributor Author

Review by claude-opus-5-5@claude-code@wf_22baf019-c03 (head fc00b1c7acd7)
Reviewed-Head: fc00b1c

[review r2 #204] independent adversarial review (Opus), head fc00b1c

[review r2 #204] independent adversarial review (Opus), head fc00b1c

Review of fc00b1c (round 2)

What I verified

  • Head established from GitHub: fc00b1c, equal to the author's worktree HEAD (clean). Read the delta edca11e..fc00b1c and the whole change against main (3 files: reader_records.go, reader_recordset.go, reader_value_normalisation_test.go). No existing test, workflow or coverage floor was touched.
  • Checks: wb ci wait passed on this head. Build & test (100% coverage floor) and Lint are green; merge state is clean.
  • Ran the 11 normalisation tests in the author's worktree through wb run: pass.
  • Attacked the strict decimal form against pgx, because it is my round 1 recommendation and would break the acceptance line if pgx ever emitted an exponent. pgx v5.10.0 (and v5.0.0): NUMERIC goes through the default *string scan plan; in binary format that is scanPlanBinaryNumericToTextScanner -> encodeNumericText -> numberTextBytes, which prints plain digits with an optional - and fraction, or exactly NaN, Infinity, -Infinity. In text format it is the server's own text. NULL arrives as nil. Every pgx value matches the new rule.
  • The regexp is anchored and Go's $ is end of text, so a trailing newline does not match. EqualFold has no non-ASCII fold onto NUMERIC.
  • MySQL (DECIMAL) and SQL Server (DECIMAL) never report NUMERIC; they are untouched.
  • No SQL is built and no connection string is handled in this change. No exported identifier changed.
  • Not executed: I could not run the tests at edca11e (read-only), so "fails without the fix" is from reading the old code against each new test. The SQLite 'NaN' text case in finding 2 is from code reading.

Earlier findings

  1. Blocker, []byte column panic: CLOSED. Normaliser runs only for a float64 recordset column; sqlmock and real-SQLite BLOB tests prove it.
  2. Major, MySQL DECIMAL: CLOSED. Only NUMERIC matches; tested in both readers.
  3. Major, error quoting the cell: CLOSED. Column and Go type only; two tests assert the cell text is absent.
  4. JSON arm: CLOSED (removed).
  5. ParseFloat leniency: CLOSED (strict decimal text plus three exact spellings).
  6. Precision documentation: CLOSED.
  7. SQLite pin: CLOSED, with a smaller gap left (new finding 1).
  8. Get path: DECLINED-ACCEPTED. Out of scope, stated in the PR body; the follow-up task still needs creating.
  9. PR body and title: CLOSED (two inexact sentences, new finding 2).

New findings (minor)

  1. reader_value_normalisation_test.go:347-410 — the declared residual is not pinned. The 'blob columns' subtest reads n and dec only through the recordset reader, and the records reader only for docb. The rewrite also dropped the round 1 real-SQLite text row ('abc' in a bare NUMERIC column, records reader). Fix: add records-reader assertions SELECT n FROM blobs -> [float64(13), "abc"] and SELECT dec FROM blobs -> ["13", "abc"], and a texts (n NUMERIC) table with 'abc' expecting "abc".
  2. Wording. (a) The PR body says SQLite results are unchanged "including BLOBs in ... NUMERIC columns" and then declares the opposite for the records reader; the texts NaN, Infinity, -Infinity in a bare SQLite NUMERIC column also become float64 there and are not mentioned. (b) reader_records.go:104-106 says a value is never turned into a type the column cannot hold; unparsable text is returned as a string and then refused with an error. (c) With lib/pq the readers differ (records: float64; recordset: []byte); say so. The ecosystem uses pgx, so this is documentation only.

For the lead's decision (residual SQLite change)

Accept it. Before this PR that cell was already returned as the string "13", not as bytes; now it is the number 13. It needs a BLOB of ASCII digits in a column declared bare NUMERIC. The author's stricter option (convert only string) does not remove the 'NaN' text case and costs the lib/pq bytes case. If you want it narrower, the cheaper rule is to normalise in the records reader only when the driver's scan type is float64, which is what the recordset reader now does in effect; that excludes SQLite BLOB-first and text-first columns but also drops lib/pq. Not required for landing.

Not a finding: a []byte in a float64 column whose type is not NUMERIC (a SQLite BLOB row in a REAL column) still panics in dalgo's value.(T). This is the same on main and is not caused by this change.

Lead's disposition of the remaining findings. The two remaining minors (no test pins the declared SQLite change for a BLOB in a bare NUMERIC column in the records reader; three inexact statements in the description and comments) are DEFERRED to follow-up task pg-sql-w1-minors in this repository, dispatched with wave 2 and reviewed independently.

VERDICT: blockers=0 majors=0 minors=2 land=yes

@trakhimenok
trakhimenok enabled auto-merge (squash) October 4, 2026 20:59
@trakhimenok
trakhimenok merged commit 94c5c81 into main Oct 4, 2026
5 checks passed
@trakhimenok
trakhimenok deleted the pg-sql-05-value-normalisation branch October 4, 2026 21:01
trakhimenok added a commit that referenced this pull request Oct 4, 2026
…rendering, docs, SQLite reader tests) (#207)

All five findings are fixed (#205 findings 1-3, #204 findings 1-2) and the pull request is open.

Source commits:

- 985fd2e fix(sql): close deferred minor findings of #204 and #205 (guard JSON rendering, docs, SQLite reader tests)

Pull request: #207
Review: #207 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant