Skip to content

fix(driver-memory): count a boolean aggregand as 1/0 in avg and sum - #11153

Merged
os-zhuang merged 2 commits into
mainfrom
claude/issue-11065-avg-boolean-memory-driver
Aug 22, 2026
Merged

fix(driver-memory): count a boolean aggregand as 1/0 in avg and sum#11153
os-zhuang merged 2 commits into
mainfrom
claude/issue-11065-avg-boolean-memory-driver

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Fixes#11065

avg over a boolean column returned null on driver-memory while SQLite answered the arithmetically correct rate over the same rows. sum over the same column returned 0. Both are aligned here, on both of this package's faces.

The three implementations of "average a column", and which one was wrong

implementationexpressionavg over a boolean column
SQLite (SqliteWasmDriver)AVG(col)0.4 / 0.25 — the reported, arithmetically correct numbers
packages/objectql/src/in-memory-aggregation.tsvalues.filter(v => v != null).map(toNumber)already correcttoNumber is Number(v) gated on Number.isFinite, and Number(true) === 1
packages/drivers/driver-memory/src/memory-driver.tsvalues.filter(v => typeof v === 'number')null — every boolean is dropped, nums.length === 0, the arm returns null

So this is not a new convention: it is an alignment to the two implementations that already agreed. A refusal instead of a value was considered and is foreclosed — it would have to break in-memory-aggregation.ts too, which is a contract change needing its own ruling.

Which face the repro reaches — traced, not grepped

driver-memory has two aggregate faces, and only a trace says which one the report's repro lands on. Instrumenting driver.aggregate and driver.find on a live ObjectQL.create + InMemoryDriver instance over the reported dataset:

FACES REACHED: [
{ "face": "driver.find", "arm": "engine in-memory fallback -> objectql applyInMemoryAggregation" },
{ "face": "driver.aggregate", "arm": "AST -> performAggregation -> computeAggregate" },
{ "face": "driver.aggregate", "arm": "AST -> performAggregation -> computeAggregate" }
]
[memory] unfiltered [{"avg_sla_violated":null,"row_count":5}]
[memory] closed-filter [{"avg_sla_violated":null,"row_count":4,"sum_sla_violated":0}]

Both aggregate calls arrive as a non-arrayDriverQuery, which is the AST arm — performAggregationcomputeAggregate. That is engine.aggregate pushing the aggregate down to the driver: it prefers driver.aggregate whenever the driver has the method and the query needs no in-memory bucketing (no structured date granularity, no non-UTC timezone, no per-aggregation filter), all true here. The single driver.find call is the insert path, not an aggregate. The reproduction is exact, including sum_sla_violated: 0.

The second face carried the same divergence, independently

memory-analytics.ts's buildAggregator emitted a bare mingo $avg, and mingo ignores a non-numeric value exactly as MongoDB does. Measured directly against mingo 7.2.4 over five booleans:

mingo $avg/$sum over booleans: [{"_id":null,"avg":null,"sum":0,"cnt":5}]
mingo numeric control: [{"_id":null,"avg":0.6,"sum":3}]

So it is in scope and fixed too. Aligning the data face alone would have left this one free to keep its own answer — the shape #6814 recorded on count_distinct.

The analytics face carries the rule as a $group expression rather than as post-processing, deliberately: the count_distinct neighbour's post-processing step runs after the pipeline's own $sort and $limit stages, so a sum or average left as a collected array until then would be sorted as an array, and order over a sum/avg measure is an ordinary analytics query.

The sum decision

sum is coerced alongside avg. It shares the data face's arm, and SUM(bool) is "how many true" on every SQL face and in the objectql fallback — 2 over these rows, where driver-memory answered 0. Fixing only the function the report named would have left the identical defect alive one function over, in a form that is harder to notice: 0 is a plausible number where null is a visible hole.

The coercion is boolean-only

