Skip to content

Commit 357f499

Browse files
Trumpclaude
andauthored
feat(service-analytics)!: refuse an aggregate a datetime measure's field type cannot carry, and reconcile the storage-form annotations to one measured statement (#16778)
* feat(service-analytics)!: refuse an aggregate a measure's field type cannot carry WIP — compile-leg refusal + the four reconciled datetime-storage annotations. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37 * wip: changeset + adr-0087 ledger entry * fix(service-analytics): scope the compile-leg refusal to temporal source fields The full table refuses `min`/`max` over the string classes and the boolean rows, both of which this platform answers on purpose and pins with tests (#15768, maintainer ruling #11152). Executing those is a product judgement that belongs to #16099; the temporal rows carry no such collision and are the ones this card is about. Re-points the two `measure-result-type.test.ts` fixture measures that aggregated a datetime column, and corrects the module header that recorded the missing refusal as an open finding. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37 * docs(changeset,spec): scope the breaking declaration to the temporal class it actually withdraws The compile leg was scoped to TEMPORAL source fields in 80ec9f2, but the changeset and the ADR-0087 ledger entry still described the pre-scoping full-table gate. A breaking-change record that overstates what changed tells every consumer reading the release notes that behaviour was withdrawn which was not. F1 — the breaking declaration: - changeset FROM/TO: the two `percent` rows dropped (`sum` x `percent` is a table row this leg does not execute; `avg` x `percent` was never a migration at all), and the surviving `avg` row widened to name all three temporal members it does refuse. - changeset: a new scope section states the temporal scope outright, and that the string rows sit under #16785 (ruled C - the table is to be AMENDED to accept them), the boolean rows were settled as ACCEPT by #16685 / #16750, and `sum` x `percent` is not executed here. - ledger `surface`: scoped to the temporal class; "sum over a percent" removed. - ledger `replacement`: the "`avg` for a `percent`" prescription dropped with the surface rows it belonged to. - ledger `acceptanceCriteria`: qualified to a measure over a `date` / `datetime` / `time` field, and says outright that a field of any other class is neither refused nor certified by this leg. - `registry.ts` REGENERATED with `pnpm --filter @objectstack/spec gen:migration-registry`, never hand-edited; two consecutive runs are byte-identical and the diff is confined to this entry's block. - also corrected: the changeset said "four contradictory annotations"; the sweep reconciled seven source sites plus two test narratives. F2 — the boolean collision is settled, so stop narrating it as live. #16685 was ruled A and #16750 added `boolean` / `toggle` to the four arithmetic / order rows, so the table ACCEPTS them. Reworded in `dataset-compiler.ts`'s scope docblock, the refusal suite's module header, and `measure-result-type.ts` (whose boolean paragraph still referred a missing refusal onward). All three now also record #16785 C for the string rows. F3 — the ledger `reason` presented both dialect halves as measured alike. The SQLite half is pinned by a live `sql.js` suite; the Postgres 42883 half was measured in-session and is pinned by nothing. Said so where it is stated. F4 — the scope-boundary test asserted `isAggregateCompatibleWithFieldType( 'min', 'text') === false`, a verdict #16785 C is about to amend. Dropped: the case now pins only what this PR owns - a non-temporal field is not judged, so the measure compiles and SQL is emitted. Refutability is carried by a second case on `sum` x `text`, a row no ruling is moving, plus a non-vacuity assertion that SQL reached the driver in both. F5 — the changeset now names the two uncovered faces: `/analytics/query` and any `compileDataset` caller wiring no `declaredFieldType` probe. Refs #16737. Review: PR #16778 contract review, comment 5580295870. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 0e7ef84 commit 357f499

13 files changed

