Uh oh!
There was an error while loading. Please reload this page.
fix(analytics,driver-sql): coerce datetime filter comparands to storage form - #2034
Merged
Conversation
…ge form
Dashboard time-series charts and "last N months" KPIs that filter/group by a
`Field.datetime` column silently returned "No rows" even though data existed;
`Field.date` columns worked.
Root cause: `NativeSQLStrategy` expands dashboard relative-date tokens
(`{12_months_ago}`, `{today}`, …) to ISO date strings and binds them directly
into raw SQL, bypassing the driver's CRUD filter coercion. Under better-sqlite3
a `Field.datetime` column is stored as an INTEGER epoch (ms), so
`assessed_at >= '2025-06-18'` is a TEXT-vs-INTEGER affinity compare that is
always false → empty result. `Field.date` stores ISO TEXT and compared fine.
Fix: make the comparand coercion type- and storage-aware via a new optional
`StrategyContext.coerceTemporalFilterValue` hook, wired by the analytics plugin
to the driver's public `SqlDriver.temporalFilterValue` — the single source of
truth for the storage convention (reuses the existing `coerceFilterValue`).
Coercion is dialect-correct: SQLite `Field.datetime` → epoch ms; `Field.date`
text and native-timestamp dialects (Postgres/MySQL) are left unchanged, so
Postgres is never handed an epoch integer. Also gated the driver's existing
datetime→epoch coercion on `isSqlite` to make the native-timestamp path correct.
Applied to gte/lte/gt/lt/equals, in/notIn, and the dateRange/timeDimension path.
Tests:
- service-analytics: NativeSQLStrategy binds epoch for a datetime `gte`/range/in,
leaves ISO text unchanged when the hook reports no coercion (Postgres/date),
and is backward-compatible with no hook.
- driver-sql: E2E SQLite repro proving 0→4 rows once coerced; dialect-gating unit
test asserting Postgres/MySQL are NOT epoch-coerced (no regression).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>…+ follow-ups Append an addendum to ADR-0053 covering the gap it did not reach: the analytics NativeSQLStrategy raw-SQL path bypasses the driver's dialect-aware filter coercion, producing the datetime analogue of Phase 1's date equality miss (epoch-vs-ISO-string → 0 rows). Records the hotfix increment (6f4cf856e) and two follow-ups: formalize temporalFilterValue onto the IDataDriver contract, and add a cross-driver temporal conformance matrix as the runtime regression backstop. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📓 Docs Drift CheckThis PR changes 3 package(s): 95 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
Uh oh!
There was an error while loading. Please reload this page.
xuyushun441-sys pushed a commit
that referenced
this pull request
Jul 28, 2026
`ObjectQLStrategy` hands `engine.aggregate()` uncoerced ISO bounds and relies on the driver's own CRUD filter coercion — the justification for NOT doing what `NativeSQLStrategy` does with `coerceTemporal` (#2034). That is an assumption about the driver, not the strategy, so it is pinned against real SQLite: a `Field.datetime` column stores INTEGER epoch ms, where an uncoerced ISO TEXT comparand would match nothing and quietly make the #3650 fix a no-op on the column type dashboards use most. The assumption holds — the window applies on both `datetime` and `date` storage. Writing it surfaced an unrelated pre-existing gap on the SAME query shape: bucketing an epoch-stored `datetime` collapses every row into one NULL bucket (`strftime` reads a bare INTEGER as a Julian day; SQLite advertises the granularity so the engine never falls back to in-memory bucketing). Not fixed here — pinned as a KNOWN GAP test so it is not later rediscovered as "the dateRange fix did nothing", with the assertion it should become once fixed. Co-Authored-By: Claude <noreply@anthropic.com>
os-zhuang added a commit
that referenced
this pull request
Jul 28, 2026
…e-granularity 图表必走的那条路 (#3650) (#3766) * fix(analytics): ObjectQLStrategy applies timeDimensions[].dateRange (#3650) `execute()` built its engine filter purely from `normalizeAnalyticsFilters`, which reads only `query.where`. `dateRange` is a SIBLING of `where`, never folded into it, so the window was dropped on the floor — no error, and the chart plotted all of history. Not a corner case: `NativeSQLStrategy.canHandle` declines any query carrying a `granularity`, so a date-bucketed trend lands on this path on EVERY driver, and a bucketed trend is exactly the shape that also carries a range. It also made `compareTo` structurally dead — `runCompare` shifts `dateRange` and nothing else, so both passes issued a byte-identical aggregate and every `<measure>__compare` equalled its primary. The window now lowers to an inclusive `{$gte,$lte}` on the resolved field — the shape NativeSQL binds as BETWEEN and the memory driver builds as a `$match`. No storage coercion here on purpose: this path goes through `engine.aggregate()`, where the driver's own CRUD filter coercion applies. Same-field composition is fixed alongside it, because the window makes the collision routine: operands naming DIFFERENT operators still share one entry, colliding ones become their own `$and` conjunct so the engine intersects them instead of the last writer winning. `generateSql()` renders the window as a parameterised BETWEEN to match. A cross-object time dimension is still rejected, now reported as the bucketing error it is. Co-Authored-By: Claude <noreply@anthropic.com> * test(driver-sql): pin the storage-coercion assumption #3650 rests on `ObjectQLStrategy` hands `engine.aggregate()` uncoerced ISO bounds and relies on the driver's own CRUD filter coercion — the justification for NOT doing what `NativeSQLStrategy` does with `coerceTemporal` (#2034). That is an assumption about the driver, not the strategy, so it is pinned against real SQLite: a `Field.datetime` column stores INTEGER epoch ms, where an uncoerced ISO TEXT comparand would match nothing and quietly make the #3650 fix a no-op on the column type dashboards use most. The assumption holds — the window applies on both `datetime` and `date` storage. Writing it surfaced an unrelated pre-existing gap on the SAME query shape: bucketing an epoch-stored `datetime` collapses every row into one NULL bucket (`strftime` reads a bare INTEGER as a Julian day; SQLite advertises the granularity so the engine never falls back to in-memory bucketing). Not fixed here — pinned as a KNOWN GAP test so it is not later rediscovered as "the dateRange fix did nothing", with the assertion it should become once fixed. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com>
This was referenced Jul 28, 2026
os-zhuang added a commit
that referenced
this pull request
Jul 28, 2026
…nt (#3773) (#3775) On SQLite every trend chart bucketed by day/week/month/year over a `Field.datetime` column put every record in a single `(null)` bucket — one bar carrying the whole total. The measure was right; only the bucket key was wrong. better-sqlite3 stores a `Field.datetime` as INTEGER epoch milliseconds, and `buildDateBucketExpr` emitted a flat `strftime('%Y-%m', col)`. SQLite reads a bare integer as a Julian day number, and epoch ms is far outside the legal range, so `strftime` returned NULL for every row. Nothing downstream noticed: SQLite advertises `queryDateGranularity.month`, so `engine.aggregate` pushes the bucketing down, and its in-memory fallback only engages for an unsupported granularity or a non-UTC timezone. The SQLite expression is now storage-aware, sharing one `isEpochStoredDatetime` predicate with the filter-comparand coercion added for the same root cause in #2034 — a window and a bucket that disagree about storage is exactly how an epoch column ended up correctly filtered and then entirely bucketed as NULL. Postgres and MySQL are untouched and pinned as such: `defineColumn` maps `Field.datetime` to a native timestamp there. Two details are load-bearing and each has a test that fails without it: - The conversion dispatches on each stored value's type, not just the declared one. A SQLite `Field.datetime` column is genuinely mixed-form — `formatInput` passes datetime values through, so a `Date` lands as INTEGER while an ISO string lands as TEXT. Dividing TEXT by 1000 coerces it to its leading year, filing live rows under 1970 — worse than the NULL it replaces. - Division is `/1000.0`, not `/1000`: integer division truncates toward zero, so a pre-1970 instant would surface a day late. `bucketDateValue` (the in-memory fallback) now reads a finite number as epoch ms. `new Date(String(1767225600000))` is an Invalid Date, so fixing only the driver would have traded one wrong answer for two different ones — the two paths have to label the same instant identically for a drill-down to survive crossing them. Coverage goes through `initObjects` rather than `knex.schema.createTable`, which is why the existing date-bucket suite never saw this: its fixture is ISO TEXT, the half `strftime` parses natively. Four granularities x both storage forms, plus a mixed-form column, a pre-1970 instant, and dialect gating for pg/mysql. `SqliteWasmDriver` inherits the expression, so it carried the bug and is pinned too. Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Dashboard time-series charts and "last N months/days" KPIs that filter or group by a
Field.datetimecolumn silently returned 0 rows on SQLite, even though the data existed (charts onField.datecolumns worked).Root cause: the analytics
NativeSQLStrategybuilds raw SQL and runs it viaengine.execute, which bypasses the driver's dialect-aware filter coercion (coerceFilterValueindriver-sql). Dashboard relative-date tokens ({12_months_ago}, …) resolve to ISO date strings ("2025-06-18");Field.datetimeunder better-sqlite3 stores an integer epoch (ms), and in SQLite an INTEGER always sorts before any TEXT, soepoch >= 'ISO'is always false. This is thedatetimeanalogue of thedatewrite/filter asymmetry that ADR-0053 Phase 1 fixed — on the analytics-raw-SQL path ADR-0053 didn't reach (same bug family #1874).Fix
The driver is the single source of dialect truth for filter-value coercion; the analytics raw-SQL path now routes through it instead of re-deriving types from value shape.
driver-sql: gate the datetime→epoch coercion behindisSqlite(native-timestamp dialects leave the ISO/Date comparand intact → Postgres/MySQL correct), and exposetemporalFilterValue(object, field, value)delegating to the existingcoerceFilterValue(one code path).service-analytics:NativeSQLStrategyresolves the real(object, column)each member binds against and coerces temporal filter values via the driver, threaded intobuildFilterClause(gte/lte/gt/lt/equals,in/notIn) and thedateRangeBETWEEN path. NewStrategyContext.coerceTemporalFilterValuehook wired from the data engine.spec: added thecoerceTemporalFilterValuecontract hook.No
CAST(col …)is emitted (indexes preserved). When the hook is absent, behaviour is byte-for-byte identical to before.Verification
assessed_at >= '2025-06-18'(datetime) 0 → 4 after driver coercion;Field.datetext path unchanged (4).dateare not (no regression).main:driver-sql169 passed,service-analytics134 passed; spec/driver-sql/service-analytics build green.ADR
Records this as the first increment in a new Addendum to ADR-0053 (driver-as-single-source-of-dialect-truth; formalize
temporalFilterValueonto theIDataDrivercontract; add atype × operator × token × driverconformance matrix as the runtime regression backstop).🤖 Generated with Claude Code