Commit 16c5a33
fix(objectql)!: per-aggregation filter and having refusals belong to the query, not the data — row-independent walk, shape and type doors, unknown having keys, temporal addDays pairs (#20147)
Fixes #20122
Fixes #20123
Fixes #20127
Clause-②: no (narrowing)
A refusal on `engine.aggregate` is now a property of the query, never of
the data. The per-aggregation `filter` and `having` are judged once,
before any driver is asked for a row, and an empty table refuses what a
full one refuses. This is the combined claim of #20122 (the chain head),
#20123 and #20127, with the seat's amendment 5832109186 folding the
per-aggregation filter's shape door into #20122, and amendment 2
(5832826252) ruling that an array filter is refused too.
| commit | card | change |
|:--|:--|:--|
| `866212209f` | #20122 | the per-aggregation `filter` loop runs the
comparand-TYPE door and a row-independent walk
(`assertAggregationFilterIsEvaluable`) |
| `71e3ae2092` | #20123 | a `having` key naming no column of the
aggregated row is refused |
| `1690b79b59` | #20127 | `having` evaluates `addDays` only between two
temporal columns of one class; each aggregated column's class is read
statically (`aggregatedRowColumnClasses`) |
| `3c2fa822db` | | merge of `main` at `949e99bed9`, where PR #20144
landed |
| `a17770aa77` | #20122 | the shape gate `where` takes, on the
per-aggregation `filter` (seat amendment 5832109186) |
| `796b06a12e` | | changeset prose only |
| `211cfba477` | #20122 | an array per-aggregation `filter`, `[]`
included, is refused (seat amendment 2, 5832826252, ruling A) |
Session `session_01Bvd69VPa6puiNzzPUroDBx`, branch
`claude/issue-20122-aggregate-filter-doors`. The three card commits sit
on base `f09d4122bc`. The branch merged `main` at `949e99bed9`, and head
is `211cfba477` (`engine.ts` `45301fa9e4`, `having-filter.ts`
`876293a1e9`). Readings below name the commit they were taken on.
## 1. Measured first
A scratch harness (not committed) ran each shape through the public
`ObjectQL.aggregate` and through `POST /api/v1/data/:object/query`
(`RestServer`, then `ObjectStackProtocolImplementation.findData`, then
`ObjectQL.aggregate`). It used a real `InMemoryDriver` and a real
`SqlDriver` (better-sqlite3 `:memory:`), on a populated table (6 rows,
groups c1, c2, c3) and on an empty one, and counted driver calls.
Per-aggregation filters ran grouped and ungrouped. Each `having` ran on
both `applyHaving` doors: the native `driver.aggregate()` door, and the
fallback forced by an extra per-aggregation filter. That is 1464 cells
per tree, plus 72 for six extra shape-door shapes. The base is
`f09d4122bc`, and the shape-door base is the merged tree before
`a17770aa77`. On every row below, both drivers and both doors agreed.
### H1 (#20122): held for the walker's refusals, partly falsified for
the type door
| `aggregations[i].filter` | base, empty table | base, populated | head
|
|:--|:--|:--|:--|
| `{ amount: { $median: 1 } }`, `$nand`, `$regex`, `$regex` with
`$options`, `$like`, a dangling `$like` escape, `$ilike`, a non-`$` key
beside an operator, `$median` under `$not`, a bare `{ $field }` | `200
[]` grouped, `[{ n: 0 }]` ungrouped | 400 `INVALID_FILTER`, raised after
`find` (1 call) | 400 `INVALID_FILTER`, 0 driver calls, both
populations, engine and REST |
| an empty or non-string `$icontains` | the same at the engine; the REST
door already refused it (`VALIDATION_FAILED` / 400), whatever the rows |
the same | 400 at the engine, 0 calls; REST unchanged |
| `{ nope: { $median: 1 } }` (a column the source row does not carry) |
`[]` | counted no row in any group, no error | 400, 0 calls |
| `$median` in a `$or` branch after one that held | `[]` | counted EVERY
row (c1 2, c2 3, c3 1) | 400, 0 calls |
| a `{ $field }` as an `$in` member or `$contains` pattern / as a `$nin`
member or `$exists` operand | `[]` | no row / every row | 400, 0 calls |
| `addDays: 1.5` / `addDays: '7'` | `[]` | answered (c1 1, c2 2, c3 1 /
c1 2, c2 2, c3 1) | 400, 0 calls |
| type door: `{ $eq: { v: 1 } }`, an implicit `undefined`, `{ $eq: new
Map() }`, a function under `$gt`, an `undefined` `$in` member, a bigint
beyond 2^53, `{ $gt: { $field: 5 } }` | `[]` | counted no row | 400, the
type door's words rooted at `aggregations[i].filter`, 0 calls |
| type door: a `Symbol` under `$ne` / under `$gt` | `[]` | every row / a
raw `TypeError` with no `code` and no `status` | 400, 0 calls |
So H1 holds for every walker refusal. For the type-mismatched comparands
the premise was only half right. They were not refused on a populated
table either: they answered silently, apart from the uncoded `Symbol`
throw. The head refuses all of them before any read, as H1's head asks.
The exact-range bigint changes an answer instead of narrowing. `{
amount: { $in: [400n, 20n] } }` counted no row, and it now counts c1 1,
c3 1, because it is narrowed as `where` narrows it.
### H2 (#20123): held
| `having` | base, both populations | head |
|:--|:--|:--|
| `{ totl: { $gt: 100 } }`, `{ totl: 500 }`, `{ amount: { $gt: 100 } }`
(a source column), `{ $and: [{ total: { $gt: 0 } }, { totl: … }] }`, `{
'customer_id.name': 'x' }`, `{ customer_id: 'c1' }` under an aliased
groupBy | no group, no error | 400 `INVALID_FILTER`, 0 driver calls,
both doors, engine and REST |
| `{ totl: { $ne: 1 } }`, `{ totl: { $exists: false } }`, `{ $not: {
totl: … } }`, `{ $or: [{ total: { $gt: 0 } }, { totl: … }] }` | EVERY
group on a populated set | 400, 0 calls |
Controls, accepted and byte-identical: a groupBy column, an aggregation
alias, a `count` alias, a structured item's alias (`cust`), keys nested
under `$and` / `$or` / `$not`.
### H3 (#20127): held
| `having` pair with `addDays` | base, populated | head |
|:--|:--|:--|
| two numeric aliases (`total` vs `max_cap`) | no group | 400, "addDays
adds whole days to a date or datetime column, and "max_cap" is numeric —
an offset has no meaning on it." |
| a `count` against itself, `addDays: 0` | every group | 400, same words
|
| `date` vs numeric, numeric vs `date`, groupBy text vs `date` | no
group | 400, driver-sql's cross-class sentence |
| `date` vs `datetime` / `datetime` vs `date` | c3 / c2, c3 | 400,
cross-class |
| a `date` pair whose offset column is text or `date` | no group | 400,
"the addDays offset … is not a numeric column, and a day offset must be
a number of days." |
Controls, byte-identical: `date` / `date` with `7`, with `-3`, with an
offset from a numeric `max`, and with an offset from a `count`;
`datetime` / `datetime`; a `day` bucket against a `date`; and every `{
$field }` pair WITHOUT `addDays`.
### H4: held
664 control cells over 43 control shapes answered byte-identically at
base and head. They cover the #20122 controls (implicit equality, `$gt`,
`$in`, `$nin`, `$between`, `$icontains`, `$startsWith`, `$ne: null`,
`$exists`, `$null`, `$or`, `$not`, `{}`, a scalar `{ $field }`,
`addDays` date pairs, an exact bigint, a `Date` bound), the #20123 and
#20127 controls above, the structured-groupBy probes, and the H5 shapes
the shape gate leaves alone.
### H5: folded in (seat amendments 5832109186 and 5832826252)
| `aggregations[i].filter` | base, engine | base, REST | head, engine |
|:--|:--|:--|:--|
| a string | memory: every row of every group; sql: `NOT_IMPLEMENTED` /
501 (driver-sql's native aggregate saw the key) | `VALIDATION_FAILED` /
400 | 400 `INVALID_FILTER`, 0 driver calls |
| a number, `true`, `false`, `0`, `''` | every row of every group, both
drivers | `VALIDATION_FAILED` / 400 | 400, 0 calls |
| a `Map`, a `Date`, a `Set` | every row of every group | (not JSON) |
400, 0 calls |
| `[]` | no filter (every row) | `VALIDATION_FAILED` / 400 | 400
`INVALID_FILTER`, 0 driver calls (from `211cfba477`) |
| `[['amount', '>', 100]]` | counted no row: the walker reads its index
keys as column names | `VALIDATION_FAILED` / 400 | 400 `INVALID_FILTER`,
0 driver calls (from `211cfba477`) |
| a null-prototype filter object | filters correctly | (not JSON) |
unchanged |
The reach is in-process only: the REST door refuses every JSON shape
through `AggregationNodeSchema`. An array is refused because the slot is
declared `FilterConditionSchema`, which admits no array form (the
condition-array sugar is lowered on `where` alone), and REST already
refused every array there with `VALIDATION_FAILED`: one rule at both
doors, as `having`'s condition check has on #20099.
## 2. What changed
- **`packages/objectql/src/engine.ts`, `ObjectQL.aggregate` only.**
- The per-aggregation `filter` loop now opens with the shape gate
`where` takes. It reuses `isWhereFilterObject` /
`describeNonFilterWhere` from PR #20144 and `where`'s words, adapted
because no array is accepted here: "`aggregate('order'):
'aggregations[1].filter' must be a filter object, received string "…".
It was not applied, and an unapplied filter would have aggregated every
row of each group for that aggregation.`". An array of any length, `[]`
included, is refused first, naming the object form: "`… must be a filter
object, received an array ([["amount",">",100]]). The condition-array
form … is input-only sugar lowered on 'where' alone …`". `undefined`,
`null`, a plain object and a null-prototype object pass as before.
- After the doors the loop already ran, it adds
`normalizeFilterComparandTypes` rooted at `aggregations[i].filter`,
copying a narrowed bigint on write, and then
`assertAggregationFilterIsEvaluable`.
- The `having` entry passes `aggregatedRowColumnClasses(groupBy,
aggregations, object fields)` to `assertHavingIsEvaluable`.
- **`packages/objectql/src/having-filter.ts`.**
- The row-independent walk takes a scope: the clause whose words it
speaks (`having`, or `aggregations[i].filter` through
`aggregationFilterClause`), plus, where the position has a closed
namespace, the column set and the column classes.
- `assertAggregationFilterIsEvaluable(filter, index)` runs the walk with
the per-aggregation clause and no column set. The filter reads the
object's raw columns, whose names the engine does not judge on `where`
either, so the `{ $field }` NAME check stays `having`'s.
- The #20123 refusal (`unknownHavingColumnError`) collects keys naming
no column of `aggregatedRowColumns` at any depth and refuses them once
the rest of the clause has passed. Operators are judged first, so `{
nope: { $median: 1 } }` keeps its operator refusal, and its pin is
unchanged. The refusal names every unknown key, the first with its
position, and lists the columns. It opens the way the REST ingress's
unknown-`where`-field refusal does: "`having` filters on 'totl' at
having.totl, which is not a column of the aggregated row … so the query
was refused instead of answered."
- For #20127, `aggregatedRowColumnClasses` reads each column's class
statically. A groupBy projection takes its field's declared type through
the spec's value-class sets. A `day` bucket is a `date`, by the
`YYYY-MM-DD` label contract, and a coarser bucket is a text label.
`count` / `count_distinct` / `sum` / `avg` are numeric, and `min` /
`max` take their field's type. A class the declaration cannot tell (no
field map, an undeclared field, a `formula`) is not judged.
`assertOffsetPairIsTemporal` applies driver-sql's `addDays` arm in its
order, in its sentences: same class, then a temporal referent, then a
numeric offset column.
- **Tests**:
- `engine-aggregate-filter.test.ts` gains the #20122 tables. Each
refusal runs on both stand-in driver kinds, empty and populated, grouped
and ungrouped, asserting `code`, `status`, one message and 0 driver
calls. The walker rows are held to the per-row floor's words, and the
type rows to the type door's own words. The file also gains controls and
the shape-gate table.
- `engine-aggregate-having-comparand-shape.test.ts` gains the #20123 and
#20127 tables, both doors, empty and populated, with controls.
- **Three changesets**, one per card, all `@objectstack/objectql`
`minor`.
## 3. Declaration (H6)
`Clause-②: no (narrowing)`, BREAKING, `minor`. The accept set only
narrows. The one answer change of an already-accepted input is the
bigint narrowing, a correction to the answer `where` gives.
- `check-changeset-no-major --base origin/main`: "✓ This diff introduces
no `major` bump." Its level axis reads NOT APPLICABLE locally, with no
`pull_request` payload; CI reads it from this body.
- `check-adr-0087-registration --base origin/main`: "✓
check-adr-0087-registration: 3 declared-breaking changeset(s), each
carrying an ADR-0087 disposition."
- 20122: `not-required (already-registered
filter-between-field-reference-endpoint-refused,
filter-icontains-comparand-refused-at-parse,
filter-regex-options-retired)`.
- 20123 and 20127: `not-required (no-migration-prescription)`. Their
tables record before and after and prescribe no rewrite: a typo'd column
or a non-temporal `addDays` pair has no accepted spelling to migrate to.
## 4. Tests, reverse verification, ablation
At `211cfba477`:
- `@objectstack/objectql`, whole suite, both vitest projects: `Test
Files 317 passed (317)` · `Tests 5589 passed (5589)`. The two aggregate
files alone: `Test Files 2 passed (2)` · `Tests 211 passed (211)`.
- `pnpm --filter @objectstack/objectql run typecheck`: exit 0.
`check:test-typecheck` reads "OK … 40 file(s) / 234 error(s) / 65 pinned
signature(s)", unchanged.
At `a17770aa77` (before the array refusal; not re-run for `211cfba477`):
- Consumer suites, run against the rebuilt `objectql` `dist/`, which
carries the new symbols (2 hits each):
- REST (`list-view-grouping-query-door`,
`rest-server-canonical-query-ast`, `request-schema-gate.conformance`):
81 passed, 1 skipped (pre-existing);
- `metadata-protocol` (`protocol.query-param-arity`,
`protocol.read-verb-canonical-fold`): 64 passed;
- `plugin-security` `predicate-guard`: 10 passed;
- the dogfood aggregate and analytics tests
(`analytics-inline-dataset-admission`, `analytics-label-scope`,
`analytics-rls`, `analytics-timezone`, `date-bucket-parity-conformance`,
`date-bucket-parity-turso`, `empty-group-bucket-parity`,
`group-key-read-shape-parity`) plus PR #20144's
`engine-where-shape-refusal`: `Test Files 9 passed` · `Tests 55 passed`.
- **Reverse verification.** `engine.ts` and `having-filter.ts` were
restored to their merge-base blobs (`4f5d28170b`, `7d9f8ace64`) under an
EXIT/INT/TERM trap, with the fix committed first. The two test files
then read `59 failed | 150 passed (209)`, which is exactly the new
refusal and narrowing rows. The restore was proven by HEAD-blob equality
and an empty `git diff HEAD`. At `1690b79b59`, before the shape gate,
the same run against the `f09d4122bc` blobs read `53 failed | 148 passed
(201)`.
- **Ablation**, one per door, through `scripts/ablation-replace.mjs`:
the first four at `1690b79b59`, the shape gate at `a17770aa77` and
again, branch by branch, at `211cfba477`. Each anchor hit once (x1 to
x0), the blob moved, and the file was restored to its HEAD blob with
`git diff HEAD` empty. The tests import `./engine.js` from source, so no
`dist/` is on their resolution path.
| ablated | failed / total | red set |
|:--|:--|:--|
| `assertAggregationFilterIsEvaluable(typed, i)` | 19 / 201 | the 12
walker rows and the 7 reference rows |
| the type-door call (the filter passed as is) | 11 / 201 | the 10 type
rows and the bigint narrowing |
| the #20123 key push | 12 / 201 | the 10 unknown-key rows, the "every
key named" row, the aliased-groupBy row |
| the #20127 `assertOffsetPairIsTemporal` call | 11 / 201 | the 10 pair
rows and the month-bucket row |
| the shape gate's condition (to `false`), at `a17770aa77` | 6 / 209 |
the 6 shape rows |
| at `211cfba477`: the array branch's condition (to `false`) | 3 / 211 |
the 3 array rows |
| at `211cfba477`: the non-object branch's condition (to `false`) | 6 /
211 | the 6 non-object rows |
| at `211cfba477`: both conditions (the whole gate) | 9 / 211 | the 6
non-object and 3 array rows |
## 5. Gates
- Derived with `node scripts/pm/dispatch-gates.mjs --commands --repo
objectstack-ai/objectstack`, 7 paths against merge base `949e99bed`: 64
commands. Each was run before any pipe and its exit code recorded, at
`796b06a12e`: all 64 exit 0. At `211cfba477` the gates the patch round
touches were re-run:
- `check-changeset-no-major --base origin/main --event` (this body): "✓
This diff introduces no `major` bump." · "✓ LEVEL AXIS: this PR declares
clause-② `no (narrowing)`, and no package whose `packages/**/src/**` it
moves is graded `patch`."
- `check-adr-0087-registration --base origin/main`: "✓
check-adr-0087-registration: 3 declared-breaking changeset(s), each
carrying an ADR-0087 disposition."
- `check-empty-changeset --base origin/main`: "✓ No empty-frontmatter
changeset introduced by this diff (3 declaring changeset(s) added)."
- `check-issue-citations --base 949e99b`: "every citation this change
adds resolves" (25 judged).
- `check:nul-bytes`: "OK (scanned 9571 text file(s) … no raw ASCII
control bytes)". `--ran`: "Run reconciliation — 64 derived, 64 run, 0
NOT-MEASURED, 0 UNRUN."
- `check:dual-build-cjs-loads` first exited 3 (PREREQUISITE NOT MET).
The eight packages it named were built, and it reran at exit 0.
- `node scripts/check-issue-citations.mjs --base 949e99b`: "every
citation this change adds resolves" (24 judged).
- Lint, a declared narrowing, because `pnpm lint` is CI's. `eslint
--no-inline-config --format json` over the 4 changed `.ts` files reads 4
files, 0 errors, 0 warnings, 0 fatal. `--print-config` returns a config
for each file. `eslint.config.mjs` sets no `parserOptions.project` and
no typed rule, so no untouched file's verdict can move.
- Control-byte self-scan of the 7 changed files: grep exit 1 (none).
## 6. Deviations and conflicts, declared
- **The array question is ruled.** The first round kept arrays outside
the shape gate, as the first amendment instructed, and held a condition
array's old answer (no row) in a test marked as kept, not approved. The
seat ruled A in amendment 2 (5832826252): an array, `[]` included, is
refused, because `AggregationNodeSchema.filter` is
`FilterConditionSchema` and REST already refuses every array.
`211cfba477` does that; the held control became three refusal rows.
- **#20123's code.** The card's suggested shape said "the unknown-field
refusal's envelope", which is `INVALID_FIELD`. The dispatch's H2 said
`INVALID_FILTER`. This PR uses `INVALID_FILTER`, the code of every other
`having` refusal, including #20099's unresolved `{ $field }` refusal
over the same column set. The name is a column of the query's own
projection, not a field of the object, and the words follow the
unknown-`where`-field refusal.
- **Real drivers in the pins.** The committed pins use the stand-in
drivers with call counters. `@objectstack/objectql` has no driver
dependency, and the amendment keeps the file surface unchanged. The
`driver-memory` and `SqlDriver` legs, through the engine and through
REST, are the scratch measurement above.
- **The `{ $field }` refusal words for a bare reference in a
per-aggregation filter.** On a populated table this was already refused,
as an unsupported `$field` operator. It is now refused whatever the
rows, in the walk's bare-reference words, with the same code and status.
No committed test pinned the old text.
## Acceptance notes
Observed and not fixed here. The report carries each with its class and
evidence; the seat files the per-aggregation family as one class-closure
card (amendment 2, 5832826252).
- **Docs drift check (5832744152), read and receipted here.** It names
six hand-written pages, each only through the `count_distinct` literal
in `NUMERIC_RESULT_FUNCTIONS`. Re-read against this change: none states
anything it falsifies. `data-modeling/queries.mdx` already gives
`having`'s namespace as the aggregated row's own columns, and
`protocol/objectql/query-syntax.mdx` gives the `addDays` class rule this
change now applies to `having`. One pre-existing stale row, not
introduced here: `query-syntax.mdx`'s table of members "not executed on
the `find()` path" still calls `aggregations[].filter` "EXPERIMENTAL —
not enforced", although the engine has enforced it since #10576. Left to
the docs lane. The two release-owned pages are read-only.
- The per-aggregation filter still lacks `where`'s temporal-comparand
door. `{ placed_on: { $gt: 'not-a-date' } }` counts no row on both
drivers, while the same predicate as a `where` is refused 400.
- A `Date` comparand in a per-aggregation filter counts no row against
an ISO-text `datetime` column on both drivers: the walker compares a
string with a `Date`. `driver-sql` answers the rows for the same
`where`.
- `addDays` on a numeric pair in a per-aggregation filter still answers
by coercion (no row). The #20127 rule classifies only the aggregated
row's columns; the per-aggregation position would read its classes from
the object's declared types, where `driver-sql` withholds the reason
from the wire.
- A `{ $field }` in a per-aggregation filter naming no field of the
object counts no row, where `driver-sql` refuses it on `where`. The
engine keeps its registry-less tolerance on names.
- `POST /data/:object/query` with `aggregations: [{ …, filter: { nope: 1
} }]` answers 200 with zero counts. The REST ingress refuses the same
unknown key in `where` (`INVALID_FIELD` / 400), and it does not judge
per-aggregation filter keys.
- A `having` `{ $field }` pair across classes WITHOUT `addDays` (a sum
against a date) still answers by coercion. `driver-sql` refuses
cross-class pairs on `where`, but `FieldReferenceSchema` declares the
class rule only for `addDays`, and this change keeps to that.
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent 437bb0d commit 16c5a33
7 files changed
Lines changed: 973 additions & 26 deletions
| 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 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 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 | + | |
| 28 | + | |
| 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 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
0 commit comments