null, a missing key and a non-numeric string reach the accumulators unchanged and stay excluded. Adopting the wider half of toNumber — which maps a non-numeric string to 0 — would average garbage as zero, a separate question this PR does not open. Measured on the analytics face: a numeric column carrying a null, a string and a missing key answers identically with and without the wrapper ({avg: 15, sum: 30} both ways). A regression row pins the exclusion on both faces so the wider behaviour cannot arrive by accident.

Reverse verification

Both coercions reverted to origin/main's expressions, with the mutation confirmed on disk by marker counts in both directions (fixed markers 1/3 → 0/0; reverted markers 0 → 1/1) and restored by an EXIT INT TERM trap.

Predicted before running: 10 of the 15 rows fail, every one on a value rather than a throw, because both faces drop aggregands silently. Measured: 11 failed / 4 passed — the direction held exactly (expected null to be 0.4, expected [ +0, +0 ] to deeply equal [ 2, 1 ], expected 'object' to be 'number'; not one throw), the predicted count was off by one. Both numbers are recorded in the test's docblock rather than the prediction being rewritten to match.

No build stands between the pin and the mutation: the test imports the driver by relative path, so vitest runs src. That is measured, not assumed — the package's dist/ was built from origin/main before the fix and contains neither coercion, yet the unmutated run is green, which it could not be if the assertions were reading dist.

Verification

Union re-run at final commit 09c4fc44e (working tree clean, so the tree measured is that commit's tree):

  • pnpm --filter @objectstack/driver-memory testTest Files 27 passed (27) / Tests 779 passed (779). The new file alone, verbose: Test Files 1 passed (1) / Tests 15 passed (15).
  • pnpm --filter @objectstack/driver-memory typecheck — clean.
  • pnpm lint (eslint . --no-inline-config, whole repo) — exit 0, no findings. Not narrowed.
  • Gates derived with node scripts/pm/dispatch-gates.mjs from the real change set, all green: check:changeset-gate-self-tests, check:driver-conformance (OK — 45 covered cell(s), 0 in the DEBT ledger, 0 exempt), check:objectui-changeset, check:slot-lookup, check:test-source-alias, check:type-source-resolution, check-adr-0087-registration, check-changeset-no-major (✓ This diff introduces no major bump), check-ci-filter-parity, check-empty-changeset, check-plugin-teardown-shape, check-affected-docs, check:nul-bytes.
  • Convention-triggered by the new test file, all green: check:query-options-erasure, check:type-check-coverage, check:engine-double-contract (OK — 381 pinned), check:where-matcher, and check:type-check-debt on a fully built workspace closure (--re-measure: OK — 33 ledger entr(ies) re-measured, none above its recorded number).

Existing assertions

Nothing in the repo pinned the old null. No existing assertion changed answer — the full package suite was green before and after. The reason the false-green could persist is structural and is reported separately below: the shared cross-driver fixture AGGREGATION_ROWS carries no boolean column at all, so the aggregation-conformance family cannot see this cell on any driver.

Scope

driver-memory is under the 2026-08-05 investment freeze (#5499); this rides that ruling's explicit restore-invariant exception and stays inside it. Only the avg/sum aggregand rule on the two faces changed. packages/objectql was read as the reference and not edited, not imported from, and not refactored; packages/spec untouched.

Out-of-scope findings — filed as their own cards, not addressed in this PR


Generated by Claude Code

`avg` over a boolean column returned `null` on driver-memory while SQLite
answered the arithmetically correct rate over the same rows, and objectql's
in-memory fallback answered that same rate — driver-memory was the lone
outlier of the three. Both of this package's faces carried the divergence
independently: the data face filtered aggregands with `typeof v === 'number'`,
and the analytics face emitted a bare mingo `$avg`, which ignores non-numeric
values as MongoDB's does.
`sum` shares the data face's arm and had the same defect one function over
(`SUM(bool)` is "how many true" on every SQL face), so it is aligned with
`avg` rather than left behind.
The coercion is boolean-only: null, missing and non-numeric strings reach the
accumulators unchanged and stay excluded, rather than being folded to 0.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RfyXxZ2WPjcjhuXpiQQc3y
…ngeset
The ablation was run after the fix was committed so it had a restore point:
both coercions reverted to origin/main's expressions, the mutation confirmed
on disk by marker counts in both directions, then restored by a trap.
Measured 11 failed / 4 passed against a predicted 10 — the direction held
exactly (every failure on a value, none on a throw) and the predicted count
was off by one. Both numbers are recorded rather than the prediction being
quietly rewritten to match.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RfyXxZ2WPjcjhuXpiQQc3y
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

3 anchor(s) derived from 1 changed package(s); no hand-written page names any of them. ✅

What this run could not see
  • the SDK route bridge reached 45 of 221 client-bound route-ledger rows — the other 176 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run: node scripts/docs-audit/affected-docs.mjs --bridge-coverage

Coarse fallback — 8 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 17bad125cca89674cfd2dca59025b0d31da68968packageMentionDocs.

Which tree this was computed on

This run read content/docs from 71481b0838259ab3c54ad214e6ef5aba40591b81 — the merge of head 09c4fc44ebe6b0d3e1b7160a96a3f2f242044137 into base 17bad125cca89674cfd2dca59025b0d31da68968, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 71481b0838259ab3c54ad214e6ef5aba40591b81 && git checkout 71481b0838259ab3c54ad214e6ef5aba40591b81
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 17bad125cca89674cfd2dca59025b0d31da68968 09c4fc44ebe6b0d3e1b7160a96a3f2f242044137 && git checkout -B drift-repro 17bad125cca89674cfd2dca59025b0d31da68968 && git merge --no-ff 09c4fc44ebe6b0d3e1b7160a96a3f2f242044137
node scripts/docs-audit/affected-docs.mjs --json 17bad125cca89674cfd2dca59025b0d31da68968

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

@github-actions

Copy link
Copy Markdown
Contributor

⛔ merge queue 构建失败 — 先分诊,再决定要不要重排

队列构建 32602076972 红了。队列跑的是全量套件(PR 侧 CI 只跑 affected 子集),
所以失败的测试可能在本 PR 没碰过的包里 —— 那不是重排能修的。每次盲目重排都会让排在后面的所有 PR 重建一轮。

失败的 job(日志抽取,best effort):

  • Console Pin Gate — 失败步骤: Build the Console SPA at the pinned objectui SHA

    ✗ Build failed in 4.28s
    

↳ 失败原因 是判读的关键:超时Test timed out in … / Hook timed out in …)多半是负载/时序,不是本 PR 的回归;
断言AssertionError: …)才指向真实的行为改变。两者的 FAIL 行长得一模一样,只有这一行能区分。

