Skip to content

Commit 2791138

Browse files
fix(service-analytics): qualify a native-SQL base column whenever the statement joins a relationship path, read from the hop resolver's joins, not cube.joins (#21266)
Fixes #21249 Clause-②: no ## What changed On the native-SQL strategy, a bare base-table column is now qualified with the base table (`"deal"."note"`) exactly when the statement joins something. What the statement joins is read from the joins the one hop resolver (`hop-object.ts`, consumed unchanged) registered for the query. It is no longer read from whether the cube declares `joins`. ⛔ No new resolver. - **`qualifyAndRegisterJoin`** reads `joins.qualifyBaseColumns`, a flag the statement's `StatementJoins` now carries, instead of `canJoin = !!cube?.joins && Object.keys(cube.joins).length > 0`. - **`generateSql`** compiles the query once with bare base columns. When that compile registered any join, it compiles the query once more with every base column qualified. The flag cannot be read at call time, because joins are registered lazily: whichever member walks a relationship path first registers the join, and an absorbed `$or` takes back the joins its branches registered. In the card's own pair, `note` is resolved before `owner.email` registers the join. Ablation A2 below measures that a call-time read (`joins.size > 0`) leaves the card's pair broken. The method is split into `compileClauses` (the SELECT / GROUP BY / WHERE half, unchanged in place) and `assembleStatement` (the allowlist, read-scope and assembly half, unchanged in place). Both compiles walk the same members through the same resolver, so the second registers the same joins. A statement that joins nothing is compiled once. - **`resolveFieldSql`'s fallback** for a filter member the cube does not declare (`where: { id }`) used to return the bare name. It now goes through `qualifyAndRegisterJoin` like a declared member. Measured below: the fallback was the same 500 beside a join. - Every base-column reference now takes the one rule: the select list, GROUP BY, WHERE (the `where`, the dataset scope, measure filters), measures and time-dimension windows. **ORDER BY** emits the member's output alias (`ORDER BY "note"`), never a table column, so it is not a qualification site. See the Acceptance notes for ordering by a member the query does not select. ## Measured: `POST /api/v1/analytics/query` and `/sql` on the real dispatcher route Setup: `AnalyticsServicePlugin` over a real `ObjectQL` engine and `SqlDriver`, a signed-in caller, the real `dispatcher-plugin` mount, SQLite in memory and a private PostgreSQL 16.14. A configured cube over `deal` declares **no** join. `owner` is a lookup whose `reference` is `person`, and `person` also declares `note`, `amount`, `closed_on` and `id`. A second cube declares the join (the control). Before: `origin/main` at `4727fcb2`. After: this branch's build, re-taken at the merge with `origin/main` (`1b176fbf`). The scratch probe was deleted. | query (no-join cube unless named) | native, SQLite before → after | native, PostgreSQL 16.14 before → after | ObjectQL face | |:--|:--|:--|:--| | dimensions `note` + `owner.email` (the card's pair) | **500** → 200, 3 groups by deal note | **500** (42702 `note`) → 200 | 200 → 200, same groups | | the same pair, the dimension declared on the cube (`owner_email`) | **500** → 200 | **500** → 200 | 200, unchanged | | the pair with `where: { note: 'x' }` and `order: { note: 'asc' }` | **500** → 200 | **500** → 200 | 200, unchanged | | `sum(amount)` by `owner.email` | **500** → 200 (15 / 8) | **500** (42702 `amount`) → 200 | 200, unchanged | | `where: { note }` by `owner.email` | **500** → 200 | **500** → 200 | 200, unchanged | | a `closed_on` time-dimension window by `owner.email` | **500** → 200 | **500** (42702 `closed_on`) → 200 | 200, unchanged | | `where: { id: 'd1' }` (undeclared member) by `owner.email` | **500** → 200 | **500** (42702 `id`) → 200 | 200, unchanged | | ad-hoc query over `deal` (inferred cube), the pair | **500** → 200 | **500** → 200 | 200, unchanged | | control: the declared-join cube, the pair | 200 → 200, statement byte-identical | same | 200, unchanged | | control: `note` alone, either cube, or an absorbed `$or` over `owner.email` | 200 → 200 | 200 → 200 | 200, unchanged | The compiled statement for the card's pair, before: `SELECT note AS "note", "owner"."email" AS "owner.email", COUNT(*) AS "count" FROM "deal" LEFT JOIN "person" "owner" ON "deal"."owner" = "owner"."id" GROUP BY note, "owner"."email"`. After, it is the statement the declared-join cube always compiled: `SELECT "deal"."note" AS "note", … GROUP BY "deal"."note", "owner"."email"`. **The no-path control, stated (dispatch Zone 2, item 3).** A statement that joins nothing stays **unqualified, as before**, on a cube that declares no join. On a cube that declares a join, a query that uses none of it now also compiles bare columns. The `/sql` dry run shows `SELECT note AS "note", COUNT(*) AS "count" FROM "deal" GROUP BY note`, where before it showed `"deal"."note"`. The answer is the same on both drivers: the statement reads one table, so both spellings name one column. This follows from triage's predicate (what the query joins, not `cube.joins`). The other way to keep that statement byte-identical is to keep reading `cube.joins` beside the predicate, which is what triage rules out. A third reading, qualifying every base column always, was measured and rejected: it reddens 64 tests in 21 files of this package, among them `native-sql-rls.test.ts`'s deliberate pin that a single-object cube keeps bare columns. ## Pins **New file:** `packages/services/service-analytics/src/__tests__/native-sql-base-column-qualify.test.ts`. It uses the plugin's own composition over a real engine, a SQLite cell and a PostgreSQL cell (a named skip without `OS_TEST_POSTGRES_URL`), and both faces. It has 12 tests, 6 per cell. Each serve pin checks the rows on both faces, that the native face answered with one raw statement and no engine aggregate, and the compiled statement. 1. The card's pair, grouped by the deal's `note`: (x, a, 2) · (x, b, 1) · (y, b, 1). The person rows carry different `note` / `amount` / `closed_on` values, so a statement that read the person's column would answer differently. 2. The pair with a `where` on `note` and an `order` by it. 3. The other emitters beside the join: a measure (`sum(amount)`), a time-dimension window (`closed_on`), and a member the cube does not declare (`where: { id }`). 4. A join registered by a LATER member: `dimensions: ['note']` with `where: { 'owner.email': … }`. The SELECT list is compiled before the filter joins `owner`. Native face only, because the ObjectQL face refuses a cross-object filter of its own accord. The reversed pair is pinned too. 5. CONTROL: the declared-join cube compiles the pair to the exact statement it compiled on `4727fcb2`, and the no-join cube now compiles the same statement. 6. CONTROL: a statement that joins nothing keeps bare columns, on either cube, and when an absorbed `$or` takes its join back. ## Ablations The tests import the subject by relative path (`../plugin.js`), so each run reads `src` and there is no `dist` leg. Every mutation went through `scripts/ablation-replace.mjs` in WRAP mode, with an outer `trap` restore on `EXIT INT TERM` against the absolute path. They ran from committed `2a575561`, with the PostgreSQL cell live. Predictions were written before each run. | ablation | mutation | predicted | observed | |:--|:--|:--|:--| | A1 | `canJoin` put back on `cube.joins` (`joins.qualifyBaseColumns` → the removed expression) | all 12 red: pins 1–4 on the 500 / bare statement, pin 5 on "the no-join cube compiles the same statement", pin 6 on the declared cube's bare statement | **12 failed / 0 passed** | | A2 | the flag read at call time (`joins.qualifyBaseColumns` → `joins.size > 0`) | 8 red (pins 1, 2, 4, 5 on each cell). Pin 3 stays green because there the join is registered before the base column. Pin 6 stays green because nothing joins | **8 failed / 4 passed** | | A3 | `resolveFieldSql`'s fallback back to `return fieldName;` | 2 red (pin 3 on each cell) | **2 failed / 10 passed** | Each mutation landed: anchor 1 → 0, and the blob changed from `d2c652842c9f` to A1 `06e880595f77`, A2 `79a30a8137c0` and A3 `09066c0079e7`. Each was restored and proven: the blob equals the HEAD blob `d2c652842c9f`, and `git diff HEAD` is empty. ## Fixture triage The full `service-analytics` suite turned up exactly three expectations that pinned a bare base column inside a statement that joins. That is the shape this PR corrects. Each was replaced, and its comment, which described the old rule, now describes the new one. The tests' subjects (member resolution, spelling parity, traversal unification) are unchanged. - `analytics-service.test.ts`, "should resolve client-style lookup.field member references": `SUM(amount)` → `SUM("opportunity"."amount")`. - `infer-cube-relation-traversal.test.ts`, "unifies a dotted key riding alongside a bare one": `WHERE (industry = $1 AND …)` → `WHERE ("crm_account"."industry" = $1 AND …)`. - `infer-cube-where-spelling-parity.test.ts`, "bare and dotted keys reach parity together": `WHERE (stage = $1 AND …)` → `WHERE ("deal"."stage" = $1 AND …)`. The file header and block 2's comment named the old rule, and now name the new one. Block 2's assertion (a statement that joins nothing stays bare) is unchanged and green. Consumer radius, run against this branch's build at `560ab361` with `OS_TEST_POSTGRES_URL` set: every test file that drives the analytics service in `packages/rest` (23 files), `packages/runtime` (19), `packages/drivers/driver-memory` (27) and `packages/drivers/driver-sql` (7). The results were 359 / 359, 551 / 551, 863 / 863 and 133 passed / 1 skipped, with no failures. The 9 `packages/qa/dogfood` files that drive analytics are **NOT MEASURED**: their build closure (the example apps, the CLI) was not built here. They pin no statement text (`git grep` for `GROUP BY` / `LEFT JOIN` / `SELECT` over them has zero hits). They are left to the required `Dogfood Regression Gate`. ## Docs `git grep -niE` for `ambiguous`, `qualif…`, `declares (no) join`, `bare column`, `single-object cube`, `cube can/has join` and `declared join` over `content/docs/**`, excluding `releases/` and `references/`, gave no sentence about analytics column qualification. The native-SQL join sentences are `protocol/objectql/query-syntax.mdx:517` (the positive control: "on the native-SQL path those joins serve dimensions, measures and filter members", about a dataset's `include`) and `data-modeling/analytics.mdx:189-193` (dataset joins come from `include`). Both stay true. No page edited. ## Verification at `560ab361` `560ab361` is this branch's head. It carries `origin/main` at `434c6c7c` merged in (no overlap with this diff's files), with install and the build refreshed after the merge. - `pnpm --filter @objectstack/service-analytics exec vitest run`, with the PostgreSQL cells live: 165 files, **3827 passed, 0 skipped, 0 failed**. `typecheck` (`tsc --noEmit`): exit 0, and its program lists all 5 touched `.ts` files (`--listFiles`). - Base reading on `4727fcb2`, cells skipped: 164 files, 3758 passed / 56 skipped. - Gates: `node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands` derives 61 commands, and all 61 exit 0 at `560ab361`. `--ran` reconciliation: "61 derived, 61 run, 0 NOT-MEASURED, 0 UNRUN", with every exit code recorded. `check:dual-build-cjs-loads` first answered `PREREQUISITE NOT MET` (exit 3), because some packages had no `dist`. After `pnpm build` (72 tasks, 71 cached) it exits 0, and that is the run recorded. - Lint, a declared narrowing: `eslint --no-inline-config --format json` over the 5 touched `.ts` files at `560ab361` reports 5 files, 0 errors and 0 warnings. ① All 5 are inside the population `eslint.config.mjs` lints (`**/*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}`, minus the global `node_modules` / `dist` / `build` / `.next` / `.turbo` ignores), and the changeset `.md` is in no `files` glob. ② The count of 5 is read from the JSON output. ③ The config enables no type-aware linting (`--print-config` gives `parserOptions` `{"ecmaVersion":"latest","sourceType":"module"}`, with no `project`), so this diff cannot move a verdict on an untouched file. The repo-wide `pnpm lint` is left to CI. ## Acceptance notes - **Ordering by a member the query does not select is a separate defect, reported to the seat and not handled here.** The ORDER BY emitter writes the request key as an identifier (`ORDER BY "note"`), which names an output column only when that member is selected. Measured after this PR, all through `POST /api/v1/analytics/query`, native face, with the ObjectQL face answering 200 in every row: - `{ dimensions: ['owner.email'], order: { note: 'asc' } }` answers 500 on both drivers. On PostgreSQL that is 42702, `note` ambiguous at the ORDER BY. - `{ dimensions: ['note'], order: { amount: 'asc' } }` with no join at all answers 500 on PostgreSQL (42803, must appear in GROUP BY). On SQLite it answers 200, ordered by an arbitrary row's value. - `order: { 'owner.email': 'asc' }`, unselected, answers 500 on both drivers (42703, column "owner.email" does not exist). Qualifying the column would not mend it (PostgreSQL would still answer 42803). The fix is a decision about what an unselected order key means. - The other `cube.joins` readers in this file (`canHandle`'s federated decline, and the three `readScopedObjects` fallbacks) were not seen failing at a door in these measurements. They stay with PR #21247's acceptance notes. - The `/sql` dry run's text changes for every statement that joins through an undeclared lookup (now qualified), and for a declared-join cube's statement that joins nothing (now bare). The consumer radius above found no test outside this package that pins that text. ## Deviation from the declared file surface The claim names `native-sql-strategy.ts` and its tests, plus a changeset. The diff stays inside that. The three fixture files above are this strategy's tests in `service-analytics`. The `resolveFieldSql` fallback is in the same file and is the same 500 beside a join, measured at the door. --- _Generated by [Claude Code](https://claude.ai/code/session_01DiCSbmJrkzNhuEAier4VoJ)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent f148852 commit 2791138

