Skip to content

Commit e66da5c

Browse files
claude[bot]claude
andauthored
feat(service-analytics)!: refuse a deriving aggregate over a field type that cannot carry it, for every field type and not only the temporal class (#17559)
* feat(service-analytics): refuse a deriving aggregate over a field type that cannot carry it Claude-Session: https://claude.ai/code/session_01ToDPcx9AESFubJkDiFMtKW Co-authored-by: Claude <noreply@anthropic.com> * test(service-analytics): pin the non-temporal deriving-aggregate refusal and re-aim the boundary Claude-Session: https://claude.ai/code/session_01ToDPcx9AESFubJkDiFMtKW Co-authored-by: Claude <noreply@anthropic.com> * chore(changeset): declare the deriving-aggregate accept-set narrowing Claude-Session: https://claude.ai/code/session_01ToDPcx9AESFubJkDiFMtKW Co-authored-by: Claude <noreply@anthropic.com> * docs(changeset): state the FROM -> TO migration per refused shape Claude-Session: https://claude.ai/code/session_01ToDPcx9AESFubJkDiFMtKW Co-authored-by: Claude <noreply@anthropic.com> * docs(service-analytics): correct the third narrative that stated the gate's scope as temporal The formula-return-type suite's header explained why `min`/`max` over a `formula` field is still reachable by quoting the temporal-class early return this card removes. The conclusion is unchanged — `min`/`max` are outside the gate's reach either way — but the reason moved from the field class to the aggregate class. Claude-Session: https://claude.ai/code/session_01ToDPcx9AESFubJkDiFMtKW Co-authored-by: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent f721ef0 commit e66da5c

5 files changed

Lines changed: 572 additions & 82 deletions

File tree

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
---
2+
"@objectstack/service-analytics": minor
3+
---
4+
5+
feat(service-analytics)!: a dataset measure applying `sum` or `avg` to a field whose declared type cannot carry it is refused at compile time, for every field type and not only the temporal class (#16099)
6+
7+
<!-- adr-0087: not-required (already-registered dataset-measure-aggregate-field-type-refused) the hand-migration prescription for this exact narrowing is already registered under protocol major 18 by #16778 — store the quantity as a numeric field and aggregate that — and this change widens which pairs reach it without changing what an affected author must do. ⚠️ That entry's `acceptanceCriteria` is scoped to a `date`/`datetime`/`time` field and says a field of any other class is "neither refused nor certified by this leg", which is no longer true at HEAD; widening that sentence is a `packages/spec` edit this card is fenced out of and is reported to the `domain:spec` seat rather than done here. -->
8+
9+
**BREAKING** — an accept-set narrowing on a published authoring surface, continuing the
10+
one #16778 began. A dataset measure pairing `aggregate: 'sum'` with a `text` field (or
11+
`avg` with a `select`, `json`, `lookup`, `formula`, … field) used to compile and reach
12+
the backend; it is now refused by `compileDataset` with `DATASET_INVALID` / **400**
13+
before any query is built. Shipped as `minor` under the repo's launch-window convention
14+
for accept-set narrowings.
15+
16+
⛔ This changeset adds no rows to any table and restates none. The verdict is
17+
`AGGREGATE_FIELD_TYPE_COMPATIBILITY`'s — the one table `@objectstack/spec` declared in
18+
#16353 under the director ruling of decision batch #59 ("both legs, table in spec") —
19+
read through `isAggregateCompatibleWithFieldType`.
20+
21+
## What was wrong
22+
23+
#16778 landed the compile leg SCOPED to temporal source fields, leaving "every other
24+
non-temporal pair the table refuses" as a stated residual that had never been driven.
25+
Driven on this card, through the real service door:
26+
27+
```
28+
sum × text the table refuses the pair the compile leg does NOT throw SQL IS emitted
29+
sweep 6 aggregates × 49 field types = 294 pairs; 155 refused by the table;
30+
minus 6 temporal (#16778's) minus 42 `min`/`max` × the string classes;
31+
residual 107 — and 107 of 107 were ACCEPTED by the compile leg
32+
control avg × datetime / date / time → DATASET_INVALID / 400, no SQL emitted
33+
```
34+
35+
The control is what makes that a reading of the tree rather than of a blind harness: the
36+
same service, door and `sourceFieldMeta` hook sees the pairs #16778 enforces refused.
37+
38+
So `sum` over a `text` column reached whichever backend the object is bound to, and the
39+
answer was a property of the dialect rather than of the data — the shape Prime Directive
40+
#12 exists to remove, and the same shape #16778 closed for one field class.
41+
42+
## What it does now
43+
44+
- `compileDataset` judges a measure whose aggregate DERIVES a number (`sum` / `avg`)
45+
against the table for **every** declared field type, and refuses an unaccepted pair
46+
with `DATASET_INVALID` / **400** — naming the measure, the field, its declared type
47+
and the accepted set read off the table. Nothing reaches the driver.
48+
- `sum` × `percent` is refused at last: the row `analytics-service.ts` has called
49+
"incoherent" in a comment since before the table existed. `avg` × `percent` is still
50+
ACCEPTED by the same table, which is what makes it a row and not a class.
51+
- The refusal's closing prescription is now chosen by the source field's class: the
52+
temporal sentence #16778 measured is kept verbatim for temporal fields, and a
53+
non-numeric field is pointed at `count` / `count_distinct`, which accept every type
54+
because they read no arithmetic off a value.
55+
- Unchanged: `derived` is covered by construction (a dataset carrying a refused base
56+
measure never finishes compiling), and the three "cannot answer, do not block" tiers —
57+
no `sourceFieldMeta`, an unresolvable field, a `relationship.field` path.
58+
59+
## ⚠️ Scope: the DERIVING aggregates. `min` / `max` are still not judged here
60+
61+
`min` / `max` SELECT one of the stored values; `sum` / `avg` DERIVE a number. This is the
62+
line this package already draws — `measureResultType` branches on exactly that pair of
63+
aggregates — and the defect is about a derived number, so the deriving aggregates are its
64+
population.
65+
66+
The `min` / `max` rows stay with **#17513**, and that is measured rather than assumed.
67+
Enforcing the residual whole was tried on this card: with `min` / `max` × the string
68+
classes subtracted, **15** cases in `measure-result-type.test.ts` still went red, every
69+
one of them on `min` × `json` — a pair the table refuses, in no ruling's scope, driven
70+
end to end by the same shared fixture as the string rows. One dataset compiles every
71+
measure in that fixture, so one refused pair reds the whole section. ⇒ `min` / `max` is
72+
one question, and it is the table-amendment card's.
73+
74+
## Upgrading — FROM → TO
75+
76+
Nothing an author writes is removed or renamed: `DatasetMeasure.aggregate` and
77+
`DatasetMeasure.field` keep their spellings and their types. What narrows is which PAIRS of
78+
values are accepted. The one-line fix, per shape:
79+
80+
| FROM (compiled before, refused now) | TO |
81+
|---|---|
82+
| `{ aggregate: 'sum', field: <a text / select / lookup / user / autonumber field> }` | `{ aggregate: 'count_distinct', field: <the same field> }` — counting reads no arithmetic off the value |
83+
| `{ aggregate: 'sum' | 'avg', field: <a json / file / location / vector / composite field> }` | store the quantity you meant as its own numeric field and aggregate that |
84+
| `{ aggregate: 'sum', field: <a formula field> }` | aggregate the formula's numeric INPUT column; a `formula` is virtual in SQL storage, so no arithmetic aggregate can be lowered to it |
85+
| `{ aggregate: 'sum', field: <a percent field> }` | `{ aggregate: 'avg', field: <the same field> }` — a rate averages, it does not add |
86+
| `{ aggregate: 'sum' | 'avg', field: <a date / datetime / time field> }` | unchanged from #16778: use `min` / `max` for a real instant, or store a duration as a number and aggregate that |
87+
88+
`min` / `max` are **not** affected by this change at all, over any field type.
89+
90+
No shipped dataset in this repository declares a newly-refused pair — every one of the
91+
eleven shipped dataset measures resolves to `number`, `currency`, `summary` or `progress`.
92+
The refusal names the accepted set for the aggregate, read off the table.

‎packages/services/service-analytics/src/__tests__/aggregate-datetime-measure-refusal.test.ts‎

Lines changed: 56 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -41,22 +41,28 @@
4141
* so a row changed upstream changes these expectations with it rather than
4242
* leaving a second, drifting account of the contract.
4343
*
44-
* ## ⚠️ The compile leg lands SCOPED to temporal source fields, and says why
44+
* ## ⚠️ The compile leg lands SCOPED, and #16099 has since moved where the line sits
4545
*
46-
* The verdict is the table's; what is scoped is which FIELDS the gate judges.
47-
* Executing every row of the table today would refuse `min` / `max` over the
48-
* STRING classes — typed `'string'` by `measureResultType` (#15768) and pinned
49-
* end to end in `measure-result-type.test.ts` — which this platform answers on
50-
* purpose. #16785 has since **ruled C** on those rows: the TABLE is to be
51-
* amended to accept them, so enforcing them from here would pre-empt a ruling
52-
* that goes the other way. The BOOLEAN rows are settled and are not a collision
53-
* any more: #16685 was ruled A and #16750 added `boolean` / `toggle` to
54-
* `sum` / `avg` / `min` / `max` (maintainer ruling #11152 — booleans aggregate
55-
* as numbers on every backend), so the table ACCEPTS them and nothing refuses
56-
* them anywhere. The temporal rows carry no such counter-evidence, which is why
57-
* they are the ones executed here; the full-table leg stays with #16099. The
58-
* last suite in this file pins that boundary so it cannot widen by accident —
59-
* ⛔ without pinning any verdict the table itself is still under ruling for.
46+
* The verdict is the table's; what is scoped is which PAIRS the gate judges.
47+
* #16737 landed that scope as the TEMPORAL source-field class and said why:
48+
* executing every row would refuse `min` / `max` over the STRING classes —
49+
* typed `'string'` by `measureResultType` (#15768) and pinned end to end in
50+
* `measure-result-type.test.ts` — which this platform answers on purpose, and
51+
* which are ruled to be AMENDED into the table rather than executed (#17513).
52+
*
53+
* **#16099 has since re-cut that scope along the AGGREGATE instead**, and the
54+
* two boundary cases at the foot of this file are where the move is visible:
55+
* the gate now judges the DERIVING aggregates (`sum` / `avg`) over EVERY field
56+
* type, and does not judge `min` / `max` at all. So `sum` × `text` — which this
57+
* file pinned as compiling, and predicted in writing would go red when the gate
58+
* widened — is now refused, while `min` × `text` still compiles and keeps its
59+
* row's ruling intact. The BOOLEAN rows were never a collision in either
60+
* scope: #16685 was ruled A and #16750 added `boolean` / `toggle` to the
61+
* `sum` / `avg` / `min` / `max` rows (maintainer ruling #11152 — booleans
62+
* aggregate as numbers on every backend), so the table ACCEPTS them and nothing
63+
* refuses them anywhere. The non-temporal deriving population is pinned in
64+
* `aggregate-nontemporal-measure-refusal.test.ts`; this file keeps the temporal
65+
* population and the boundary.
6066
*
6167
* ## Dissolution verification — direction predicted BEFORE running
6268
*
@@ -481,16 +487,18 @@ describe('#16737 — the gate stands down rather than guessing', () => {
481487
// The scope boundary, pinned — so it cannot widen (or narrow) unnoticed
482488
// ─────────────────────────────────────────────────────────────────────────────
483489

484-
describe('#16737 — the compile leg is scoped to temporal source fields, on purpose', () => {
490+
describe('#16737 — the compile leg is SCOPED, on purpose; #16099 moved where the line sits', () => {
485491
it('a `min` over a TEXT field still compiles here — WHATEVER the table says about that pair', async () => {
486492
// ⚠️ Not an endorsement of the pair, and ⛔ deliberately NOT a pin on the
487-
// table's verdict for it. `min` × `text` is the row #16785 is ruled C on —
488-
// the table is to be AMENDED to accept it — so asserting today's `false`
489-
// here would make this file go red when that ruling lands, and would make a
490-
// test of this card the thing standing in the way of a decision this card
491-
// does not own. What this case owns is one fact, true on either side of
492-
// that amendment: a NON-TEMPORAL field is not judged by this gate, so the
493-
// measure compiles and reaches the driver.
493+
// table's verdict for it. `min` × `text` is the row ruled C — the table is
494+
// to be AMENDED to accept it, tracked as **#17513** — so asserting today's
495+
// `false` here would make this file go red when that ruling lands, and
496+
// would make a test of this card the thing standing in the way of a
497+
// decision this card does not own. What this case owns is one fact, true on
498+
// either side of that amendment: a `min` / `max` measure is not judged by
499+
// this gate, so the measure compiles and reaches the driver. #16099 widened
500+
// the gate to every field type, but only for the DERIVING aggregates, and
501+
// `min` / `max` stayed outside it precisely so this row keeps its ruling.
494502
const { svc, sqls } = makeService([{ status: 'open', first_note: 'Archive the backlog' }]);
495503
const result: any = await svc.queryDataset(
496504
dataset([{ name: 'first_note', aggregate: 'min', field: 'note' }]),
@@ -502,22 +510,36 @@ describe('#16737 — the compile leg is scoped to temporal source fields, on pur
502510
expect(sqls.length).toBe(1);
503511
});
504512

505-
it('a `sum` over a TEXT field also compiles here — the boundary, on a row no ruling is moving', async () => {
506-
// The refutable half of the boundary. #16785 C amends only `min` / `max`
507-
// for the string classes, so `sum` × `text` stays a pair the TABLE refuses
508-
// and this GATE does not act on — before that amendment and after it. If
509-
// the gate ever widened past the temporal class, this case goes red.
513+
it('⭐ a `sum` over a TEXT field is now REFUSED — the boundary #16099 moved, replacing this pin', async () => {
514+
// ⚠️ This case asserted the OPPOSITE until #16099, and the branch it pinned
515+
// is the one that card removed: it read "`sum` × `text` stays a pair the
516+
// TABLE refuses and this GATE does not act on", and predicted in writing
517+
// that "if the gate ever widened past the temporal class, this case goes
518+
// red". It did, and it did — driven, not assumed. So the pin is replaced
519+
// rather than deleted or relaxed: the same pair, through the same door,
520+
// asserting the answer the platform now gives. The fuller population lives
521+
// in `aggregate-nontemporal-measure-refusal.test.ts`; what this case keeps
522+
// owning is the BOUNDARY — this is the row that moved, beside the `min`
523+
// row above that did not.
510524
expect(isAggregateCompatibleWithFieldType('sum', 'text')).toBe(false);
511525
const { svc, sqls } = makeService([{ status: 'open', note_sum: 0 }]);
512-
const result: any = await svc.queryDataset(
513-
dataset([{ name: 'note_sum', aggregate: 'sum', field: 'note' }]),
514-
{ dimensions: ['status'], measures: ['note_sum'] },
526+
const err = await refusalOf(() =>
527+
svc.queryDataset(
528+
dataset([{ name: 'note_sum', aggregate: 'sum', field: 'note' }]),
529+
{ dimensions: ['status'], measures: ['note_sum'] },
530+
),
515531
);
516-
expect(result.rows.length).toBe(1);
517-
expect(sqls.length).toBe(1);
532+
expect(err.code).toBe('DATASET_INVALID');
533+
expect(err.status).toBe(400);
534+
expect(err.message).toContain('note_sum');
535+
expect(err.message).toContain('text');
536+
// Not vacuous in the other direction either: the refusal happens BEFORE any
537+
// statement is emitted, which is what makes it a rejected DECLARATION
538+
// rather than a query that happened to return nothing.
539+
expect(sqls.length).toBe(0);
518540
});
519541

520-
it('every temporal member IS judged — the scope is the class, not the one type the card named', async () => {
542+
it('every temporal member IS judged — not just the one type the card named', async () => {
521543
for (const [field, label] of [['submitted_at', 'datetime'], ['close_date', 'date'], ['shift_start', 'time']] as const) {
522544
const { svc } = makeService();
523545
const err = await refusalOf(() =>

0 commit comments

Comments
 (0)