跨 PR 相同签名(24h,按失败测试文件聚合):

  • ⚠️本次没有可用的聚合签名(日志里没有能解析出测试文件名的 FAIL 行)—— 这不是「没有同签名的其他 PR」,是这一轮没测到。跨 PR 聚合本次不可用,请手工比对其他 PR 的同类评论。
  • ⚠️ 24h 评论账本没读完(超过 5 页仍未读到窗口尽头),所以上面的「不同 PR 数」是下界,不是全量。

历史信号:

  • 本 PR 过去 24h 无队列失败记录(首次)。
  • 过去 24h 队列共有 103 个失败构建(不含本次)。

分诊清单:

  1. 失败测试在本 PR 改动的包里 → 真回归,修 PR。
  2. 失败测试与本 PR 无关 → 看上面的「跨 PR 相同签名」;已有汇总 issue ⇒ flaky/环境问题实锤,去那张 issue 上谈,修好前重排只会再烧一轮全队列。
  3. 两者都不是 → 可能与同组 PR 语义冲突;等前面的 PR 落地或失败出队后再重排一次即可,不要连续重排。

Generated by Claude Code · merge-queue-triage workflow (#4859)

Merged via the queue into main with commit 2095040Aug 22, 2026
32 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-11065-avg-boolean-memory-driver branch August 22, 2026 22:33
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/mteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

avg over a boolean field returns null on driver-memory and a number on sqlite — same rows, same dataset, same executor

2 participants

@os-zhuang@claude