6 files changed

Lines changed: 451 additions & 26 deletions

File tree

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
---
2+
"@objectstack/service-analytics": patch
3+
---
4+
5+
fix(service-analytics): on the native-SQL strategy, a base-table column is qualified with its table whenever the statement joins a related object, not only when the cube declares a join
6+
7+
Clause-②: no
8+
9+
A cube that declares no join still joins a lookup's declared `reference` when a query names a relationship path through it (`owner.email`). The native-SQL strategy qualified base-table columns only for a cube that declares a join, so it wrote them bare beside the joined object. When that object declares a column of the same name, the database refused the statement as ambiguous, and `POST /api/v1/analytics/query` answered `500 DATABASE_ERROR` on SQLite and on PostgreSQL. The ObjectQL strategy answered `200` for the same query.
10+
11+
**Before and after**, measured on a configured cube over a `deal` object that declares no join, whose lookup `owner` points at a person object that also declares `note`, `amount`, `closed_on` and `id`:
12+
13+
- Dimensions `note` and `owner.email`, with or without a `where` on `note` and an `order` by it: `500` → `200`, one group per (deal note, owner email).
14+
- A `sum` over `amount`, a `timeDimensions` window on `closed_on`, or a `where` on `id`, each grouped by `owner.email`: `500` → `200`.
15+
- An ad-hoc query over the object, whose inferred cube never declares a join: the same.
16+
17+
The strategy now reads what the statement actually joins, from the one relationship-path resolver, and qualifies every base column in the select list, the grouping, the filters, the measures and the time windows. A statement that joins nothing keeps bare columns. That is now also true on a cube that declares a join when the query uses none of it: the statement it shows on `POST /api/v1/analytics/sql` reads `note` where it read `"deal"."note"`, and the answer is the same.
18+
19+
**Unchanged.** The ObjectQL strategy; every query on a cube that declares the join it uses; every statement that joins nothing on a cube that declares no join; the refusals.

