Skip to content

Commit 1690b79

Browse files
committed
fix(objectql)!: having evaluates addDays only between two temporal columns of one class (#20127)
`FieldReferenceSchema.addDays` declares that the offset applies between two temporal columns of the same class (date/date, datetime/datetime), read from an integer or a numeric column, and `driver-sql` refuses every other pair on `where`. `having` resolved every pair through the in-memory evaluator, which reads a number as epoch milliseconds, because an aggregated row declared no class for its columns. `aggregatedRowColumnClasses` now reads each column's class statically: a groupBy projection takes its field's declared type (a `day` bucket is a date, a coarser bucket a text label), count / count_distinct / sum / avg are numeric, and min / max take the type of the field they read. The engine hands it, with the object's field map, to `assertHavingIsEvaluable`, which refuses a mismatched pair in driver-sql's own sentences, once per query. A class the declaration cannot tell is not judged. Claude-Session: https://claude.ai/code/session_01Bvd69VPa6puiNzzPUroDBx Co-authored-by: Claude <noreply@anthropic.com>
1 parent 71e3ae2 commit 1690b79

4 files changed

Lines changed: 346 additions & 4 deletions

File tree

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
---
2+
"@objectstack/objectql": minor
3+
---
4+
5+
fix(objectql)!: in `engine.aggregate({ having })`, a `{ $field, addDays }` reference is evaluated only between two temporal columns of one class, with a numeric offset column, as `FieldReferenceSchema.addDays` declares, instead of answering by epoch-millisecond coercion (#20127)
6+
7+
Clause-②: no (narrowing)
8+
9+
<!-- adr-0087: not-required (no-migration-prescription) this change adds no transition to migrate. It enforces on `having` the rule `FieldReferenceSchema.addDays` already declares ("between two temporal columns of the same class (date/date, datetime/datetime)", the offset a whole number or a numeric column), which `driver-sql` already enforces on `where`. A refused pair never had the meaning the declaration gives `addDays`: on a number, a text or a mixed pair, it was answered by the in-memory evaluator's coercion. There is no accepted spelling it can be mechanically rewritten to: whether the author meant two date columns, no offset, or a different column is an authoring decision. `having` is a request-only key: no metadata type stores it, so there is no stored document for `objectstack migrate meta` to rewrite. The table below records the answer each pair had and has; it prescribes no rewrite. -->
10+
11+
**BREAKING**: this narrows what `having` accepts on `engine.aggregate`, and on the REST aggregate query (`POST /data/:object/query`) that forwards it there. A `{ $field }` reference that carries `addDays` in one of the six scalar comparisons is now refused with `INVALID_FILTER` / 400 unless the column it filters and the column it references are both `date` or both `datetime`, and an `addDays` offset read from a column reads a numeric one. The refusal is raised once per query, before any driver is asked for a row, on both the native `driver.aggregate()` path and the in-memory fallback, whether or not any group exists. It ships as `minor` under the launch-window convention for accept-set narrowings.
12+
13+
An aggregated row has no declared field types, so each column's class is now read off the query and the object's declaration, before any row exists:
14+
15+
- a groupBy projection takes its field's declared type. A `day` date bucket is a `date`, because its label is `YYYY-MM-DD` on every face. A `week`, `month`, `quarter` or `year` bucket is a text label;
16+
- `count`, `count_distinct`, `sum` and `avg` are numeric;
17+
- `min` and `max` take the type of the field they read.
18+
19+
A column whose class the declaration cannot tell is not judged: an object with no field map, a field it does not declare, or a `formula` field.
20+
21+
`having` resolved every pair through `@objectstack/formula`'s evaluator, which reads a number as epoch milliseconds, while `driver-sql` refuses the same pair on `where`. The refusal reuses `driver-sql`'s sentence for that pair, with "is stored as" read as "is", because an aggregated column is computed rather than stored. Measured on the base through `engine.aggregate` on `driver-memory` and `driver-sql`, both paths, and through `POST /data/:object/query` on both, over three groups with a `sum` alias `total`, a `max` of a number `max_cap`, `max` / `min` of two `date` fields, `max` / `min` of two `datetime` fields and a `count` `n`:
22+
23+
| `having` | before | now |
24+
|:--|:--|:--|
25+
| `{ total: { $gt: { $field: 'max_cap', addDays: 1 } } }` (two numeric columns) | no group, no error | refused: "addDays adds whole days to a date or datetime column, and "max_cap" is numeric — an offset has no meaning on it." |
26+
| `{ n: { $gte: { $field: 'n', addDays: 0 } } }` (a count against itself) | every group | refused, in the same words |
27+
| a `date` column against a numeric column, a numeric column against a `date` one, or the `customer_id` groupBy text column against a `date` one | no group | refused, naming both columns and their classes: "… and a cross-class comparison answers differently in SQL (storage-class ordering) than in memory (JS coercion) — compare same-class columns." |
28+
| a `date` column against a `datetime` one | one group | refused, in the cross-class words |
29+
| a `datetime` column against a `date` one | two groups | refused, in the cross-class words |
30+
| a `date` pair whose `addDays` reads a text column or a `date` column | no group | refused: "the addDays offset … is not a numeric column, and a day offset must be a number of days." |
31+
32+
Who is affected: `having` is a request-only key (`QuerySchema.having`, `EngineAggregateOptions.having`), and no metadata type stores it. No `having` in this repository's docs and published skills carries a `{ $field }` reference. Callers of `engine.aggregate` and of the REST aggregate query in a deployment were NOT measured.
33+
34+
Not changed, measured identical before and after on both paths: a `date` / `date` pair and a `datetime` / `datetime` pair, with a positive or negative whole-day literal or with an offset read from a numeric column (`max` of a number, or a `count`); a `day` date bucket against a `date` column; and any `{ $field }` comparison WITHOUT `addDays`, including a numeric pair and a numeric column against a `date` one. A per-aggregation `filter` (`aggregations[i].filter`) is not judged by this rule: it reads the object's raw columns, and this change classifies only the aggregated row's.

‎packages/objectql/src/engine-aggregate-having-comparand-shape.test.ts‎

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -180,6 +180,10 @@ async function makeEngine(rows: ReadonlyArray<Record<string, unknown>>, native:
180180
placed_on: { type: 'date' },
181181
due_on: { type: 'date' },
182182
grace: { type: 'number' },
183+
// [#20127] Declared types are what an aggregated column's class is read
184+
// from, so the `addDays` pairs need a datetime class beside the date one.
185+
opened_at: { type: 'datetime' },
186+
closed_at: { type: 'datetime' },
183187
},
184188
} as any);
185189
return { engine, calls };
@@ -750,3 +754,113 @@ describe('[#20123] having — a key naming no column of the aggregated row is re
750754
});
751755
}
752756
});
757+
758+
// ───────────────────────────────────────────────────────────────────────────
759+
// [#20127] `addDays` between two temporal columns of one class
760+
// ───────────────────────────────────────────────────────────────────────────
761+
762+
describe('[#20127] having — a { $field, addDays } pair is judged by each aggregated column\'s class, whatever the rows', () => {
763+
const OPENED = ['2026-01-01T10:00:00.000Z', '2026-01-02T10:00:00.000Z', '2026-02-01T10:00:00.000Z', '2026-02-05T10:00:00.000Z', '2026-02-06T10:00:00.000Z', '2026-03-01T10:00:00.000Z'];
764+
const CLOSED = ['2026-01-03T10:00:00.000Z', '2026-01-02T12:00:00.000Z', '2026-02-10T10:00:00.000Z', '2026-02-05T11:00:00.000Z', '2026-02-06T10:00:00.000Z', '2026-03-01T10:00:00.000Z'];
765+
const DT_ROWS = [
766+
{ customer_id: 'c1', amount: 100, cap: 50, placed_on: '2026-01-10', due_on: '2026-01-05', grace: 3 },
767+
{ customer_id: 'c1', amount: 400, cap: 10, placed_on: '2026-01-02', due_on: '2026-01-20', grace: 10 },
768+
{ customer_id: 'c2', amount: 900, cap: 5000, placed_on: '2026-03-01', due_on: '2026-01-01', grace: 1 },
769+
{ customer_id: 'c2', amount: 300, cap: 1, placed_on: '2026-02-01', due_on: '2026-02-01', grace: 1 },
770+
{ customer_id: 'c2', amount: 50, cap: 2, placed_on: '2026-01-15', due_on: '2026-03-01', grace: 1 },
771+
{ customer_id: 'c3', amount: 20, cap: 20, placed_on: '2026-02-01', due_on: '2026-01-31', grace: 0 },
772+
].map((row, i) => ({ ...row, opened_at: OPENED[i], closed_at: CLOSED[i] }));
773+
// Grouped: total max_cap last_placed first_due max_grace first_opened last_closed
774+
// c1 500 50 2026-01-10 2026-01-05 10 2026-01-01T10:00Z 2026-01-03T10:00Z
775+
// c2 1250 5000 2026-03-01 2026-01-01 1 2026-02-01T10:00Z 2026-02-10T10:00Z
776+
// c3 20 20 2026-02-01 2026-01-31 0 2026-03-01T10:00Z 2026-03-01T10:00Z
777+
const DT_QUERY: EngineAggregateOptions = {
778+
groupBy: ['customer_id'],
779+
aggregations: [
780+
{ function: 'count', alias: 'order_count' },
781+
{ function: 'sum', field: 'amount', alias: 'total' },
782+
{ function: 'max', field: 'cap', alias: 'max_cap' },
783+
{ function: 'max', field: 'placed_on', alias: 'last_placed' },
784+
{ function: 'min', field: 'due_on', alias: 'first_due' },
785+
{ function: 'max', field: 'grace', alias: 'max_grace' },
786+
{ function: 'min', field: 'opened_at', alias: 'first_opened' },
787+
{ function: 'max', field: 'closed_at', alias: 'last_closed' },
788+
],
789+
};
790+
791+
// Before, measured through `engine.aggregate` on driver-memory and
792+
// driver-sql, both doors, and through `POST /data/:object/query`: every row
793+
// ANSWERED, by `@objectstack/formula`'s reading of the pair (a number read
794+
// as epoch milliseconds, a day added to it), where `driver-sql` refuses the
795+
// same pair on `where`. Each fragment is `driver-sql`'s own sentence.
796+
const REFUSED: ReadonlyArray<readonly [string, () => Record<string, unknown>, string]> = [
797+
['two numeric columns (the triage shape)', () => ({ total: { $gt: { $field: 'max_cap', addDays: 1 } } }), 'addDays adds whole days to a date or datetime column, and "max_cap" is numeric'],
798+
['a count against itself', () => ({ order_count: { $gte: { $field: 'order_count', addDays: 0 } } }), '"order_count" is numeric — an offset has no meaning on it'],
799+
['a date target against a numeric referent', () => ({ last_placed: { $lte: { $field: 'max_cap', addDays: 1 } } }), '"last_placed" is date but "max_cap" is numeric'],
800+
['a numeric target against a date referent', () => ({ total: { $gt: { $field: 'first_due', addDays: 1 } } }), '"total" is numeric but "first_due" is date'],
801+
['a date against a datetime', () => ({ last_placed: { $lte: { $field: 'last_closed', addDays: 1 } } }), '"last_placed" is date but "last_closed" is datetime'],
802+
['a datetime against a date', () => ({ last_closed: { $gte: { $field: 'first_due', addDays: 1 } } }), '"last_closed" is datetime but "first_due" is date'],
803+
['a groupBy text column against a date', () => ({ customer_id: { $lte: { $field: 'first_due', addDays: 1 } } }), '"customer_id" is text but "first_due" is date'],
804+
['a text offset column', () => ({ last_placed: { $lte: { $field: 'first_due', addDays: { $field: 'customer_id' } } } }), 'the addDays offset "customer_id" (text) is not a numeric column'],
805+
['a date offset column', () => ({ last_placed: { $lte: { $field: 'first_due', addDays: { $field: 'last_placed' } } } }), 'the addDays offset "last_placed" (date) is not a numeric column'],
806+
['a numeric pair nested under $not in a $or', () => ({ $or: [{ total: { $gt: 0 } }, { $not: { total: { $gt: { $field: 'max_cap', addDays: 1 } } } }] }), '"max_cap" is numeric'],
807+
];
808+
809+
for (const [name, having, fragment] of REFUSED) {
810+
it(`${name}: refused in driver-sql's words`, async () => {
811+
const message = await expectRowIndependentRefusal(offContract({ ...DT_QUERY, having: having() }), DT_ROWS);
812+
expect(message).toContain(fragment);
813+
});
814+
}
815+
816+
// Answered exactly as before, on both doors: the pairs the declaration admits.
817+
const ANSWERED: ReadonlyArray<readonly [string, FilterCondition, readonly string[]]> = [
818+
['date / date with a positive literal', { last_placed: { $lte: { $field: 'first_due', addDays: 7 } } }, ['c1', 'c3']],
819+
['date / date with a negative literal', { last_placed: { $gte: { $field: 'first_due', addDays: -3 } } }, ['c1', 'c2', 'c3']],
820+
['date / date with a numeric offset column (a max)', { last_placed: { $lte: { $field: 'first_due', addDays: { $field: 'max_grace' } } } }, ['c1']],
821+
['date / date with a numeric offset column (a count)', { last_placed: { $lte: { $field: 'first_due', addDays: { $field: 'order_count' } } } }, ['c3']],
822+
['datetime / datetime', { last_closed: { $gte: { $field: 'first_opened', addDays: 1 } } }, ['c1', 'c2']],
823+
['a numeric pair with NO addDays (the rule is the offset\'s)', { total: { $gt: { $field: 'max_cap' } } }, ['c1']],
824+
];
825+
826+
for (const [name, having, expected] of ANSWERED) {
827+
it(`${name} answers ${JSON.stringify(expected)} on both doors`, async () => {
828+
for (const [door, native] of DOORS) {
829+
const { engine } = await makeEngine(DT_ROWS, native);
830+
expect(groups(await engine.aggregate(OBJECT, { ...DT_QUERY, having })), door).toEqual([...expected]);
831+
}
832+
});
833+
}
834+
835+
it('a "day" date bucket is a date column; a coarser bucket is a text label', async () => {
836+
const bucketed = (granularity: 'day' | 'month', having: FilterCondition): EngineAggregateOptions => ({
837+
groupBy: [{ field: 'placed_on', dateGranularity: granularity, alias: 'placed' }],
838+
aggregations: [{ function: 'min', field: 'due_on', alias: 'first_due' }],
839+
having,
840+
});
841+
const shifted = { placed: { $lte: { $field: 'first_due', addDays: 1 } } };
842+
for (const [door, native] of DOORS) {
843+
const { engine } = await makeEngine(DT_ROWS, native);
844+
const rows = await engine.aggregate(OBJECT, bucketed('day', shifted));
845+
expect(rows.map((r: any) => r.placed).sort(), door).toEqual(['2026-01-02', '2026-01-15', '2026-02-01']);
846+
}
847+
const message = await expectRowIndependentRefusal(bucketed('month', shifted), DT_ROWS);
848+
expect(message).toContain('"placed" is text but "first_due" is date');
849+
});
850+
851+
it('a column whose class the declaration cannot tell is not judged — the pair is answered as before', async () => {
852+
// `ghost` is declared nowhere, so `min(ghost)` has no class to judge. The
853+
// engine keeps the fail-open direction its other declared-type doors take
854+
// for what the declaration cannot see; the reference reads no value, so
855+
// the ordering is false in every group.
856+
const query = offContract({
857+
...DT_QUERY,
858+
aggregations: [...DT_QUERY.aggregations!, { function: 'min', field: 'ghost', alias: 'ghost_min' }],
859+
having: { first_due: { $gte: { $field: 'ghost_min', addDays: 1 } } },
860+
});
861+
for (const [door, native] of DOORS) {
862+
const { engine } = await makeEngine(DT_ROWS, native);
863+
expect(groups(await engine.aggregate(OBJECT, query)), door).toEqual([]);
864+
}
865+
});
866+
});

