Skip to content

[finding][drivers] GroupByNode.alias 只有内存分桶认,三个 SQL 面一律忽略 —— 同一个 aggregate 走下推与走回退拿到不同的列键 #6401

Closed

Description

@os-zhuang

在 #6212 批 B(aggregate 收窄)实施期间实测发现,记录备查。观察类,不挂 pm:queue,请分诊轮定级。

事实

GroupByNodeSchema 声明的结构化条目有三个键:field / dateGranularity? / alias?,其中 alias 的 describe 是 "Alias for the projected group value"(默认回落到 field)。

读它的只有内存分桶那一条路:

位置 是否读 alias
packages/objectql/src/in-memory-aggregation.ts:89 ✅ const fieldName = typeof g === 'string' ? g : (g.alias ?? g.field);
packages/drivers/driver-sql/src/sql-driver.ts aggregate() 的 groupBy 循环 ❌ 两条分支都只用 g.field(带 granularity 的按 g.field 起别名;不带的 groupBy(g.field) + select(g.field))
packages/drivers/driver-sqlite-wasm ❌ 整个继承上面那份
packages/drivers/driver-turso 本地 / 远端两面 ❌ 同上(远端面在 #6212 批 B 之后开始编结构化条目,也刻意不读 alias,以免只在一面读而造出新分叉)

后果

同一份 groupBy: [{ field: 'closed_at', dateGranularity: 'month', alias: 'qtr' }]:

  • 引擎判定可下推(驱动发布了该 granularity)⇒ 走 driver.aggregate ⇒ 结果行的列键是 closed_at;
  • 引擎判定不可下推(能力位缺该 granularity、或 timezone 非 UTC 强制走内存)⇒ 走 find() + applyInMemoryAggregation ⇒ 列键是 qtr。

也就是说同一条查询的结果列名取决于引擎当次选了哪条路,而选路依据是驱动能力位与时区 —— 调用方看不见的东西。packages/objectql/src/in-memory-aggregation.test.ts:45 正是拿 alias: 'qtr' 写的,所以内存那一半有覆盖,下推那一半没有,两半也从没被放在一起比过。

这是 ADR-0049 的形状

alias 是已声明、部分执行的键:写了它,一半的执行路径认,另一半静默忽略。enforce-or-remove 两条路都摆着,且分别有代价:

  • enforce:三个 SQL 面都读 alias(SELECT … AS "qtr",GROUP BY 仍按 field)。要顺带确认 presentReadColumns 的 presentedOutput 映射改按 alias 记(sql-driver.ts 里那张 map 现在以列名为键),以及 having 的 post-filter(它按「聚合别名 + groupBy 投影」判定)跟着改口径。
  • remove:删 GroupByNodeSchema.alias(走 ADR-0087 墓碑 + 转换层),代价是内存那一半现有行为要改,且要先量一量非测试书写者。

⚠️ 我没有量过 alias 的真实生产者(showcase / CRM / 图表配置面是否有人写),定级前需要补这一量 —— 这正是 #5021「能力扩张要有真实业务牵引」那条轴要问的。

与 #6212 批 B 的关系

批 B 只做了「不加剧」:远端 turso 开始编结构化 groupBy 条目时,刻意不读 alias,与 SqlDriver.aggregate 保持一致 —— 只在一面读会是新分叉而不是修复。本分歧既非批 B 造成、也不在批 B 范围内,故单独记账。

关联:#6212、ADR-0049。

Activity

  1. os-project-manager commented on Aug 8, 2026

    @os-project-manager
    Collaborator

    Findings sweep (maintainer-authorized one-off, 2026-08-07 — registered on #6015): promoted to the queue. A declared key honored by one of four faces means the same aggregate returns different column keys depending on pushdown vs fallback — the multi-face consistency invariant (same rows/keys or loud refusal, never a third quiet answer) applies verbatim. Batch candidate with #6212/#6231/#6402; frozen drivers get mechanical alignment only. finding → pm:queue.


    Generated by Claude Code

  2. self-assigned this
    on Aug 8, 2026
  3. os-zhuang commented on Aug 8, 2026

    @os-zhuang
    ContributorAuthor

    Claim: PM loop, drivers lane (serial sql-driver.ts chain, position 3 of 3) — dispatched under the maintainer's standing authorization of 2026-08-08 ~17:15Z:「6577 落地后继续按序派发,不用等我确认」.
    Session: session_01Hg9Pkg5nDedCRihRsdeCdX
    Branch: claude/issue-6401-groupby-alias-multi-face
    Worktree: objectstack-issue-6401 (cloud session — own container, Opus)
    Domain: domain:drivers

    Serial constraint cleared. PR #6816 (issue #6409) MERGED as e120a5a — it was the last in-flight change to sql-driver.ts. Race check at dispatch time: no other open PR claims this issue, and no open PR touches driver-sql / driver-turso / driver-sqlite-wasm / in-memory-aggregation.

    Premise re-verified on the post-#6409 main, not taken from the issue. Line anchors moved; the fact did not. SqlDriver.aggregate's structured groupBy branch now sits at sql-driver.ts:4118, and both limbs still read g.field only — builder.groupBy(g.field) / builder.select(g.field) at :4134–:4135, and the date-bucket limb aliasing to g.field at :4132. presentedOutput.set(g.field, kind) at :4137 is the presentation map the issue predicts must re-key if the enforce leg is taken.

    Re-priced against the #6409 diff — this card got CHEAPER, and the reason is specific. #6409 landed packages/spec/src/data/aggregation-conformance.ts (AGGREGATION_ROWS + AGGREGATION_CASES) plus three face suites (driver-sql on better-sqlite3, driver-turso REMOTE on the libsql stub, driver-sqlite-wasm on the sql.js dialect) and a check:driver-conformance column that forces a verdict for all five drivers. That is precisely the instrument this issue needs: the alias divergence is a multi-face disagreement whose only honest measurement is the faces pinned against each other on values. The harness exists now; this card should extend it rather than build a second one.

    Two consequences worth stating up front, so they are decided rather than discovered:

    ⚠️ This is not a pure implementation card, and it is dispatched as such. The issue states plainly that the real producers of GroupByNode.alias were never measured, and that grading needs that measurement first (the #5021 axis: capability expansion needs real business pull). ADR-0049's two legs both carry cost — enforce means three SQL faces read alias plus the presentedOutput map and the having post-filter change口径; remove means an ADR-0087 tombstone plus changing the in-memory half's current behaviour. The dispatch therefore requires the producer measurement as step one and, if the measurement does not clearly indicate a leg, a needs-user-decision filing with both legs argued and no recommendation — not a guess.


    Generated by Claude Code

  4. claude commented on Aug 9, 2026

    @claude
    Contributor

    os-dev report

    premise_still_valid: YES — re-verified on e120a5a, not taken from the issue. SqlDriver.aggregate's structured groupBy branch read g.field in both limbs (builder.groupBy/select in the plain limb, as ?? bound to g.field in the date-bucket limb), and presentedOutput.set(g.field, kind) keyed the presentation map by the field. driver-turso's remote transport flattened the union to string[] with an explicit comment saying alias was deliberately not read. The engine fork the issue names is at engine.ts — allStructuredSupported && !tzRequiresInMemory.

    The issue's table undercounts the honouring side. Two consumers it does not list also read alias, and both matter to the ruling:

    Face reads alias? where
    objectql in-memory fallback ✅ in-memory-aggregation.ts — g.alias ?? g.field
    driver-memory ✅ memory-driver.ts:1066-1068 normalizeGroupBy
    chart category resolver ✅ chart-aggregate.ts:105 chartAggregateCategoryKey
    driver-sql / driver-turso (both) / driver-sqlite-wasm ❌ —
    driver-mongodb ❌ cannot take a structured node at all

    The measurement, and the leg it drove

    Step one was the measurement the issue asked for, run structurally (balanced-bracket scan of every groupBy: region across git ls-files, ts/tsx/js/mjs/json/md/mdx/yml), not by impression.

    Real non-test producers of GroupByNode.alias: ZERO.

    • examples/ — 0. All 7 files with groupBy were read: the showcase's ObjectChart uses groupBy: 'status' (bare string), and CRM/showcase groupByField is the kanban prop, a different key.
    • apps/, non-test packages/, scripts/ — 0.
    • Tests — 4, counted separately: in-memory-aggregation.test.ts:45 (qtr), chart-aggregate.test.ts:60 (quarter), remote-transport-groupby-node.test.ts:185 (bucket, pinning that remote IGNORES it), validate-react-page-props.test.ts:961 (degenerate, alias === field).
    • Docs teaching it — 1: skills/objectstack-query/rules/aggregation.md:86, two lines above the promise "results are correct either way" across the pushdown/fallback fork — which was false for keys.
    • Declaring surfaces exposing it to outside producers — 4: query.zod.ts, chart.zod.ts (ChartGroupBySchema.alias, described as "this becomes the category column"), mcp-http-tools.ts:419 (LLM-facing tool schema), and the chart strictObject alias map that actively steers as / label → alias.

    Zero producers reads like "remove". It is not, and the deciding fact is a gate.

    ADR-0049's decision table does not split on producer count — it splits on whether the feature already exists: a dangling promise is removed or marked, a live feature with a missing gate is enforced. alias is live on three consumers, and decisively it is compelled by the publish gate: validate-react-page-props.ts:491 computes chartAggregateResultKeys and errors REACT_CHART_AXIS_UNKNOWN unless a chart's category axis names alias ?? field, with the remedy "The category axis is keyed by groupBy — bind it to <alias>". A key the build gate makes you write is not a dangling promise. (Its own error text meanwhile says rows are "keyed by the RAW FIELD NAMES" — the gate and the SQL faces contradicted each other in the same message.)

    So the zero-producer count is what makes enforcing safe — no shipped payload changes its result keys — rather than what argues against it. Under ADR-0049 the "remove" leg was also materially more expensive than the issue priced it: it would have to retract ChartGroupBySchema.alias, chartAggregateCategoryKey's alias limb, the lint gate's category computation, the as/label guidance entries and the MCP tool key — a shipped, gate-enforced UI capability — not merely "change the in-memory half's behaviour". No needs-user-decision filing was needed; the measurement indicated the leg.

    Delivered, per face

    PR #6849 (draft) — every SQL face in one change, since a fix landing on one and not its twin is the #6203 shape and TursoDriver picks its face from url:

    • driver-sql — both limbs project alias ?? field; ?? as ?? only when the name actually moves (a degenerate alias emits no self-rename). presentedOutput re-keyed by the OUTPUT column, matching the aggregation branch beside it — an aliased group value went unpresented before, which the issue predicted.
    • driver-turso REMOTE — "field" AS "alias"; the alias reaches the statement as a quoted identifier and is therefore held to assertSafeIdentifier, like field.
    • driver-sqlite-wasm — inherits SqlDriver; covered by its own suite rather than by assumption.
    • GROUP BY still keys on the FIELD everywhere, pinned explicitly: SQLite resolves output names in GROUP BY, so a face that grouped by the alias would look correct on SQLite and diverge elsewhere.
    • having needed no code change and now means one thing: applyHaving filters on the aggregated row's own keys, so it follows the projection instead of meaning the alias on one path and the field on the other.

    Conformance — AGGREGATION_CASES gains a groupByAlias axis and 2 cases whose VALUES are an existing case verbatim, so they can fail only on the key. objectql's in-memory fallback is enrolled as a fourth face, answering #6409's open question ②: it is the face the SQL three were converged onto, and enrolling it turned out to be cheap because applyInMemoryAggregation is a pure function of rows and an AST — no engine needed.

    Reverse verification — predictions written down BEFORE measuring

    Direction Predicted Measured
    A — the pre-fix state 1 fail per SQL face on the named case; degenerate alias === field green ✅ driver-sql 1/18, driver-turso 3/34 (the 3 named), sqlite-wasm 1/15, objectql 0
    B — group by the alias (copied neighbour) values blind; only the emitted-SQL pin catches it ✅ driver-sql 18/18 green, wasm 15/15 green; turso alone fails on GROUP BY "bucket"
    C — harness reads c.groupBy all green (a false GREEN) ❌ prediction WRONG — 1 fail per face

    Two results I did not predict, recorded rather than tidied away:

    1. (C) was wrong, and the correction is the useful part. Reading the FIELD name against a face that honours the alias fails loudly (r['region'] is undefined → group: 'undefined'). The false green needs both mistakes at once — so I measured that directly: pre-fix face + harness reading the field name = 18/18 GREEN on driver-sql. The groupByAlias ?? groupBy read is load-bearing exactly where predicted, but for the opposite reason: not a silent no-op on a correct face, but one half of a pair that only goes quiet together.
    2. driver-sqlite-wasm's suite consumes driver-sql's built dist, not its src. Direction A showed a spurious 15/15 green there until driver-sql was rebuilt. That suite verifies its inheritance of the compiler at BUILD granularity, not working-tree granularity — worth knowing for anyone reverting against it.

    Both are written into the new suite's header, not just here.

    Gates and tests — CI judged on the JOB log

    • Local gate list: 59/59 green, enumerated from .github/workflows/lint.yml and run step by step with exit statuses read directly (never through a pipe). ESLint job 33/33, TypeScript Type Check job 26/26. That deliberately includes the steps that are not check:* scripts — the omission that took drivers: implement count_distinct in the SQL family — enforcement half of the #6188 ruling #6409 red: turbo run build --filter='./packages/*' --filter='./examples/*^...', turbo run typecheck --filter='./packages/*' --filter='./packages/*/*' --filter='./apps/*', turbo run build --filter='./packages/*' --filter='./packages/*/*', pnpm --filter './examples/*' run typecheck, and downstream-contract typecheck. The workspace was built first, so check:i18n / check:i18n-coverage ran for real rather than reporting PREREQUISITE NOT MET.
    • Suites: objectql 2685, driver-sql 1097, driver-turso 906, driver-sqlite-wasm 304, spec 8877, lint 1730, driver-memory 540, driver-mongodb 220 — all green.
    • CI on PR feat(drivers,spec)!: GroupByNode.alias is honoured by the three SQL faces (#6401) #6849, read from the check-runs API on the head SHA 71842f4: 23 success, 2 skipped, 0 failures. Not my local run — and it caught something local runs did not: the first push went red on Check Changeset, step "Require an ADR-0087 disposition on a declared-breaking changeset" ([finding] ADR-0087 台账没有「完备性」门禁:已发生的退役漏登记时全仓全绿,只有人工能发现(#6011 即如此) #6148). The changeset carries a bang, so the gate requires the ledger question to be answered in writing. Answered not-required (no-migration-prescription): nothing is retired — alias keeps its declaration and starts being honoured — so there is no tombstone and no authored metadata a codemod could rewrite. Green on re-run.

    Frozen drivers (#5499) — measured, never flipped

    • driver-memory: agrees. normalizeGroupBy already returned { field, alias: node.alias ?? node.field } and projected under the alias. It had reached the enforce answer independently, so the leg required no mechanical alignment. Its AGGREGATION_CASES cell stays open on count_distinct alone.
    • driver-mongodb: DEBT, and wider than alias. buildAggregationPipeline annotates groupBy as string[] and builds groupId[field] = '$' + field (mongodb-aggregation.ts:66-69, mirrored in the $project at :85-88). A structured node — aliased or not — is an object there, so the $group._id key becomes the literal "[object Object]" and its value "$[object Object]". The alias is unreachable, not ignored. mongodb-driver.ts:512 passes (query as any).groupBy, which is why tsc never saw it. Read from source; not executed.

    Both DEBT rows in scripts/check-driver-conformance.mjs were re-measured and extended for the new axis, so check:driver-conformance carries a verdict rather than an omission.

    Out-of-scope findings — filed unassigned (deduped first)

    Open questions — recorded, not blocking

    1. [spec/drivers] AggregationNode.distinct is honoured by the in-memory fallback and ignored by every SQL face — one query, two numbers (ADR-0049) #6815 is this card's exact sibling and is still needs-user-decision, unassigned: AggregationNode.distinct is honoured by the in-memory fallback and ignored by every SQL face. My ADR-0049 reasoning does not transfer unexamined — the deciding fact here was a publish gate compelling the key, and distinct has no such gate that I found. Whoever takes [spec/drivers] AggregationNode.distinct is honoured by the in-memory fallback and ignored by every SQL face — one query, two numbers (ADR-0049) #6815 should re-run the measurement rather than inherit this verdict.
    2. The lint gate's error text ("Object-bound aggregate rows are keyed by the RAW FIELD NAMES") is now loose whenever an alias is present. Its computed remedy is correct — only the explanatory sentence is stale. Left alone to keep this PR to the drivers; noting it so it is a decision rather than an oversight.
    3. chartAggregateCategoryKey reads only the SINGLE-object groupBy form; an array-valued chart groupBy returns undefined. Out of scope here and not obviously wrong (the chart protocol takes one category), but it is the reason the chart face never disagreed on multi-key groups.

    PR left as a draft for the step-7 gate review.


    Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions