Repository navigation
Commit 097ef80
Fixes #21042
Clause-②: yes (widening)
## What this changes
The analytics native-SQL strategy (`NativeSQLStrategy`, the default on a
SQL driver) skipped three aggregate policies that
`SqlDriver.aggregate()` applies. So one route answered different
numbers, or a `500`, depending on which strategy served it. This PR
follows the route ruling on #21042 (comment `5925613967`): the operand
policies are hoisted into `@objectstack/core`, beside
`AGGREGATE_ANSWER_KIND`, and both faces read them from there.
- **`packages/core/src/utils/aggregate-answer.ts`**
(`@objectstack/core`, `minor`). It takes:
- `AGGREGATE_ACCUMULATION`, moved from `driver-sql` with its docblock
(the docblock and the table are byte-identical to the base, apart from
the `export` keyword);
- `aggregandColumnClass({ type, multiple })`, the one column-class
predicate (`'fractional'`, `'integral'`, `'boolean'`, or none);
- `POSTGRES_BOOLEAN_AGGREGAND_CAST`, the #11635 cast, as a `Record` over
`AggregationFunction`: `sum` / `avg` / `min` / `max` are cast, the
counts never are;
- `doubleAccumulationOperand(operand, dialect)`, the double operand with
the dialect as a parameter;
- `aggregandOperandSql(func, columnClass, dialect, operand)`, the one
composition both faces emit (the cast inside, the double operand around
it).
No `index.ts` line was added: the module is already exported.
- **`packages/drivers/driver-sql/src/sql-driver.ts`** (`patch`), in the
ruled regions only. The `AGGREGATE_ACCUMULATION` table becomes a pointer
plus an import. `isFractionalNumericType` is deleted. The two registry
fills fill `fractionalNumericFields` through the predicate.
`accumulatesInDouble` / `doubleAccumulationOperand` are replaced by
`aggregandColumnClassOf`, which maps the driver's registries onto the
predicate's classes. In `aggregate()`, the private boolean-cast
condition and the accumulation call become one
`aggregandOperandSql(funcName, class, this.dialectName, '??')`.
-
**`packages/services/service-analytics/src/strategies/native-sql-strategy.ts`**
(`patch`), in two places.
- **`resolveMeasureSql`** wraps the column it hands `AGGREGATE_SQL` /
`CONDITIONAL_AGGREGATE_SQL` in `aggregandOperandSql`. The column class
comes from the declaration the host already relays
(`declaredValueShape`), on the object the column lives on
(`columnObjectOf`, the one hop resolver). The dialect comes from
`sqlDialect`, which the strategy already reads.
- **The `execute` shaping point** that PR #21040 added now folds a
`null` measure answer to `emptyGroupValueFor(measure.type)`
(`@objectstack/spec`). It does this for every measure, measure-scoped
ones included, before the number presenter, in `driver-sql`'s order. The
dataset door's `DatasetExecutor` fill stays, and it is idempotent on a
folded row.
- The one line outside those two regions is the `generateSql` call site,
which now passes `ctx` to `resolveMeasureSql`.
- No native copy of any policy, and no runtime hook asks the driver: the
rejected (C) route was not taken. `canHandle`, `buildFieldMeta`, the
hop-object sites, the filter / text-match rendering,
`analytics-service.ts` and `field-read-admission.ts` are untouched.
## The card's table, before and after
Measured through `AnalyticsService.query` (the cube door, which `POST
/api/v1/analytics/query` relays verbatim) and
`AnalyticsService.queryDataset` (the dataset door), on
`AnalyticsServicePlugin` over a real ObjectQL engine and `SqlDriver`.
**Native** is the plugin's own composition (`NativeSQLStrategy`
answered, with one raw statement and no engine aggregate). **ObjectQL**
is the same composition narrowed to `engine.aggregate`. "Before" is the
strategy file at the base `d34aa58a2a`; "after" is this branch at
`61aab5013a`. Neither merge since then touches the aggregate code paths,
and the pins below are green at `ef1f9d8484`.
Fixture:
- group `f`: `frac` (a `number` column) holds 0.1 and 0.2, and `flag`
holds true and false;
- group `i`: `stars` (a `rating` column) holds seven 1s and two 2s, and
`flag` holds 7 trues and 2 falses;
- group `n`: every aggregand is NULL in all three rows.
The measure-scoped measures filter on `tag = x`, which only group `f`
holds.
**PostgreSQL 16.13** (a private local server; the ObjectQL column is the
same before and after):
| measure | group | door | native before | native after | ObjectQL |
|:--|:--|:--|:--|:--|:--|
| `sum(frac)` | f | cube, dataset | `0.3` | `0.30000000000000004` |
`0.30000000000000004` |
| `avg(frac)` | f | cube, dataset | `0.15` | `0.15000000000000002` |
`0.15000000000000002` |
| `avg(stars)`, an integer column | i | cube, dataset |
`1.222222222222222` | `1.2222222222222223` | `1.2222222222222223` |
| `sum(flag)` | i | cube, dataset | `500 DATABASE_ERROR` | `7` | `7` |
| `avg(flag)` | i | cube, dataset | `500 DATABASE_ERROR` |
`0.7777777777777778` | `0.7777777777777778` |
| `min(flag)` / `max(flag)` | i | cube, dataset | `500 DATABASE_ERROR` |
`0` / `1` | `0` / `1` |
| `sum(frac)`, all-NULL group | n | cube | `null` | `0` | `0` |
| `sum(frac)`, all-NULL group | n | dataset | `0` (executor fill) | `0`
| `0` |
| `sum(flag)`, all-NULL group | n | cube | `500 DATABASE_ERROR` | `0` |
`0` |
| `avg(frac)`, all-NULL group | n | cube, dataset | `null` | `null` |
`null` |
| measure-scoped `sum(frac)`, no admitted row | i | cube | `null` | `0`
| `0` |
| measure-scoped `sum(frac)`, no admitted row | i | dataset | `0`
(executor fill) | `0` | `0` |
| measure-scoped `avg(frac)`, no admitted row | i | cube | `null` |
`null` | `null` |
| `count` control | n | cube, dataset | `3` | `3` | `3` |
| measure-scoped `count` control | i | cube, dataset | `0` | `0` | `0` |
**SQLite** (better-sqlite3): accumulation and the boolean answers
already agreed on every face (`0.30000000000000004`,
`0.15000000000000002`, `1.2222222222222223`, `7`, `0.7777777777777778`,
`0` / `1`). The fold is the policy that diverged there:
| measure | group | door | native before | native after | ObjectQL |
|:--|:--|:--|:--|:--|:--|
| `sum(frac)` / `sum(stars)` / `sum(flag)`, all-NULL group | n | cube |
`null` | `0` | `0` |
| `sum(frac)` / `sum(stars)` / `sum(flag)`, all-NULL group | n | dataset
| `0` (executor fill) | `0` | `0` |
| measure-scoped `sum(frac)`, no admitted row | i, n | cube | `null` |
`0` | `0` |
| measure-scoped `sum(frac)`, no admitted row | i, n | dataset | `0`
(executor fill) | `0` | `0` |
After the fix, the native and ObjectQL faces **differ in 0 of 144
cells** (2 drivers × 2 doors × 12 measures × 3 groups).
**MySQL is NOT MEASURED**: there is no MySQL server in this container.
The MySQL operand text is pinned offline: by `core`'s
`aggregate-answer.test.ts`, and by the `driver-sql` move proof for the
driver's own statements.
## The move proof
`driver-sql`'s aggregate statements were dumped at the base, before any
consumer changed. The dump covered `SqlDriver.aggregate()` for every
function (`count`, `count_distinct`, `sum`, `avg`, `min`, `max`, and
`count(*)`), aliased and unaliased, over 23 columns: every fractional,
integral and boolean type, the `float` / `integer` / `int` aliases,
multi-valued and untyped columns, and text / date / lookup / formula. It
ran on SQLite, PostgreSQL and MySQL, through both registration paths
(`registerObjectMetadata` and `registerExternalObject`), offline (knex
`toSQL()`).
The policies were then hoisted, `driver-sql` was switched to the
imports, and the same dump was run again:
- base dump: 1668 entries, 0 errors, md5
`8eee668372a28a7568f3eb1cc5a2bc9b`;
- after dump (at `bc8aa0cc2f`): 1668 entries, md5
`8eee668372a28a7568f3eb1cc5a2bc9b`. `cmp` printed nothing: the two dumps
are **byte-identical**.
`sql-driver.ts` and `aggregate-answer.ts` are unchanged between
`bc8aa0cc2f` and `61aab5013a`.
The committed move-proof pin,
`packages/drivers/driver-sql/src/sql-driver-21042-aggregate-policy-move.test.ts`,
holds the captured expressions for one column of each class, on each
dialect and through each registration path. It passed at the base
(`192fc0010b`: 54 / 54) and passes after (54 / 54).
## Pins (committed red first, then the fix)
| file | at the pins commit (`192fc0010b`, base code) | after |
|:--|:--|:--|
| `core` `aggregate-answer.test.ts` | 16 red (the exports did not exist)
| 22 / 22 |
| `service-analytics` `native-sql-aggregate-policies.test.ts` (each
measure on both faces at both doors, against the engine's arithmetic;
SQLite and live PostgreSQL cells) | SQLite: the cube-door folds red.
PostgreSQL: accumulation, boolean `500`, folds red | 49 / 49 |
| `service-analytics` `cube-measure-field-type-door.test.ts`, **the
lifted skip** | PostgreSQL native `max(boolean)` red (`500`) | 23 / 23 |
| `rest` `analytics-dataset-aggregate-policies-door.test.ts` (the route,
both strategies) | PostgreSQL: 7 red (accumulation and booleans). SQLite
green (that door already folded) | 19 / 19 |
| `driver-sql` move proof | 54 / 54 | 54 / 54 |
**The lifted skip:** `it.skipIf(cell.id === 'pg' && face === 'native')`
in `cube-measure-field-type-door.test.ts` (from PR #21128) is gone. Its
comment now says why the cell runs on every cell and face. The
PostgreSQL native `max_flag` cell answers `1`.
`native-sql-measure-number-presentation.test.ts` gets a comment-only
edit: its header said the native statement does not carry #20387's
accumulation, and it now points at the new pin.
## Ablations
There was one ablation per policy, each predicted in writing before it
ran. Each mutation was planted through `scripts/ablation-replace.mjs`,
which checks that the anchor hit and that the blob changed. Each
mutation was confirmed in the built `dist/`
(`ablation-dist-preflight.mjs`: marker present). Each restore ran by
absolute path (`git checkout HEAD`), and the file's blob was proven
equal to its `HEAD` blob with `git diff HEAD` empty. After each restore
the package was rebuilt, and the marker was proven absent from `dist/`
with the tree clean. Every prediction held exactly.
| ablation | mutation | predicted red | observed red |
|:--|:--|:--|:--|
| A1 accumulation | `core` `accumulatesInDouble`'s dialect gate never
admits PostgreSQL or MySQL | `core` 3; `driver-sql` move proof 24 (pg
and mysql × both fills × the six numeric / boolean columns);
`service-analytics` 10, PostgreSQL only (both doors × `sum` /
`avg(frac)`, `avg(stars)`, measure-scoped `sum` / `avg`); `rest` 3,
PostgreSQL only | the same 3 / 24 / 10 / 3; SQLite cells green;
`avg(flag)` green as predicted |
| A2 boolean cast | `core` `aggregandOperandSql` never casts | `core` 1;
move proof 4 (pg × both fills × `boolean` / `toggle`);
`service-analytics` 8, PostgreSQL only; the lifted cell, PostgreSQL
native and ObjectQL, 2; `rest` 4, PostgreSQL only | the same 1 / 4 / 8 /
2 / 4 |
| A3 fold | the native shaping point never folds | `service-analytics`
8, cube door only (SQLite and PostgreSQL × the three all-NULL `sum`s and
the measure-scoped `sum`); everything else green, the `rest`
dataset-door route included, because the executor fill folds there | the
same 8; `rest` 19 / 19 green |
A1 and A2 show one policy reaching both faces. Each one turned
`driver-sql`'s own statements red. Under A2 the ObjectQL face's
`max(boolean)` cell failed too, with the driver's refusal. Under A1 the
ObjectQL face answered the same exact decimal as the native face for the
plain measures: the failing assertion was the engine's number, while
native and ObjectQL still agreed.
A **reverse type check** also ran. Passing a dialect the new type
rejects (`'oracle'`) to `aggregandOperandSql` turned
`service-analytics`' typecheck red (`TS2345 ... not assignable to
parameter of type 'AggregandSqlDialect'`), which shows the rebuilt
`core` `.d.ts` was read. The file was restored byte-identical.
## Verification (at `ef1f9d8484`, after merging `origin/main` at
`cb45469e67`, which carries PR #21170 and PR #21173)
Everything below ran as one locked script, at `ef1f9d8484`, with each
exit code captured before any pipe. The live PostgreSQL 16.13 server ran
at `timezone = Asia/Shanghai`, and the `driver-sql` suite ran under
`TZ=America/New_York`, which are its own non-vacuity preconditions.
- **Refresh after the merge:** `pnpm turbo run build
--filter='!@objectstack/docs' --concurrency=1` exit 0, and `pnpm
--filter @objectstack/spec check:generated` exit 0.
- **Gates:** `node scripts/pm/dispatch-gates.mjs --commands --repo
objectstack-ai/objectstack` derived 67 commands from the real diff (10
paths). All 67 exited 0. Reconciliation with `--ran` and the recorded
exit codes printed: `Run reconciliation — 67 derived, 67 run, 0
NOT-MEASURED, 0 UNRUN.`
- **Typecheck:** `pnpm --filter … typecheck` exit 0 for
`@objectstack/core`, `@objectstack/driver-sql`,
`@objectstack/service-analytics` and `@objectstack/rest`.
- **Tests, every vitest project of every touched package** (`vitest run
--maxWorkers=2`, with `OS_TEST_POSTGRES_URL` set so the live cells ran):
| package / project | files | tests |
|:--|:--|:--|
| `core` local | 74 passed | 2120 passed |
| `core` repo | 3 passed | 48 passed |
| `driver-sql` | 216 passed, 3 skipped | 4300 passed, 96 skipped |
| `service-analytics` | 161 passed | 3765 passed |
| `rest` local | 256 passed | 5027 passed, 127 skipped |
| `rest` repo | 5 passed | 179 passed, 1 skipped |
- **The five pin files, run explicitly:** move proof 54 / 54, `core` 22
/ 22, policies 49 / 49 (24 SQLite + 24 PostgreSQL + the oracle),
field-type door 23 / 23, `rest` route 19 / 19 (9 SQLite + 9 PostgreSQL +
the oracle).
- **Lint, narrowed and proven:** `eslint --no-inline-config --format
json` over the 9 changed code files answered 9 results, 0 errors and 0
warnings. The population is read from eslint's own config:
`ESLint.isPathIgnored` answers `false` for each of the 9. The narrowing
excludes nothing that could move, because `eslint.config.mjs` never
enables type-aware linting (no `parserOptions.project`, no typed rules),
so this diff cannot change the verdict on an untouched file. The
repo-wide `pnpm lint` is CI's.
- **The `driver-sql` live preconditions:** the first gate run used a
private server at UTC, and the suite's four timezone non-vacuity cells
failed by design (`… start it with timezone=Asia/Shanghai`). With the
server at `Asia/Shanghai` and the process at `America/New_York` the
suite is green, as listed above.
- **`main` after the final merge:** `origin/main` moved 4 commits past
`cb45469e67` before this PR opened (#21149, #21188, #21195, #21192).
None of them touches `core`, `driver-sql`, `service-analytics` or the
analytics `rest` tests. The one `packages/spec` file in the analytics
area, `ui/dataset.zod.ts`, changes a comment only. They are not merged
here; CI runs on the merge ref.
## Acceptance notes
- **MySQL is NOT MEASURED** (no server in this container). The MySQL
operand text is pinned offline in `core` and in the `driver-sql` move
proof.
- **The live PostgreSQL cells are not run in CI.** No CI step sets
`OS_TEST_POSTGRES_URL` for `service-analytics` or `rest`. The cells
above ran against a private PostgreSQL 16.13 started for this run and
removed afterwards. In CI the SQLite cells run, and so do the offline
`core` / move-proof pins.
- **Phase 0's note on the dataset door, which is no divergence:** a
measure-scoped `avg` is **absent** from a row its supplementary query
reported no row for. That is `x_avg_frac` for groups `i` and `n`, on
both strategies and before and after. It is not `null`. The new service
pin holds this cell only to "both faces agree", not to a value.
Relatedly, a dataset-door selection made only of measure-scoped measures
reports only the groups their filter admits, so the pin asks each one
beside the base count.
- **Residual, as stated in the ruling:** a host that relays no field
declarations (`declaredValueShape`), or names no SQL dialect, gets no
column class or no policy. It keeps the native arithmetic it had, and a
PostgreSQL boolean `sum` there still answers `500`. The plugin's own
composition wires both.
- **How `driver-sql` reads the class:** it reads its own registries
rather than calling the predicate per column. `fractionalNumericFields`
is filled by the predicate. `booleanFields` and `numericFields` are
filled by the driver's coercion rules, whose populations equal the
predicate's `'boolean'` and `'fractional'` ∪ `'integral'` classes. The
move proof pins that equality per column class. Asking the predicate per
column through the driver's `valueShapeFields` would retire
`fractionalNumericFields`, but its declaration and shard-alias regions
are outside the ruled surface, so this PR does not do it.
- **The scan-order residual is unchanged.** On PostgreSQL and MySQL the
double sums are added without compensation (`AGGREGATE_ACCUMULATION`'s
docblock), so three or more fractions can still differ in the last place
from SQLite and the rows path. The pins use two addends.
- #21129 (the presenter for a relationship-path `min` / `max`) is not
addressed here.
---
_Generated by [Claude
Code](https://claude.ai/code/session_01XY5uCwTjZj7884yYtyur4H)_
---------
Co-authored-by: Claude <noreply@anthropic.com>
1 parent 862f12c commit 097ef80
10 files changed
Lines changed: 1309 additions & 139 deletions
File tree
- .changeset
- packages
- core/src/utils
- drivers/driver-sql/src
- rest/src
- services/service-analytics/src
- __tests__
- strategies
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
11 | 11 | | |
12 | 12 | | |
13 | 13 | | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
14 | 23 | | |
15 | 24 | | |
16 | 25 | | |
17 | | - | |
18 | | - | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
19 | 38 | | |
20 | 39 | | |
21 | 40 | | |
| |||
74 | 93 | | |
75 | 94 | | |
76 | 95 | | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
| 214 | + | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
0 commit comments