‎packages/objectql/src/engine.ts‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -259,6 +259,7 @@ import {
259259
import {
260260
applyHaving,
261261
aggregatedRowColumns,
262+
aggregatedRowColumnClasses,
262263
assertAggregationFilterIsEvaluable,
263264
assertHavingIsEvaluable,
264265
assertHavingIsFilterCondition,
@@ -16080,7 +16081,19 @@ export class ObjectQL implements IObjectQLEngine {
1608016081
assertListComparandShapes(object, 'aggregate', query.having, 'having');
1608116082
{
1608216083
const having = normalizeFilterComparandTypes(query.having, `aggregate('${object}')`, 'having');
16083-
assertHavingIsEvaluable(having, aggregatedRowColumns(query.groupBy, query.aggregations));
16084+
// [#20127] …and each column's class, read off the query and the
16085+
// object's declaration, so a `{ $field, addDays }` pair is judged by
16086+
// the rule `FieldReferenceSchema.addDays` declares (two temporal
16087+
// columns of one class) rather than answered by epoch-ms coercion.
16088+
assertHavingIsEvaluable(
16089+
having,
16090+
aggregatedRowColumns(query.groupBy, query.aggregations),
16091+
aggregatedRowColumnClasses(
16092+
query.groupBy,
16093+
query.aggregations,
16094+
(this._registry.getObject(object) as { fields?: Record<string, unknown> } | undefined)?.fields,
16095+
),
16096+
);
1608416097
if (having !== query.having) query = { ...query, having };
1608516098
}
1608616099
const driver = this.getDriver(object);

0 commit comments

Comments
 (0)