Uh oh!
There was an error while loading. Please reload this page.
fix(driver-sql): emit an aggregate alias as one identifier, not a qualified reference - #14112
Conversation
…lified reference A cube query carrying a `timeDimensions[].granularity` answered 500 DATABASE_ERROR. The bucket expression was never the fault: an analytics measure is addressed on the wire as `<cube>.<measure>` and `ObjectQLStrategy` uses that dotted name verbatim as the driver-level aggregation `alias`, and this face bound the alias through knex's `??` placeholder — which parses an identifier rather than quoting one, splitting on `.` into `table.column`. The statement reached the database as ``count(*) as `showcase_delivery`.`count` `` and was refused before it ran. The granularity was the router, not the fault: `NativeSQLStrategy.canHandle` declines exactly on a granularity and that face already hand-wrote `AS "<measure>"`, so an un-bucketed cube query never reached this door while a bucketed one always did — which is why the reporter's controls were 200. `aliasIdentifierSql` renders an alias through `client.wrapIdentifier`, the same function knex calls on each segment it split out, so only the segmentation goes away. Applied at all five alias positions on the aggregate/window builders. Column references still bind through `??` and may still be qualified. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016ZC5rNQj3WEet5HAmmAkMs
…-sqlite3 driver Replaces the dogfood HTTP pin with one at the analytics layer, driving AnalyticsService against a real SqlDriver on better-sqlite3 — the driver the field report was filed against. It closes the link that was unpinned: that ObjectQLStrategy hands the driver an aggregation whose `alias` is the caller's cube-qualified measure name, which is why nothing but analytics ever put a dot in an alias. The un-bucketed cases are named for what they measure — the same door, without a granularity — rather than as a reproduction of the reporter's controls: this harness declares `nativeSql: false` so they take the failing door too, and the ablation reddens them alongside the bucketed cases. The reporter's controls travel the native face, whose fork is already pinned in both directions by native-sql-granularity-decline.test.ts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016ZC5rNQj3WEet5HAmmAkMs
📓 Docs Drift CheckThis PR changes 1 package(s): 8 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
⛔ 1 release-owned page(s) also name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 9 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin d0872a721ddcfd3fe237110db21ab6e427f25674 && git checkout d0872a721ddcfd3fe237110db21ab6e427f25674
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 09e4b0eceda31e7fed661ed334cff6b21d97425c 7d20ebda1b2a5e170c1b61afe9b4da0e4541b6ee && git checkout -B drift-repro 09e4b0eceda31e7fed661ed334cff6b21d97425c && git merge --no-ff 7d20ebda1b2a5e170c1b61afe9b4da0e4541b6ee
node scripts/docs-audit/affected-docs.mjs --json 09e4b0eceda31e7fed661ed334cff6b21d97425c
|
Uh oh!
There was an error while loading. Please reload this page.
Closes#13714
What was actually wrong
The filer's hypothesis — a date-truncation SQL emission problem on the granularity clause — is falsified. The bucket expression is correct on every dialect. What the database refuses is the alias.
An analytics measure is addressed on the wire as
CUBE.MEASURE, andObjectQLStrategyuses that dotted name verbatim as the driver-level aggregationalias— it is the key the caller reads its own number back under.driver-sqlbound that alias through knex's??placeholder, which does not quote an identifier so much as parse one:wrapStringsplits the value on.intotable.columnand re-quotes each segment. So the statement reached the database as:Not valid SQL on any dialect. It never runs, so the caller gets
DATABASE_ERROR/500 fromaggregate()'s terminal envelope (#11455) — a backend fault for a query that is spelled correctly.The granularity is the ROUTER, not the fault
Measured on
origin/main62a137bae, one driver, two axes crossed:nshowcase_delivery.countshowcase_delivery.countNativeSQLStrategy.canHandledeclines exactly ontimeDimensions[].granularity, and that face hand-writesAS "the measure name"— one quoted identifier, already correct. So an un-bucketed cube query never reaches the broken door and a bucketed one always does.That fork is why the reporter's controls were 200 while the same measure bucketed by month was a 500, and it means a repair aimed at
buildDateBucketExprwould have left the defect exactly where it was.It is also why the date-bucket parity pins (#3773, #3839) were green throughout: their reference side keys rows by the alias as a plain JS object key, where a dot is inert, and their probe aliases are bare names — the one input that breaks the SQL face is the one input they never supply. They are a declared control for this card, not evidence about it.
The fix
SqlDriver.aliasIdentifierSql()renders an output-column alias as exactly one identifier viaclient.wrapIdentifier— the same function knex itself calls on each segment it split out, so the dialect's quoting and quote-doubling are unchanged and a host-suppliedwrapIdentifierhook is still honoured. Only the segmentation goes away.Applied at all five alias positions on the aggregate/window builders: the bucketed groupBy projection, the plain groupBy projection,
count(*) as …,func(arg) as …, and the window-function alias. The fifth is the same one-line emission defect in the same builder family, named here rather than left half-closed.⛔ Column references are deliberately untouched.
fieldstill binds through??and may still be qualified —a.bin a reference position really istable.column. Only the name afterasis one identifier by definition. A fenced test asserts a qualifiedgroupByreference still resolves.⛔ Not fixed at the entry validator. The reporter's controls establish the entry layer is correct; turning a legitimate "count by month" into a 400 would hide the defect and break a common chart shape.
Tests
packages/drivers/driver-sql/src/sql-driver-13714-aggregate-alias-single-identifier.test.ts— the emission layer, across the live-dialect matrix (declareDialectCell): SQLite embedded, Postgres and MySQL live when provisioned and reported as a named unprovisioned cell otherwise. SweepsDateGranularity.options(the spec's own list, so a granularity the spec grows joins without an edit), plus the un-bucketed twin, the groupBy-alias twin, an alias containing" as ", and the controls.packages/services/service-analytics/src/__tests__/timedimension-granularity-driver-alias.test.ts— the road, on better-sqlite3, the driver the report was filed against. Closes the link that was unpinned: thatObjectQLStrategyhands the driver an aggregation whosealiasis the caller's cube-qualified measure name. The route itself (NativeSQLStrategydeclines on a granularity) is already pinned in both directions bynative-sql-granularity-decline.test.tsand is cited as a declared control rather than duplicated.week, capped because%Vneeds SQLite 3.46) is asserted as its own declared answer — the #6212NOT_IMPLEMENTED/501 capability refusal thatengine.aggregatereads offsupports.queryDateGranularityand serves in memory instead. The invariant spanning both answers is the card's: no shape answersDATABASE_ERROR.Ablation — direction predicted before running
Prediction, recorded first: reverting the emission fix should redden every non-bare-alias case;
weekon SQLite must stay green, since it is refused at the capability check before any statement is built.Mutation = the file restored to its
62a137baeblob. Confirmed on disk by blob hash (32687ef2…→9e8a58e6…) and by both marker counts moving (aliasIdentifierSql7 → 0; the looseas ??count 4 → 10) — never by an editor's exit code. For the cross-package leg,driver-sqlwas rebuilt on each leg and the mutation confirmed indist/byscripts/ablation-dist-preflight.mjs … --absent. Restore proven by state: whole-treegit status --porcelainclean, blob back to the HEAD blob, markers back, and the preflight confirming the marker is indist/again.Green in both directions, therefore ⛔ declared controls, not ablation evidence:
weekon SQLite (both suites), and driver-sql's two bare-alias controls (a bare alias is unchanged,an alias EQUAL to the field). The prediction held with no revision.The ablation also corrected an overclaim in the second suite: its un-bucketed cases reddened along with the bucketed ones, because that harness declares
nativeSql: falseand so forces them down the same door. They are named for what they measure — the same door, without a granularity — not as a reproduction of the reporter's controls, which travel the native face.Measured / NOT MEASURED, by dialect
OS_TEST_POSTGRES_URL— CI's live-dialect matrix jobOS_TEST_MYSQL_URL— CI's live-dialect matrix jobThe split lives in knex's shared formatter, not in a dialect, so PG and MySQL are broken by construction and repaired by construction — but that is an argument, not a reading. ⛔ This PR does not present a green on SQLite as a measurement for the other two; the live cells are declared and will report on the CI matrix job.
Verification
Union run on
7d20ebda1(git rev-parse --short HEADfrom that run):pnpm --filter @objectstack/driver-sql— 149 files / 2273 tests pass, 9 files + 136 tests skipped;tsc --noEmitexit 0 over a 569-file program that contains both changed files (--listFiles).pnpm --filter @objectstack/service-analytics— 86 files / 1850 tests pass;tsc --noEmitexit 0 over a 537-file program containing the new pin.node scripts/pm/dispatch-gates.mjs(both output sections read whole; re-derived after the diff changed, and again aftergit fetch origin main— the set was stable at 36 across both reads): 36 commands, 35 green.check:dual-build-cjs-loadsandcheck:type-check-debtfirst exited 3 = PREREQUISITE NOT MET, both naming the same missing full-workspacedist/. That closure was then built (turbo run build --filter='./packages/*' --filter='./packages/*/*', 70/70 tasks) and both re-ran green — reported as measured only after clearing the prerequisite they named, never as a pass on the prerequisite state.scripts/check-test-completeness.mjsexits 3 because it grades a savedturbo run testlog, which is a CI artifact this container does not produce.Session: https://claude.ai/code/session_016ZC5rNQj3WEet5HAmmAkMs
Generated by Claude Code