‎packages/services/service-analytics/src/__tests__/analytics-service.test.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -247,7 +247,9 @@ describe('NativeSQLStrategy', () => {
247247

248248
expect(sql).toContain('LEFT JOIN "account" ON "opportunity"."account" = "account"."id"');
249249
expect(sql).toContain('"account"."industry" AS "account.industry"');
250-
expect(sql).toContain('SUM(amount)');
250+
// [#21249] The cube declares no join, but the statement joins `account`, so
251+
// the base column the measure sums is qualified against the base table.
252+
expect(sql).toContain('SUM("opportunity"."amount")');
251253
});
252254

253255
it('should execute query and return structured result', async () => {

‎packages/services/service-analytics/src/__tests__/infer-cube-relation-traversal.test.ts‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -352,10 +352,11 @@ describe('[#5739] the object and array `where` spellings converge on one travers
352352
);
353353

354354
expect(dimensions).toEqual(['industry', 'owner.region']);
355-
// The bare column stays BARE: `qualifyAndRegisterJoin` qualifies plain
356-
// identifiers only when the cube declares `joins`, and an inferred cube never
357-
// does — minting a dotted dimension does not change that (#5353's block 2).
358-
expect(sqls[0]).toContain('WHERE (industry = $1 AND "owner"."region" = $2)');
355+
// [#21249] The base column is qualified: the statement joins `owner`, so
356+
// `industry` beside it is written against the base table, whether or not
357+
// the cube declares `joins` — an inferred cube never does. Left bare, a
358+
// joined target that also declares `industry` makes it ambiguous.
359+
expect(sqls[0]).toContain('WHERE ("crm_account"."industry" = $1 AND "owner"."region" = $2)');
359360
});
360361
});
361362

