fix(objectql,driver-sql)!: a group key is the column's value, in the shape find() presents it (#3849) - #3864
Merged
Merged
Conversation
…shape `find()` presents it (#3849) Three code paths produce a `groupBy` key and no two agreed: qty (number) won (boolean) find() 3 number true boolean aggregate() pushed down 3 number 0/1 number in-memory fallback '3' string 'false'/'true' string Two independent causes. `applyInMemoryAggregation` ran every key through `String()`, which the pushed-down path never did. And the pushed-down path returns raw builder output: #3797 taught it to present temporal columns the way `formatOutput` does on a `find()` row, but not the boolean and numeric repairs, so a SQLite boolean — no native type, stored 0/1 — surfaced as an integer from `aggregate()` and a real boolean from `find()`. `engine.aggregate` picks between the two aggregate paths per query, so the same column changed shape with no change to the data or the query. The measures were always right, which is why nobody noticed. What broke was downstream code probing a raw `Map` keyed by the value's own type — `Map` lookup is SameValueZero, so '1' never finds 1: select-option labels fell back to raw values, lookup labels missed whenever the referenced object had a numeric id (routine for external objects), and cross-object rebucketing filed every row under RESTRICTED_BUCKET — one bar, correct grand total, no error. A boolean dimension drilled from the in-memory path also sent { won: 'true' } to SQLite, which an INTEGER column can never match. - `applyInMemoryAggregation` emits the value verbatim; its rows are find() output, so passing through is what makes the key equal the column's read shape. - The internal bucket id is type-preserving, so 1 and '1', true and 'true' stay distinct groups. BigInt is encoded explicitly — JSON.stringify throws on it, and a value that bucketed fine under String() must not start crashing. - `SqlDriver.aggregate`/`.distinct` present group keys and min/max with the same rules `formatOutput` applies, generalizing #3797 to boolean and numeric columns. The protected helpers are renamed to match (temporalFieldKind → readPresentationKind, presentTemporalValue → presentReadValue, presentTemporalColumns → presentReadColumns) and the kind union is exported. Date-bucketed groupBy is unaffected: both sides already produce canonical string labels, and #3839 pinned their empty bucket. Gate: group-key-read-shape-parity.test.ts measures both aggregate paths against find() for number, boolean and text columns on driver-sql and driver-sqlite-wasm, asserting the runtime TYPE — folding through String() is the reflex that hid this and would pass against the bug. Each half was confirmed to redden the gate on its own. Co-Authored-By: Claude <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📓 Docs Drift CheckThis PR changes 3 package(s): 20 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3849. Follow-up to #3848.
Outcome
groupBy: ['qty']returns3, not'3'.groupBy: ['won']returnstrue/false— not'true'/'false'on one path and1/0on the other. A bucket key is a column value, so there is one right answer for what it looks like: whatever that column looks like on afind()row.What was wrong — and it was not only the
String()Three paths produce a group key. Measured on better-sqlite3 before this change:
qty(number)won(boolean)find()3numbertruebooleanaggregate()pushed down3number0/1number'3'string'false'/'true'stringTwo independent causes, and the issue only named the first:
applyInMemoryAggregationran every key throughString(). The pushed-down path never did.Field.datetime的裸 epoch 从聚合出口漏出去(#3773 同根不同出口) #3797 taught it to present temporal columns the wayformatOutputdoes on afind()row — but not the boolean and numeric repairs. A SQLite boolean has no native type and is stored0/1, so it surfaced as an integer fromaggregate()and a real boolean fromfind().So deleting the
String()alone would not have closed this: it would have leftfalse/trueagainst0/1, which is still a mismatch under===and underMaplookup. Both halves are needed, and both are gated separately below.engine.aggregatechooses between the two aggregate paths per query — native aggregate? advertised granularity? UTC? — so the same column changed shape with no change to the data or the query.Why it mattered
Every measure stayed correct, which is why this survived. What broke is downstream code that probes a raw
Mapkeyed by the value's own type.Maplookup is SameValueZero, so'1'never finds1:dimension-labels.ts) — table keyed by the option's ownvalue; a numeric option value never matched a stringified key, so the chart rendered the raw stored value.cross-object-rebucket.ts) — same build-and-probe shape, but a miss is not a fallback, it isRESTRICTED_BUCKET. A numeric FK filed every row under'(restricted)': one bar, correct grand total, no error anywhere.{ won: 'true' }to SQLite, whose INTEGER column can never equal the text'true'. Zero rows.Direction
Converge on raw, i.e. on what
find()presents. Every driver that aggregates natively already returns raw values (driver-sql, sqlite-wasm, memory, mongodb, and cloud's Turso remote transport); framework's own third in-memory grouper (preview-evaluator.ts) returns raw;memory-driverandcross-object-rebucketalready stringify the internal key while emitting the value raw.applyInMemoryAggregationwas the sole outlier.Coercing in SQL instead would mean a
CAST(… AS TEXT)in four driver implementations plus hand-written analytics SQL, one of them in a different repo and release train — and dialects disagree on the text form of a boolean ('t'on Postgres,1on MySQL/SQLite) and of a float, so it would trade a JS-vs-SQL divergence for a worse Postgres-vs-MySQL-vs-SQLite one. It would also universalize the label bugs above instead of fixing them.No zod schema, TS type, or JSON schema constrains the shape either way.
DatasetDimensionSchema.typeexplicitly admitsnumberandboolean.Changes
applyInMemoryAggregation(objectql) emits the value verbatim.1and'1',trueand'true'stay distinct groups rather than merging on the way in. BigInt is encoded explicitly —JSON.stringifythrows on it, and a value that used to bucket underString()must not start crashing the aggregate.SqlDriver.aggregate/.distinctpresent group keys andmin/maxresults with the same rulesformatOutputapplies, generalizing aggregate()/distinct() 不过 formatOutput —— SQLite 上Field.datetime的裸 epoch 从聚合出口漏出去(#3773 同根不同出口) #3797 to boolean and numeric columns. Theprotectedhelpers are renamed to match (temporalFieldKind→readPresentationKind,presentTemporalValue→presentReadValue,presentTemporalColumns→presentReadColumns); the kind union is exported asReadPresentationKind. Nothing outsidesql-driver.tsreferenced the old names, including cloud'sTursoDriver.Date-bucketed
groupByis unaffected — both sides produce canonical string labels, and #3839 pinned their empty bucket.Gate
packages/qa/dogfood/test/group-key-read-shape-parity.test.tsmeasures both aggregate paths againstfind()for a number, boolean and text column, ondriver-sqlanddriver-sqlite-wasm. It asserts the runtime type, not just the value — folding both sides throughString()is the reflex that hid this for so long and would make the check pass against the bug it exists to catch.Each half proven to redden it independently:
expected [ '0<number>', '1<number>' ] to deeply equal [ 'false<boolean>', 'true<boolean>' ]Plus objectql unit pins for type preservation, for
1/'1'/true/'true'/null/'null'landing in six distinct buckets, and for a BigInt key not throwing.Verification
pnpm buildclean, no generated-artifact drift;pnpm lintclean;check:nul-bytes/check:doc-authoring/check:role-wordOK.UI impact
Not browser-verified — this is a server-side change; I traced the render path in objectui source instead.
formatDimensionValuesends integers throughString(v)(identical output to before) and booleans throughString(v)→"true"/"false"(also identical). The one visible delta: a non-integer dimension value now goes throughtoLocaleString(maximumFractionDigits: 2), so grouping by a float with more than two decimals rounds in the label — the same rounding numbers already get everywhere else in the UI. Grouping by a float is unusual; flagging it rather than burying it.🤖 Generated with Claude Code