You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Commit d5f4c2a
Browse filesBrowse the repository at this point in the historyBrowse files
fix(service-analytics): an all-NULL sum group sorts and pages as the 0 it answers, the same on SQLite and PostgreSQL (#25953)
<!-- claude-projects-attribution: {"github_login":"os-elon-musk"} -->
_Requested by **Elon** · [project
thread](https://claude.ai/code/project/chan_01WedDzV26nniwxv7jC88B9D?thread=cmsg_01WedDzV26nniwxv7jC88B9D6RGhxHE9XKMJQY9XoRwmZW)_Fixes#25907
Clause-②: no
Metadata-protocol: none
Every query accepted today is still accepted, and no key, code or schema
changes. The answer becomes the same on every named dialect and agrees
with the identity `emptyGroupValueFor` already declares. Nothing under
`packages/spec/**`, and no new error code.
## What was wrong (reproduced red first, on both dialects)
The #21042 fold rewrote a NULL `sum` to `0` in JavaScript, after
`executeRawSql`, so the database had already ordered and paged on the
NULL. Measured at `AnalyticsService.query` (what `POST /analytics/query`
calls) on base `f00e8d5c`, with the new test's fixture (`total` per
group: d -3, c all-NULL, a 5, NULL-group 7, b 10), `order { total: 'asc'
}, limit 2`:
| | native SQLite | native PostgreSQL 16 | ObjectQL face, both |
|:--|:--|:--|:--|
| page | `c 0`, `d -3` (a `0` before `-3`) | `d -3`, `a 5` (`c` cut from
the page) | `d -3`, `c 0` |
The base run of the new file: 24 of 29 red across both cells.
## What changed
`packages/services/service-analytics/src/strategies/native-sql-strategy.ts`
- **The identity is in the statement.** `withEmptyGroupIdentity` wraps a
measure's aggregate in `COALESCE(…, 0)` when `emptyGroupValueFor`
declares an identity for it, plain and measure-scoped (conditional)
forms alike. So `ORDER BY`, `LIMIT` and `OFFSET` read the value the
group answers. `count` / `count_distinct` are not wrapped: SQL `COUNT`
already answers `0` over nothing, on every dialect. The JS fold loop
after `executeRawSql` is removed. The statement is now the one place,
and the echoed `sql` / `/analytics/sql` show it.
- **NULL placement is stated.** `avg`, `min` and `max` keep NULL over
nothing (no identity), and a dimension can hold NULL. A new module
export, `orderClauseSql(order, dialect)`, next to `windowClauseSql`,
writes `NULLS LAST` on every key, in both directions, on `sqlite` and
`postgres`. That is `applyOrdering`'s rule: the ObjectQL face applies
it, and so does the dataset door's post-pass, which re-sorts a
pushed-down page. On SQLite the pushed-down window was being cut on one
order and then shown in another.
- **MySQL and `unknown` keep the driver's placement.** MySQL has no
`NULLS LAST`, and `unknown` covers it. An `x IS NULL` sort expression is
not a portable substitute: PostgreSQL resolves a name inside an
expression against the input columns, not the select alias. ⚠️ **MySQL
is NOT MEASURED**: there is no MySQL server here, and its arm is
asserted as text only.
- `orderClauseSql` is not re-exported from `src/index.ts`, so the
package's public surface is unchanged.
`packages/services/service-analytics/src/strategies/objectql-strategy.ts`
(surface amendment 6408215361)
- `generateSql`'s echo builds its `ORDER BY` through the same
`orderClauseSql`, the way it already builds the window through
`windowClauseSql`. The echo now states the nulls-last order the face
really applies. The `orderAndWindow` doc comment no longer says the
native face leaves NULL placement to the driver.
## Tests
New: `src/__tests__/native-sql-empty-sum-order-window.test.ts`, through
the real `ObjectQL` engine and `SqlDriver`, with a SQLite cell and a
live PostgreSQL cell (`OS_TEST_POSTGRES_URL`; the live PostgreSQL cell
ran locally, on PostgreSQL 16, because CI's Temporal Conformance job was
path-filtered off this PR):
- Nine literal pages on both faces: the card's `total asc, limit 2` → `d
-3, c 0`, offset and offset-only windows, `avg_score` / `min_score` with
the NULL group last in both directions, and the NULL dimension value
last in both directions.
- A sweep over every order key × both directions × four windows: the
native page equals the ObjectQL page, row for row.
- The statement carries `COALESCE(SUM(…), 0)`, carries no `COALESCE`
around `AVG` / `MIN`, and ends with `ORDER BY "total" ASC NULLS LAST,
"avg_score" DESC NULLS LAST LIMIT 2`. The echo and `generateSql` are
byte-equal to what ran.
- At the dataset door, the pushed-down `ORDER BY … NULLS LAST LIMIT 2`
answers `d, c` on both faces.
- The `unknown` arm (no `sqlDialect` hook), executed on both engines,
answers the same `sum` page, with no placement spelled.
- `orderClauseSql` text per dialect, MySQL included as text.
Pins flipped to the new bytes, each asserting the new semantics:
`measure-expression-sql`, `measure-field-and-filter-compilation`,
`objectql-dataset-filter` (the two native-door lines; the ObjectQL echo
line is unchanged), `native-sql-offset-only-window`,
`objectql-echo-offset-only-window` (it keeps its full cross-face
comparison from ` ORDER BY ` on), and `objectql-face-order-limit` (its
header now names where NULL placement is pinned).
Runs at head `acc8e8b0`, local PostgreSQL 16:
- `pnpm --filter @objectstack/service-analytics exec vitest run
--maxWorkers=2` with `OS_TEST_POSTGRES_URL` set: **Test Files 186 passed
(186), Tests 4806 passed | 2 skipped (4808)**. One environment note:
`read-scope-temporal-coercion`'s premise cell ("the server is not on
UTC") is red against a UTC PostgreSQL. It went green (15 passed, 1
skipped) once the test database's zone was set off UTC, which matches
CI's skewed-zone job. It is unrelated to this diff.
- `pnpm --filter @objectstack/service-analytics typecheck`: exit 0 (the
package's `tsc` includes `src/__tests__`).
- Ablations, each through `scripts/ablation-replace.mjs`, each restored
to the HEAD blob with `git diff HEAD` empty:
- The `COALESCE` return replaced with the bare aggregate: the new file
goes 24 failed / 5 passed of 29.
- `NULLS LAST` set to `null` for sqlite and postgres: 12 failed / 17
passed. Every `sum` page stays green, because `COALESCE` alone fixes the
card's rows. Every placement page goes red in the direction its driver
differs.
## Gates
- `node scripts/pm/dispatch-gates.mjs --commands` at `acc8e8b0` derives
68 families. Run reconciliation (`--ran`): **66 run, all exit 0; 2 NOT
MEASURED; 0 unrun.** The two NOT MEASURED, each exit 3 (`PREREQUISITE
NOT MET`):
- `node scripts/check-plugin-teardown-shape.mjs --self-test`: shallow
clone, the pinned positive-control commit is unreachable.
- `pnpm check:dual-build-cjs-loads`: needs every package's `dist/`.
- CI runs both.
- `pnpm lint` (the seat's addition): exit 0.
- The remaining families: the PR's check-runs.
## Merge note: PR #25903 (#25778) touches the same block
PR #25903 edits the fold loop this PR removes (duration measures fold by
`durationOf(member)?.aggregate`). It also adds
`resolveDurationMeasureSql`, which returns
`AGGREGATE_SQL[duration.aggregate](operand)` and its conditional form
without the identity. The two PRs conflict textually in `execute()`, so
neither merges silently. Whichever lands second should route the
duration aggregate through `withEmptyGroupIdentity(duration.aggregate,
…)` instead of restoring the JS fold. Otherwise an all-NULL duration
`sum` answers `null`. #25903's `dataset-duration-measure.test.ts` can
then take back the all-NULL group it excludes for this reason.
## Acceptance notes
- `''` in an ordered column: `applyOrdering` treats `''` as empty and
sorts it last, while SQL sorts `''` as text. The objectql-strategy doc
comment already names this difference, and it is unchanged here.
Carrier: none.
- The ObjectQL face's echo still prints a bare `SUM(…)`, while that face
answers `0` for an empty group through the driver's
`foldEmptyAggregateAnswers`. The echo runs and orders right, but would
print `NULL` for that cell. This is unchanged. Carrier: none.
- #25906 and #25814 (same file and axis) remain open and untouched here.
---
_Generated by [Claude
Code](https://claude.ai/code/session_016NB4rZT58RvxS6vBGLcXqz)_
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_016NB4rZT58RvxS6vBGLcXqz
---------
Co-authored-by: Claude <noreply@anthropic.com>
fix(service-analytics): an all-NULL `sum` group sorts and pages as the `0` it answers, the same on SQLite and PostgreSQL
6
+
7
+
Clause-②: no
8
+
Metadata-protocol: none
9
+
10
+
-**Before.** The native-SQL strategy folded a NULL `sum` (a group whose aggregand is NULL in every row, or a measure-scoped `sum` with no admitted row) to `0` in JavaScript, after the statement had already ordered and paged on the NULL. SQLite sorts NULL lowest and PostgreSQL highest, so `order { total: 'asc' }, limit 2` answered `0` before `-3` on SQLite and cut the `0` group from the page on PostgreSQL. The ObjectQL face answered `-3`, `0`.
11
+
-**Now.** The identity `emptyGroupValueFor` declares is in the statement, `COALESCE(SUM(…), 0)`, so `ORDER BY`, `LIMIT` and `OFFSET` read the value the group answers, on every dialect. The echoed `sql` and `/analytics/sql` show it. `count` is not wrapped: SQL `COUNT` already answers `0` over nothing.
12
+
-**NULL placement is stated.**`avg`, `min` and `max` keep NULL over nothing, and a dimension can hold NULL. On SQLite and PostgreSQL every `ORDER BY` key now carries `NULLS LAST`, in both directions: the ordering the ObjectQL face and the dataset door's post-pass already apply. A host whose dialect is MySQL or unnamed keeps the driver's placement, because no one spelling parses on all of them.
13
+
-**The ObjectQL face's echo states it too.** That face already orders NULL last (`applyOrdering`); its echoed `sql` and `/analytics/sql` now render the same `ORDER BY` as the native face, `NULLS LAST` included, so the printed statement runs to the order the face answered.
0 commit comments