‎packages/services/service-analytics/src/__tests__/infer-cube-where-spelling-parity.test.ts‎

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -20,9 +20,10 @@
2020
*
2121
* ## Why this had no user-visible symptom (the issue's own observation class)
2222
*
23-
* `NativeSQLStrategy.resolveFieldSql` falls back to the bare column name for a
24-
* member the cube does not declare, and `qualifyAndRegisterJoin` leaves bare
25-
* columns bare on a cube with no `joins` — which an ad-hoc cube never has. So
23+
* `NativeSQLStrategy.resolveFieldSql` falls back to the column the member names
24+
* for a member the cube does not declare, and `qualifyAndRegisterJoin` leaves
25+
* that column bare in a statement that joins nothing ([#21249]: what the
26+
* statement joins, not the cube's `joins`, which an ad-hoc cube never has). So
2627
* both spellings compiled the same SQL before the fix and still do; block 2
2728
* measures that rather than asserting it. The divergence was confined to the
2829
* dimension VOCABULARY, which is why this was filed as an observation and fixed
@@ -317,9 +318,9 @@ describe('[#5353] the seeded dimensions change no verdict and no statement', ()
317318

318319
expect(arraySpelling.sqls).toEqual(objectSpelling.sqls);
319320
// A bare column stays bare: `qualifyAndRegisterJoin` only qualifies when the
320-
// cube declares `joins`, and an inferred cube never does. This is the whole
321-
// reason #5353 was an observation rather than a defect — and the assertion
322-
// that keeps a newly-DECLARED dimension from starting to qualify.
321+
// statement joins something ([#21249]), and this one joins nothing. This is
322+
// the whole reason #5353 was an observation rather than a defect — and the
323+
// assertion that keeps a newly-DECLARED dimension from starting to qualify.
323324
expect(objectSpelling.sqls[0]).toContain('WHERE stage = ');
324325
expect(objectSpelling.sqls[0]).not.toContain('"deal"."stage"');
325326
});
@@ -459,10 +460,11 @@ describe('[#5353/#5739] a dotted `where` key is unified too — as a traversal',
459460
it('bare and dotted keys reach parity together when both ride along', async () => {
460461
// Before the ruling only `stage` was unified and the whole query was refused
461462
// for `region`; now both keys are minted, on both spellings, and the query
462-
// runs. The bare column stays BARE in the statement — `qualifyAndRegisterJoin`
463-
// qualifies plain identifiers only for a cube declaring `joins`, and minting a
464-
// dotted dimension does not give an inferred cube one (block 2's rule, still
465-
// holding with a traversal in the same filter).
463+
// runs. [#21249] The traversal makes the statement join `owner`, so the base
464+
// column is qualified against the base table on both spellings alike —
465+
// `qualifyAndRegisterJoin` reads what the statement joins, not whether the
466+
// cube declares `joins` (block 2's bare column is the statement that joins
467+
// nothing).
466468
const both = [['stage', '=', 'won'], ['owner.region', '=', 'NA']];
467469
const array = await inferredDimensions(both, NO_REGION);
468470
const object = await inferredDimensions(
@@ -473,6 +475,6 @@ describe('[#5353/#5739] a dotted `where` key is unified too — as a traversal',
473475
expect(array.dimensions).toEqual(['owner.region', 'stage']);
474476
expect(object.dimensions).toEqual(array.dimensions);
475477
expect(object.sqls).toEqual(array.sqls);
476-
expect(array.sqls[0]).toContain('WHERE (stage = $1 AND "owner"."region" = $2)');
478+
expect(array.sqls[0]).toContain('WHERE ("deal"."stage" = $1 AND "owner"."region" = $2)');
477479
});
478480
});

0 commit comments

Comments
 (0)