Lines changed: 1173 additions & 70 deletions
Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,117 @@
1+
---
2+
"@objectstack/service-analytics": minor
3+
"@objectstack/spec": minor
4+
---
5+
6+
feat(service-analytics)!: a dataset measure whose `aggregate` its `field`'s declared type cannot carry is refused at compile time with `400 DATASET_INVALID` (#16737, compile leg of #16099)
7+
8+
<!-- adr-0087: registered dataset-measure-aggregate-field-type-refused -->
9+
10+
**BREAKING** — an accept-set narrowing on a published authoring surface. A dataset
11+
measure pairing `aggregate: 'avg'` with a `Field.datetime` used to compile to
12+
`AVG(col)` and reach the backend; it is now refused by `compileDataset` before any
13+
query is built. Shipped as `minor` under the repo's launch-window convention for
14+
accept-set narrowings; the hand-migration prescription is registered under protocol
15+
major 18 as `dataset-measure-aggregate-field-type-refused`.
16+
17+
The pair is judged against `AGGREGATE_FIELD_TYPE_COMPATIBILITY` — the one table
18+
`@objectstack/spec` declared in #16353 under the director ruling of decision batch
19+
#59 (2026-09-06, "both legs, table in spec"). ⛔ This changeset adds no rows and
20+
restates none: the refusal reads the shipped predicate, so the contract has exactly
21+
one statement.
22+
23+
## What was wrong
24+
25+
The answer to `AVG` over a temporal column was decided by the SQL dialect rather
26+
than by the data. Both halves measured on this card:
27+
28+
```
29+
-- SQLite (better-sqlite3), the canonical UTC-text storage form (#3912)
30+
select typeof(submitted_at), submitted_at from clm_contract limit 1;
31+
text|2026-05-19T00:00:00.000Z
32+
select avg(submitted_at) from clm_contract;
33+
2025.5 <- text->numeric coercion: the average YEAR
34+
35+
-- PostgreSQL 16.13
36+
select avg(submitted_at) from t;
37+
ERROR: function avg(timestamp with time zone) does not exist -- SQLSTATE 42883
38+
```
39+
40+
The silent half is the dangerous one, and SQLite is the default dev datasource:
41+
`derived: { op: 'difference', of: [avg_a, avg_b] }` over two such averages returned
42+
`-0.85` and rendered on a tile labelled "average cycle time delta" — a number
43+
indistinguishable from a correct one. Nothing refused it at any layer: not the
44+
schema, not `os validate` / `os lint`, not the analytics service, not the renderer.
45+
46+
## What it does now
47+
48+
- `compileDataset` refuses an incompatible `aggregate` × `field` pair with
49+
`DATASET_INVALID` / **400**, naming the measure, the field, its declared type and
50+
the accepted set (read off the table, never restated). Nothing reaches the driver.
51+
- It reads the declared type from the `sourceFieldMeta` a host already wires, via a
52+
new optional `DatasetCompileOptions.declaredFieldType` probe.
53+
- **`derived` is covered by construction.** A derived measure's `of` operands are
54+
base measures of the same dataset, so a dataset carrying a refused base measure
55+
never finishes compiling and no `derived` op can be handed its output — including
56+
when the selection names only the derived measure.
57+
- Tiered "cannot answer, do not block" like every sibling probe: no
58+
`sourceFieldMeta`, an unresolvable field, or a `relationship.field` path (whose
59+
column lives on a joined object) leaves the pair unjudged.
60+
61+
## ⚠️ Scope: the compile leg executes the TEMPORAL rows only
62+
63+
The gate judges only a measure whose field is declared `date` / `datetime` /
64+
`time`; a field of any other class is never handed to the predicate. The
65+
verdict for the pairs it does judge is the table's — no row is restated — but
66+
which FIELDS are judged is narrower than the table, on purpose:
67+
68+
- **String rows** (`min` / `max` over `text`, `select`, `lookup`,
69+
`autonumber`, …) are **not enforced here**. They are under #16785, **ruled
70+
C**: the table itself is to be amended to accept them, because
71+
`measureResultType` (#15768) already types those results as `'string'` and
72+
pins them end to end. Enforcing them from this card would pre-empt that
73+
ruling.
74+
- **Boolean rows** are not a refusal at all any more: #16685 was ruled A and
75+
#16750 added `boolean` / `toggle` to `sum` / `avg` / `min` / `max`, so the
76+
table ACCEPTS them and this gate never judged them.
77+
- The table's `sum` × `percent` row is likewise **not** executed by this leg;
78+
`sum` over a `percent` compiles exactly as it did before.
79+
80+
⇒ The only pairs whose behaviour changes in this release are `avg` / `sum`
81+
over a `date` / `datetime` / `time` field. The full-table leg remains #16099's.
82+
83+
## FROM → TO
84+
85+
| you wrote | write instead |
86+
|:--|:--|
87+
| `{ aggregate: 'avg', field: <a date/datetime/time field> }` | `{ aggregate: 'min' \| 'max', field: <same> }` — a real instant of the field's own type |
88+
| `{ aggregate: 'sum', field: <a date/datetime/time field> }` | store the duration as a number (a computed "days open" field) and `sum`/`avg` that |
89+
| `derived: { op: 'difference', of: ['avg_a', 'avg_b'] }` over temporal averages | fix the two operand measures; the `derived` spec itself is unchanged |
90+
91+
⭐ A duration is not recoverable from an aggregate over instants on any backend.
92+
Where an "average cycle time" is wanted, the cycle length has to exist as a number
93+
before it can be averaged.
94+
95+
## What is deliberately untouched
96+
97+
`date` / `datetime` used as a **dimension** — grouping, bucketing, date-range
98+
filtering — is unchanged; this is about aggregation only. `avg` over a genuine
99+
numeric measure, `min` / `max` over a temporal one, and `count` / `count_distinct`
100+
over anything all behave exactly as before.
101+
102+
⚠️ **Two faces stay uncovered, deliberately.** The refusal lives in
103+
`compileDataset` and reads a `declaredFieldType` probe, so it applies only where
104+
a host wires one: `/analytics/query` — the non-dataset face, whose measures a
105+
Cube infers rather than an author declaring them — is NOT covered, and neither
106+
is any other `compileDataset` caller that passes no probe (those stand down
107+
unjudged rather than guessing). Closing those is #16099's, not this card's.
108+
109+
Alongside the refusal, `service-analytics`' contradictory annotations about what a
110+
SQLite `Field.datetime` column physically holds are reconciled to one statement —
111+
**seven** source sites plus two test narratives, not the four the card quoted. Some
112+
said the column holds an INTEGER epoch and ISO TEXT at once; one said flatly that it
113+
IS an INTEGER epoch. Neither is current: since #3912 the column has ONE
114+
storage form, canonical UTC text, with the epoch surviving only in a database not
115+
yet converged by `backfillCanonicalDatetimes`. The fact is now stated once, on
116+
`AnalyticsServiceConfig.coerceTemporalFilterValue`, and the other sites link to it.
117+
No behaviour changes from that half.

0 commit comments

Comments
 (0)