Uh oh!
There was an error while loading. Please reload this page.
fix(objectql)!: one key for the empty group bucket — real null, on both aggregation paths (#3839) - #3848
Merged
Merged
Conversation
…both aggregation paths (#3839) `engine.aggregate` has two implementations of one feature and picks between them per query: it pushes the aggregate down as SQL when the driver advertises every requested granularity and the reference timezone is UTC, otherwise it fetches rows and buckets them in JS. The two disagreed about how to spell "empty" — SQL NULL against the in-memory literal `'(null)'` — so the same dataset produced a different bucket key type on SQLite+UTC+`month` than on `week`, a non-UTC timezone, or driver-rest / driver-memory / a remote Turso. The measures were always right; only the key's type and literal differed, which is why it went unnoticed. It was never date-specific either — a plain `groupBy: ['stage']` over a NULL column diverged the same way. Consumers are written against `null`: they check `== null` and supply their own empty label. The sentinel defeated every one of them, leaking a raw English debug string into the UI and compiling a drill on the empty bucket to `field = '(null)'`, which matches nothing. The comment justifying the string cited the client `useReportData` hook, removed with ADR-0021 — and the literal never appeared in it. - `applyInMemoryAggregation` / `bucketDateValue` key the empty bucket as `null`; `bucketDateValue` returns `string | null`. Null and unparseable instants still share one bucket, because SQL cannot tell them apart either. - The internal composite bucket id is JSON-encoded, so the empty bucket stays distinct from a row whose value is the literal string `"null"`. Same in service-analytics' cross-object rebucketing. - `bucketKeyToCalendarRange` accepts `string | null` — the empty bucket has no calendar span, so a drill on it opens the unscoped superset as before. - The driver output contract in spec now states the rule. Gates: `checkDateBucketParity`'s fixture deliberately had no null instant because the divergence would have failed it for a reason it was not about; it has one now. Two fixes made that meaningful — the check folded labels through `String(value)`, which turns SQL NULL into `'null'` and could compare equal to a sentinel string, and it compared label sets with `JSON.stringify`, which is sensitive to key insertion order that the two paths legitimately differ on (a correct driver could be flagged, with an empty diff message). A new dogfood check covers the non-date half against real drivers. Both gates were confirmed to fail with the sentinel restored. 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 8 package(s): 118 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
os-zhuang
commented
Jul 28, 2026
ContributorAuthor
Follow-up filed as #3849 — the |
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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#3839.
Outcome
A grouped row whose dimension value is empty now carries real
nullfor that dimension, whichever way the aggregate ran. Downstream code can test the empty bucket with a plainvalue == nullagain: charts render their own empty label, a drill on that bucket buildsfield = nulland returns the rows it should, and a dashboard no longer changes shape when the driver, the granularity or the reference timezone changes.The reproduction from the issue, re-run against this branch:
Which way, and why that direction
The issue asked for the investigation before the change. Enumerating every consumer of
'(null)'and of empty bucket keys across framework + objectui + cloud, the evidence is one-sided:'(null)'.in-memory-aggregation.tssaid the string existed "to remain consistent with the clientuseReportDatahook". That hook was removed with ADR-0021 — objectuipackages/plugin-report/src/index.tsxis now just its epitaph — and the literal never appeared in it even when it existed.null, and the sentinel actively breaks them. objectui's empty labels are all keyed off== null/??—formatDimensionValue→—,useGroupedData/PivotTable/ReportViewer→(empty), kanban → localized "Uncategorized",chart-seriesdrops the null series. The sentinel bypassed all of them and rendered a raw English debug string. Worse,buildDatasetDrillFilterandReportViewbuild the drill asfield = '(null)', so clicking the empty bucket returned zero rows instead of the empty-valued ones.null: the pushed-down SQL path, cloud's Turso remote transport, and framework's own newer semantic-layer evaluator (preview-evaluator.ts).Record<string, unknown>.Converging to
nullis therefore a bug fix, not just a cleanup.Changes
applyInMemoryAggregation/bucketDateValue(objectql) key the empty bucket asnull;bucketDateValuenow returnsstring | null. A null instant and an unparseable one still share one bucket, because SQL cannot tell them apart either (strftime('%Y-%m', 'not-a-date')is NULL)."null"—${null}is'null', so plain interpolation would have merged two real groups. Same in service-analytics' cross-object rebucketing.bucketKeyToCalendarRange(core) acceptsstring | null. Behavior unchanged — the empty bucket has no calendar span, so a drill on it opens the unscoped superset — but the call site no longer casts a lie.null, never a sentinel. Propagating NULL through the bucket expression is the whole of it; a driver only breaks it by adding aCOALESCE.Gates
As the issue requested,
checkDateBucketParity's deliberate exclusion is removed and itsFIXTUREnow carries a null instant.Two latent bugs in that check had to be fixed first, or the new fixture would have been meaningless — and one of them would have made it silently bless the divergence:
String(value), which turns SQL NULL into'null'— a label a TEXT column can genuinely hold. A side spelling "empty" as a string could compare equal to one returning real NULL. The empty bucket is now keyed out of band.JSON.stringify, which is sensitive to key insertion order. Row order is not part of this contract and the two paths legitimately differ (SQL sorts its groups; the in-memory path emits first-seen order), so a driver with entirely correct buckets was reported as disagreeing — with an empty diff message, sincedescribeDiffis keyed and could not name one. This actually fired on both real drivers once the null row was added. The comparison is now order-insensitive.A new dogfood check,
empty-group-bucket-parity.test.ts, covers the non-date half against real drivers — the issue's point that this was never date-specific — for bothdriver-sqlanddriver-sqlite-wasm, both groupBy shapes.Verification
null,内存兜底给"(null)"(不限日期分桶) #3839 signature:(null): sql=— in-memory=1, ‹empty bucket›: sql=1 in-memory=—. Restored and green again.'(null)', and as'null').pnpm buildclean with no generated-artifact drift;pnpm lintclean;check:nul-bytes/check:doc-authoring/check:role-word/check:release-notesall OK.Note for reviewers
The in-memory path also
String()-coerces every non-null group value, so a numeric column's bucket keys come back as strings there and as numbers from SQL. That is a second, separate divergence in the same function with a much wider blast radius, and it is not touched here. Worth its own issue if you agree.🤖 Generated with Claude Code