Skip to content

Commit d23f770

Browse files
committed
fix(spec): fail-closed shape guard on isAggregateCompatibleWithFieldType; correct the published grounds for the boolean and time rows (#16353)
Contract-review patch round. The predicate now refuses any non-string input (a property-key lookup alone coerced ['count'] / { toString } to a member spelling); pinned. The TSDoc and changeset no longer claim booleans are the divergence class - #11152 has every backend answer them as numbers - and record that row, plus the min/max refusal over the string classes (#15768 types them as a supported 'string' result), as overrides of existing opinions referred to the maintainer. The time justification names SQLite's canonical TEXT form (#3994). No row changed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F8SRGcf2eKTK7RRpWCGxwf
1 parent 3da5832 commit d23f770

3 files changed

Lines changed: 84 additions & 17 deletions

File tree

.changeset/aggregate-field-type-compatibility.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,9 @@ A dataset measure pairs an `aggregate` with a `field`, and nothing between autho
1414
| `min`, `max` | the numeric class plus `date`, `datetime`, `time` — both return a value of the field's own type |
1515
| every other pair | refused |
1616

17-
The ruling (director, decision batch #59, 2026-09-06) named its buckets by category; the table resolves them against the real `FieldType` membership through the `field-value.zod` semantic classes: "numeric" is `NUMERIC_VALUE_TYPES` (`integer` is a driver-internal column alias, not a `FieldType` — the integer-valued authorable members are `rating` / `slider` / `progress`); "temporal" is the three temporal classes, `time` included because the driver stores it as a native TIME column and `AnalyticsResult.fields[].type` already describes `min` / `max` over it as temporal. `formula` is refused for arithmetic aggregates whatever its declared `returnType`: it is virtual in SQL storage, no column exists to aggregate.
17+
The ruling (director, decision batch #59, 2026-09-06) named its buckets by category; the table resolves them against the real `FieldType` membership through the `field-value.zod` semantic classes: "numeric" is `NUMERIC_VALUE_TYPES` (`integer` is a driver-internal column alias, not a `FieldType` — the integer-valued authorable members are `rating` / `slider` / `progress`); "temporal" is the three temporal classes, `time` included because its stored form is a dialect question exactly like `date` / `datetime` (native TIME on Postgres and MySQL, canonical `HH:MM:SS[.fff]` TEXT on SQLite), the canonical form orders chronologically on every dialect, and `AnalyticsResult.fields[].type` already describes `min` / `max` over it as temporal (#15768). `formula` is refused for arithmetic aggregates whatever its declared `returnType`: it is virtual in SQL storage, no column exists to aggregate.
18+
19+
Two refused rows override existing opinions and are recorded as such, not presented as agreement. **Booleans** are refused for `sum` / `avg` / `min` / `max` by the ruling's "every other pair: refused", while maintainer ruling #11152 already has every backend answer them as numbers (`sum(flag)=3`, `avg(flag)=0.5`, `min(flag)=0`, `max(flag)=1`, pinned in the spec's `AGGREGATION_CASES`; `driver-sql` casts the aggregand on Postgres to make it hold). That refusal is therefore not grounded in backend divergence; whether booleans belong in those rows is a collision between two rulings and is referred to the maintainer as its own decision — the row ships exactly as batch #59 stated it. **The string classes** are refused for `min` / `max` here, while `service-analytics` (#15768) already types `min` / `max` over them as a supported `'string'` result; the refusal is defensible (string order is collation-dependent) but it overrides that opinion.
1820

1921
**The narrowing, stated plainly.** Every pair outside the table — `avg` × `datetime`, `sum` × `boolean`, `min` × `text`, `sum` × `percent`, and so on — is an authoring shape `DatasetMeasureSchema` accepts today and will be REFUSED once the two consumer legs land: the compile-time refusal in the dataset compiler (#16099) and the authoring-time lint rule (its devx sub-card). A measure whose pair is refused is fixed by changing the aggregate to one the field's type supports (`min` / `max` for a temporal field; `avg` for a `percent`; `count` for anything), never by widening the table.
2022

packages/spec/src/data/aggregate-field-type-compatibility.test.ts

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ describe('isAggregateCompatibleWithFieldType — the pairs the card is about', (
107107
}
108108
});
109109

110-
it('refuses arithmetic over the divergence class (booleans) and over the computed / text / structured types', () => {
110+
it('refuses arithmetic over booleans (row as ruled; membership referred, see module TSDoc) and over the computed / text / structured types', () => {
111111
for (const fn of ['sum', 'avg', 'min', 'max'] as const) {
112112
for (const t of ['boolean', 'toggle', 'formula', 'autonumber', 'text', 'select', 'lookup', 'json', 'vector', 'file']) {
113113
expect(isAggregateCompatibleWithFieldType(fn, t)).toBe(false);
@@ -141,4 +141,37 @@ describe('isAggregateCompatibleWithFieldType — the pairs the card is about', (
141141
expect(isAggregateCompatibleWithFieldType('toString', 'text')).toBe(false);
142142
expect(isAggregateCompatibleWithFieldType('', '')).toBe(false);
143143
});
144+
145+
it('fails closed on SHAPE — a non-string that would coerce to a member spelling is refused, not looked up', () => {
146+
// `hasOwnProperty.call` applies ToPropertyKey, so without the typeof guard
147+
// `['count']` reads as 'count' and an object with a toString reads as
148+
// 'sum'. A refusal gate must not be talked past by coercion.
149+
const loose = isAggregateCompatibleWithFieldType as unknown as (a: unknown, f: unknown) => boolean;
150+
expect(loose(['count'], 'number')).toBe(false);
151+
expect(loose({ toString: () => 'sum' }, 'currency')).toBe(false);
152+
expect(loose('sum', ['currency'])).toBe(false);
153+
expect(loose('min', { toString: () => 'date' })).toBe(false);
154+
expect(loose(undefined, 'number')).toBe(false);
155+
expect(loose(null, 'number')).toBe(false);
156+
expect(loose('count', undefined)).toBe(false);
157+
expect(loose('count', null)).toBe(false);
158+
expect(loose(1, 'number')).toBe(false);
159+
expect(loose('count', 1)).toBe(false);
160+
expect(loose(Symbol('count'), 'number')).toBe(false);
161+
});
162+
163+
it('records the two overrides of existing opinions without changing the rows: booleans and the string classes', () => {
164+
// Booleans: refused here by the ruling's default; #11152 / AGGREGATION_CASES
165+
// answer them as numbers on every face. Row kept as ruled, question referred.
166+
for (const fn of ['sum', 'avg', 'min', 'max']) {
167+
expect(isAggregateCompatibleWithFieldType(fn, 'boolean')).toBe(false);
168+
expect(isAggregateCompatibleWithFieldType(fn, 'toggle')).toBe(false);
169+
}
170+
// String classes: min/max refused here; measureResultType (#15768) types
171+
// min/max over them as a supported 'string' result. Override recorded.
172+
for (const t of ['text', 'select', 'lookup', 'autonumber']) {
173+
expect(isAggregateCompatibleWithFieldType('min', t)).toBe(false);
174+
expect(isAggregateCompatibleWithFieldType('max', t)).toBe(false);
175+
}
176+
});
144177
});

packages/spec/src/data/aggregate-field-type-compatibility.ts

Lines changed: 47 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -46,18 +46,45 @@
4646
* as REAL columns beside `number`. `summary` is a roll-up persisted as a
4747
* numeric column.
4848
* - **temporal** = `CALENDAR_DATE_TYPES` ∪ `INSTANT_TYPES` ∪ `CLOCK_TIME_TYPES`:
49-
* `date`, `datetime`, `time`. `time` is a native TIME column on every SQL
50-
* dialect the driver emits DDL for, and `AnalyticsResult.fields[].type`
51-
* already describes `min` / `max` over it as temporal (#15768), so it sits
52-
* beside the two members the ruling named.
49+
* `date`, `datetime`, `time`. The ruling named the first two; `time` is the
50+
* third temporal class and takes the same treatment: its stored form is a
51+
* dialect question exactly like the other two (native TIME on Postgres and
52+
* MySQL `TIME(3)`, canonical `HH:MM:SS[.fff]` TEXT on SQLite — #3994), the
53+
* canonical form orders chronologically on every one of them, and
54+
* `AnalyticsResult.fields[].type` already describes `min` / `max` over it as
55+
* temporal (#15768, `TEMPORAL_SOURCE_FIELD_TYPES`). So it sits beside the
56+
* two members the ruling named.
5357
* - **everything else** — the text family, booleans, option types, references,
5458
* files, structured JSON, `vector`, and the computed `formula` /
55-
* `autonumber` — is refused for `sum` / `avg` / `min` / `max`. `formula`
56-
* carries a declared `returnType`, but it is VIRTUAL in SQL storage (no
57-
* column is emitted), so no arithmetic aggregate can be lowered to it
58-
* whatever that type says; `autonumber` is a formatted string. Booleans are
59-
* the divergence class exactly: one dialect sums 0/1, another has no
60-
* `sum(boolean)` at all.
59+
* `autonumber` — is refused for `sum` / `avg` / `min` / `max`, the ruling's
60+
* "every other pair: refused". `formula` carries a declared `returnType`,
61+
* but it is VIRTUAL in SQL storage (no column is emitted), so no arithmetic
62+
* aggregate can be lowered to it whatever that type says; `autonumber` is a
63+
* formatted string.
64+
*
65+
* Two rows the ruling's default covers are recorded here as OVERRIDES of
66+
* existing opinions, not as settled ground — the row stands as ruled, the
67+
* text says only what this tree can defend:
68+
*
69+
* - **Booleans** (`boolean`, `toggle`) are refused for the four arithmetic /
70+
* order aggregates by the ruling's default, yet the runtime already ANSWERS
71+
* them: maintainer ruling #11152 pins that booleans aggregate as numbers on
72+
* every face with no per-aggregate exception (`AGGREGATION_CASES` in
73+
* `aggregation-conformance.ts`: `sum(flag)=3`, `avg(flag)=0.5`,
74+
* `min(flag)=0`, `max(flag)=1`, six backends), and `driver-sql` casts a
75+
* boolean aggregand to `int` on Postgres to make that hold (#11635). So the
76+
* refusal is NOT grounded in backend divergence — the backends agree. Whether
77+
* booleans belong in these rows is a collision between two rulings (batch
78+
* #59 and #11152) and is referred to the maintainer as its own decision; the
79+
* row is left exactly as batch #59 stated it until that decision lands.
80+
* - **The string classes** (`STRING_VALUE_TYPES`, `SINGLE_OPTION_TYPES`,
81+
* `REFERENCE_VALUE_TYPES`, `autonumber`) are refused for `min` / `max` here,
82+
* while `service-analytics`' `measureResultType` (#15768,
83+
* `STRING_SOURCE_FIELD_TYPES`) already types `min` / `max` over them as a
84+
* supported `'string'` result. The refusal is defensible — the ORDER of
85+
* strings is collation-dependent, so two backends can return two different
86+
* "smallest" values — but it overrides that existing opinion, and is
87+
* recorded as such rather than presented as agreement.
6188
*
6289
* ## Relation to `isIncoherentAggregate`
6390
*
@@ -126,13 +153,18 @@ export const AGGREGATE_FIELD_TYPE_COMPATIBILITY: Readonly<Record<AggregationFunc
126153
* both consumer legs call, so one pair cannot be accepted at authoring and
127154
* refused at compile time.
128155
*
129-
* Fail-closed on vocabulary: a value outside `AggregationFunction` or outside
130-
* `FieldType` answers `false`. The parameters are typed as `string` because
131-
* the lint leg judges metadata BEFORE it is parsed; that is a convenience of
132-
* the signature, not a tolerance — off-vocabulary input is refused, never
133-
* mapped.
156+
* Fail-closed on vocabulary AND on shape: a value outside `AggregationFunction`
157+
* or outside `FieldType` answers `false`, and so does anything that is not a
158+
* string at all. The second half is load-bearing for a refusal gate: the lint
159+
* leg judges metadata BEFORE it is parsed, so the value it hands in may be an
160+
* array or an object, and a property-key lookup alone would coerce
161+
* `['count']` or `{ toString: () => 'sum' }` to a member spelling and let the
162+
* pair through. The `string` parameter types are a convenience of the
163+
* signature, not a tolerance — off-vocabulary or off-shape input is refused,
164+
* never mapped.
134165
*/
135166
export function isAggregateCompatibleWithFieldType(aggregate: string, fieldType: string): boolean {
167+
if (typeof aggregate !== 'string' || typeof fieldType !== 'string') return false;
136168
if (!Object.prototype.hasOwnProperty.call(AGGREGATE_FIELD_TYPE_COMPATIBILITY, aggregate)) return false;
137169
const row: readonly string[] = AGGREGATE_FIELD_TYPE_COMPATIBILITY[aggregate as AggregationFunction];
138170
return row.includes(fieldType);

0 commit comments

Comments
 (0)