fix(base-driver): Handle NULL values in readOnly type detection (#11094)

[ovr]

refs #11094

[claude]

Claude finished @ovr's task in 2m 5s —— View job

---

PR Review

Verdict: 1 medium, 2 low. Overall the refactor is a clean extraction and the NULL-handling fix is correct. One behavior change relative to master worth flagging.

Review details

• Read the diff and new type-detection.ts
• Compare behavior against removed inline implementation in BaseDriver.ts
• Read the new tests

Medium — Reduced sampling changes type detection for large result sets

packages/cubejs-base-driver/src/type-detection.ts:76-92

The old inline code iterated every row for each column when finding a matching type (rows.filter(row => field in row).find(row => !matcher(row[field]))). The new code stops as soon as every column has at least one non-null value (if (unresolvedFields.size === 0) break;) and additionally caps at DB_TYPE_DETECTION_MAX_ROWS = 100.

Consequences:

1. When the first row has non-null values for all columns (the common case), only row 0 is inspected. If a later row widens the type — e.g. row 0 has int = 1, row 500 has int = 2147483648 — the new code returns int, the old code returned bigint. Same problem for int → decimal, date → timestamp, string → text (length ≥256), etc.
2. Even in NULL-heavy cases where multiple rows must be scanned, capping at 100 rows silently drops information the old implementation used.

If the intent is really "sample a few rows for speed," that's a reasonable tradeoff, but it should be a deliberate, documented change (probably called out in the commit / CHANGELOG) since it can change pre-aggregation column types in readOnly mode. A safer minimal fix would be to keep the "scan all rows, ignore NULLs" semantics, or at least keep collecting non-null values on already-resolved columns while scanning to satisfy other columns (right now once unresolvedFields.size === 0 we break, so a column that got its first sample in row 0 never gets to see row 1's larger value).

Low — v instanceof Number is effectively unreachable

packages/cubejs-base-driver/src/type-detection.ts:37

decimal: (v) => v instanceof Number || DECIMAL_REGEX.test(v.toString()),

instanceof Number matches only boxed new Number(...) objects; primitive numbers (which is what drivers return) don't match. Primitive numbers reach the decimal matcher via v.toString() matching DECIMAL_REGEX, so the instanceof check is dead. Not new to this PR (carried over verbatim from the old code) but worth cleaning up while the code is being moved. Same shape as the v instanceof Date short-circuits in timestamp/date, which are reachable.

Low — Test can't distinguish "detected from later row" from "fell back to text"

packages/cubejs-base-driver/test/unit/BaseDriver.test.ts:101-112

The test named "infers type from a later row when the first is NULL" is good, but consider also asserting the all-null behavior more directly (already covered on line 114-124 — good) and adding a case where a later row has a value that would widen the type (e.g., row 0 int = 1, row 1 int = 2147483648) to lock in whichever behavior is intended re: the medium finding above. Right now no test would catch a change in the sampling-vs-full-scan tradeoff.

Nits

• packages/cubejs-base-driver/src/BaseDriver.ts:1-6 — the file's @fileoverview / license header was removed alongside the sortByKeys cleanup. If the project convention is to keep those headers, restore it; if not, ignore.

• Branch: `fix/read-only-db-type-matcher`

[codecov]

Codecov Report

❌ Patch coverage is 96.29630% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.44%. Comparing base (cc1f078) to head (1d02097).
⚠️ Report is 7 commits behind head on master.

Files with missing lines	Patch %	Lines
packages/cubejs-base-driver/src/type-detection.ts	96.07%	1 Missing and 1 partial ⚠️

❗ There is a different number of reports uploaded between BASE (cc1f078) and HEAD (1d02097). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (cc1f078)	HEAD (1d02097)
cubesql	1	0

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #11245       +/-   ##
===========================================
- Coverage   83.77%   59.44%   -24.34%     
===========================================
  Files         257      216       -41     
  Lines       79559    17474    -62085     
  Branches        0     3539     +3539     
===========================================
- Hits        66650    10387    -56263     
+ Misses      12909     6581     -6328     
- Partials        0      506      +506     

Flag	Coverage Δ
cube-backend	59.44% <96.29%> (?)
cubesql	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[claude]

Claude finished @ovr's task in 1m 18s —— View job

---

PR Review (re-check after sync)

Verdict: Prior review still applies — no changes to the base-driver files since the last review. 1 medium, 2 low re-affirmed.

Review details

Checked packages/cubejs-base-driver/src/type-detection.ts, BaseDriver.ts, and the unit tests against my earlier review. Contents match, so the findings below are unchanged.

Medium — Reduced sampling changes type detection for large result sets

packages/cubejs-base-driver/src/type-detection.ts:77-90

The old inline code iterated every row for each column when finding a matching type. The new code stops as soon as every column has at least one non-null value (if (unresolvedFields.size === 0) break;) and additionally caps at DB_TYPE_DETECTION_MAX_ROWS = 100.

Consequences:

1. When the first row has non-null values for all columns (the common case), only row 0 is inspected. If a later row widens the type — e.g. row 0 has int = 1, row 500 has int = 2147483648 — the new code returns int, the old code returned bigint. Same problem for int → decimal, date → timestamp, string → text (length ≥256).
2. Even in NULL-heavy cases where multiple rows must be scanned, capping at 100 rows silently drops information the old implementation used.

If the intent is "sample a few rows for speed," that's a reasonable tradeoff — but it should be a deliberate, documented change (called out in the commit / CHANGELOG) since it can change pre-aggregation column types in readOnly mode. A safer minimal fix would be to keep the "scan all rows, ignore NULLs" semantics, or at least keep collecting non-null values on already-resolved columns while scanning to satisfy other columns.

Low — v instanceof Number is effectively unreachable

packages/cubejs-base-driver/src/type-detection.ts:37

decimal: (v) => v instanceof Number || DECIMAL_REGEX.test(v.toString()),

instanceof Number matches only boxed new Number(...) objects; primitive numbers don't match. Primitive numbers reach the decimal matcher via DECIMAL_REGEX.test(v.toString()), so the instanceof check is dead. Not new to this PR (carried over from the old code) but worth cleaning up while the code is being moved.

Low — Test can't distinguish "detected from later row" from "fell back to text"

packages/cubejs-base-driver/test/unit/BaseDriver.test.ts:101-112

The "infers type from a later row when the first is NULL" test is good, and the all-null case on line 114-124 is well covered. Consider adding a case where a later row has a value that would widen the type (e.g., row 0 int = 1, row 1 int = 2147483648) to lock in whichever behavior is intended re: the medium finding above. Right now no test would catch a change in the sampling-vs-full-scan tradeoff.

Nits

• packages/cubejs-base-driver/src/BaseDriver.ts:1-6 — the file's @fileoverview / license header was removed alongside the sortByKeys cleanup. If the project convention is to keep those headers, restore it; if not, ignore.

Branch: fix/read-only-db-type-matcher •