Skip to content

fix(service-analytics): compareTo 带上 measure 自己的 filter,__compare 列不再是另一个 measure (#4820) - #4870

Merged
os-zhuang merged 2 commits into
mainfrom
claude/issue-4820-compare-measure-filters
Aug 3, 2026
Merged

fix(service-analytics): compareTo 带上 measure 自己的 filter,__compare 列不再是另一个 measure (#4820)#4870
os-zhuang merged 2 commits into
mainfrom
claude/issue-4820-compare-measure-filters

Conversation

@claude

@claude claude Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Fixes #4820

问题

filter 的 measure,在当前期是靠一条补充分组子查询(combineFilters(baseFilter, compiled.measureFilters[m]))来限定范围的;但 compareTo 那条路径只发一条涵盖全部 base measure 的位移查询,where 里只有 baseFilter —— compiled.measureFilters 在那条路径上从未被查阅过。

于是同一个 measure,当前期这一列被限定了,对比列没有:

# measures where 窗口
1 revenue 当前期
2 won_count {"stage":"closed_won"} 当前期
3 revenue, won_count 位移

won_count__compare 数的是上个窗口的全部商机,被这个 measure 本来要排除的行精确地放大了。误差只往一个方向走:对比期永远显得更好,所以「本月赢单 vs 上月」的卡片会在什么都没变坏的时候读作暴跌。只有带 filter 的 measure 错,旁边不带 filter 的照常正确 —— 这正是它能一直活着的原因。

修法

没有在 compare 路径上再写一遍「怎么应用 measure filter」。两条路径现在走同一个 runMeasurePass:不带 filter 的 measure 一条查询,带 filter 的每个一条子查询,按维度键合并;splitMeasuresByFilter 是「这个 measure 有没有自己的 filter」的唯一答案。两个窗口之间唯一的差别,就是位移过的 dateRange

长出第二套规则正是这类缺陷复发的地方 —— 它们下一次改动就会再次分叉。所以抽的是公共函数,不是补丁。

数据集 filter、presentation 的 runtimeFilter、measure 自己的 filter,在两个窗口里以完全相同的方式组合。

代价:设了 compareTo 时,每个带 filter 的 measure 多一条查询。measure 都不带 filter 的 selection 完全不受影响,仍然只发一条位移查询(测试钉住了这一条)。

#4708 的接缝:compare 路径现在也会扇出,所以它的补充子查询同样可能整组缺失。fillEmptyGroups 已经覆盖 __compare 列,上个窗口被 filter 清空的分组读作 0(count/sum)而不是空白 —— 和当前期的行为一致,已加测试。

测试

测试的假 service 不是按查询形状返回硬编码数字,而是一个小的内存库:它真的对种子行求值 wheredateRange,真的分组。所以丢掉 measure filter,数字会变 —— 这是本 PR 唯一值得钉的东西,查询形状证明不了用户看到的数是对的。

种子刻意让 revenue 在两个窗口都是 300(3×100 = 100 + 4×50),于是能让 won_count__compare 动的只剩「filter 有没有到位移查询」。

改动前后跑同一组用例:

用例 修复前 修复后
位移查询条数 1 2
位移的 won_count 查询 where 不含 closed_won { stage: 'closed_won' }
won_count__compare 5 1
方向(本月 3 赢单) 3 vs 5 = 暴跌 3 vs 1 = 增长

修复前的实际报错(把 fix 临时回退成原来那条扁平查询跑出来的):

AssertionError: expected [ { cube: 'pipeline', …(4) } ] to have a length of 2 but got 1
AssertionError: expected 5 to be 1 // Object.is equality
AssertionError: expected 3 to be greater than 5
AssertionError: expected '{"$and":[{"is_deleted":{"$ne":true}},…' to contain '"stage":"closed_won"'

命令与结果:

npx vitest run --maxWorkers=2                  # 38 files / 511 tests passed
npx tsc --noEmit -p .../service-analytics      # 7 errors,与 main 基线逐条相同(全在我未触碰的既有测试文件里)
                                               # 新增/改动的两个文件 0 error
npx eslint (两个改动文件)                        # 无输出

changeset 等级:patch

splitMeasuresByFilter 没有加进 src/index.ts,包的公开入口面一字未变,可观察到的变化就是 __compare 列的数字被改对了。作为对照,#4708 的 PR #4822minor,因为它确实新增了公开面(fillEmptyGroups 进了 index、emptyGroupValueFor 进了 spec);本 PR 没有对应的东西。

范围

packages/spec/** 零改动;未触碰 packages/lintskills/**content/docs/**content/docs/releases/

⚠️ #4821(mergeByDimensions 用无分隔符拼接维度键)就在本 PR 改的同一个文件里,本 PR 刻意不碰,留给它自己那一单。

🤖 Generated with Claude Code

https://claude.ai/code/session_015Br2xsJsczFsTR9bvbh2Ny


Generated by Claude Code

…<measure>__compare is the same measure as its neighbour (#4820)

A measure declared with its own `filter` was scoped by a supplementary grouped
sub-query in the current period, but the `compareTo` pass issued ONE shifted
query over every base measure with only the base filter — `measureFilters` was
never read on that path. `won_count` counted won deals while `won_count__compare`
counted every deal, side by side under one label, biased so the comparison
window always looks better.

Both windows now run the same `runMeasurePass`: unfiltered measures in one
query plus one sub-query per filter-scoped measure, merged by dimension key.
The split lives in one exported `splitMeasuresByFilter` so the two paths cannot
re-diverge; the only difference between them is the shifted dateRange.

Tests build a fake service that really evaluates `where` and groups seed rows,
so the pinned number changes with the fix (won_count__compare 5 -> 1), not just
the query shape. Selections whose measures carry no filter still compare in a
single shifted query.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015Br2xsJsczFsTR9bvbh2Ny
@vercel

vercel Bot commented Aug 3, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
objectstack Ignored Ignored Aug 3, 2026 11:34am

Request Review

@github-actions github-actions Bot added documentation Improvements or additions to documentation tests tooling size/m labels Aug 3, 2026
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/service-analytics.

8 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/api/data-api.mdx (via @objectstack/service-analytics)
  • content/docs/api/index.mdx (via @objectstack/service-analytics)
  • content/docs/kernel/services-checklist.mdx (via @objectstack/service-analytics)
  • content/docs/permissions/sharing-rules.mdx (via @objectstack/service-analytics)
  • content/docs/plugins/packages.mdx (via @objectstack/service-analytics)
  • content/docs/releases/implementation-status.mdx (via @objectstack/service-analytics)
  • content/docs/releases/v17.mdx (via @objectstack/service-analytics)
  • content/docs/releases/v9.mdx (via @objectstack/service-analytics)

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

…w NUL byte

`check:nul-bytes` (run inside the ESLint job) rejected a literal 0x00 written
into the fake service's group key. A raw NUL makes grep/ripgrep treat the whole
file as binary and return zero matches, so the file drops out of code search
and every grep-based lint; git only scans the first 8000 bytes, so at offset
5440 it still diffed as text.

Keyed with `JSON.stringify(...)` joined by `|` instead, which needs no exotic
byte at all and is unambiguous for the multi-dimension case. Deliberately NOT
the production `mergeByDimensions` key, whose delimiter-free concatenation is
#4821 — the fake must not import a second defect into a test for this one.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015Br2xsJsczFsTR9bvbh2Ny
@github-actions github-actions Bot added size/l and removed size/m labels Aug 3, 2026
@os-zhuang
os-zhuang marked this pull request as ready for review August 3, 2026 11:39
@os-zhuang
os-zhuang added this pull request to the merge queue Aug 3, 2026

Copy link
Copy Markdown
Contributor

复核通过 —— ACCEPT,已标 ready 并送合并队列

1. 没有长出第二套规则 —— 这是本单最重要的要求

我派发时写的是:「如果两条路径各自实现一遍『怎么应用 measure filter』,下一个缺陷就在它们再次分叉的地方。」

实际做法是抽出 runMeasurePass(不带 filter 的 measure 一条查询 + 每个带 filter 的 measure 一条补充子查询,按维度键合并)与 splitMeasuresByFilter,executeSelectionrunCompare 都调用它。两条路径从此共用一份语义,不会再次分叉。

2. 假 service 真的在求值 —— 这是「数值可区分」要求的正确做法

假 service 不是按查询形状返回硬编码值,而是真的对种子行求值 wheredateRange 并分组,所以丢掉 measure filter 数字会变。

而且种子设计得很巧:让 revenue 在两个窗口都等于 300(3×100 = 100+4×50),唯一能动 won_count__compare 的就只剩 filter 是否到位。这是把变量隔离到只剩被测那一个。

反向验证给出的是用户真正看到的东西:

won_count__compare 修复前 5、修复后 1,方向从「3 vs 5 暴跌」变回「3 vs 1 增长」。

一个「本月赢单 vs 上月」的卡片,修复前显示的是暴跌,实际是增长。断言查询形状证明不了这个,断言数字才行。

无 filter 的 measure 用例修复前后都通过(仍只发 2 条查询),主路径未误伤。

3. 与 #4708 的接缝处理对了

基于 PR #4822 合并后的 main 工作,fillEmptyGroups 的位置未动,现在自然覆盖 compare 路径新长出的扇出接缝 —— 上个窗口被 measure filter 清空的分组读 0 而非空白,并补了测试。两条相邻的 bug 没有互相拆台。

4. NUL 字节

首轮 CI 的 ESLint 红是 check:nul-bytes 抓到测试里一个裸 0x00(不是 lint 规则)。已改键法并作为新提交推送(未 force-push),第二轮 20 checks 全绿。

顺带记一个今天学到的东西:裸 NUL 会让 git 把文件判为二进制,于是 GitHub 上这个测试文件的 diff 显示成 +0/-0 —— 看起来像个空文件。我一开始就是这么误判的,实际是 347 行。

5. changeset patch 正确

splitMeasuresByFilter 刻意不加进 src/index.ts,包的公开入口面未变。与本仓「无新增包根导出面 ⇒ patch」一致(#4822 取 minor 是因为它确实新增了公开行为面,两者不同)。

packages/spec/** 零改动 ✅。


⚠️#4821 的一条警告 —— 它推翻了那个 issue 里「显而易见」的修法

dev 按指示没碰 #4821,但报告里留了一条会改变其修法的发现,我原样转到 #4821 并摘录于此:

issue 建议的 ${d}=${JSON.stringify(row[d] ?? null)} 键法会顺带改掉类型强制语义 —— 现行 String(row[d]) 让数字 1 与字符串 '1' 合并成同一组,而 JSON.stringify 会把它们分开dataset-executor.ts 自己的 compareValues 注释就写明「some drivers return SUM results as numeric strings」,主查询与补充子查询若对同一维度返回不同 JS 类型,换成 JSON.stringify 会让原本能合并的行不再合并 —— 一个静默的新缺陷。

也就是说:#4821 那个看起来最自然的修法,会用一个新的静默缺陷换掉旧的。dev 给的替代方向是保留 String() 的逐段强制、只把拼接改成无歧义形式(如长度前缀 s.length + ':' + s),null'' 的区分另用哨兵值。

同时 #4821 的优先级被本 PR 抬高了:compare 路径现在也会扇出并逐 measure 合并,所以一次键碰撞还会污染 __compare,受影响面比该 issue 正文估计的更大。

一条账本漂移(仅记录)

scripts/check-type-check-coverage.mjs 登记 service-analyticserrors: 3,实测 7。不是回归也不是门禁失败(该包无 typecheck 脚本,条目是冻结的 DEBT 计数,check:type-check-coverage 仍 OK),但那个数字已与实际脱节。与 #4855(qa/dogfood 的 tsconfig 无人跑)同属「DEBT 账本本身在腐烂」这一类。


Generated by Claude Code

Merged via the queue into main with commit 2f05139 Aug 3, 2026
21 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-4820-compare-measure-filters branch August 3, 2026 11:51
akarma-synetal pushed a commit to akarma-synetal/framework that referenced this pull request Aug 4, 2026
…op merging "unassigned" into "blank" (objectstack-ai#4821) (objectstack-ai#4957)

`mergeByDimensions` is the seam every multi-query dataset result is assembled
through: the primary pass against each measure-scoped supplementary pass, and
— since objectstack-ai#4870 — the current window against the shifted `compareTo` window,
which now fans out per measure the same way. A key collision there does not
fail; one group silently absorbs another's numbers.

The reported mechanism was not quite the real one, which is worth recording.
The old key was `String(row[d] ?? '')` joined on a RAW U+0001 byte written
literally into the source. A raw control byte renders as nothing, so objectstack-ai#4821 was
filed reading `join('')`, and its headline repro (`['ab','c']` vs `['a','bc']`
both keying "abc") never actually reproduced — the separator was there, merely
invisible. Two things did:

  - `?? ''` keyed a genuinely NULL dimension the same as an empty-string one,
    so "unassigned" merged into "blank": one row absorbed the other's measure
    and the other's column went absent — which objectstack-ai#4708's empty-group fill then
    turns into a confident 0. A group whose real count is 3 renders as 0.
  - A one-character separator is unambiguous only while no dimension VALUE
    contains it, and dimension values are user data (text fields, imports).

Fixed by length-prefixing each segment (`2:ab1:c` vs `1:a2:bc` differ for every
possible input, no character is reserved, and no invisible byte is left in the
source for the next reader to misread) plus an explicit sentinel for
null/undefined, kept separate from the disambiguation concern.

The per-segment `String()` coercion is deliberately KEPT, and it is not the
trade-off `cross-object-rebucket.ts` makes one file over. That function
re-buckets ONE query's rows, where a column carries one type, so its JSON key
is free and buys a real distinction. This key aligns rows across DIFFERENT
queries, and drivers do type the same group differently across them — this
file's own `compareValues` records it ("numeric strings, which is how some
drivers return SUM results"). A `JSON.stringify` key would render `1` and `'1'`
as two keys and split groups that merge correctly today, trading one silent
defect for a new one. Pinned by a regression test.

Tests: 9 new cases in `dataset-merge-dimension-key.test.ts` — the adjacent-value
pair, a value carrying the old separator, null vs empty string, an absent
dimension column, the numeric-vs-string regression pin, and the same properties
through the real executor on both the measure-filter and `compareTo` merges.
5 of the 9 fail against the pre-fix implementation.

Fixes objectstack-ai#4821


Claude-Session: https://claude.ai/code/session_01NrmBxj8rK2uGCnh9aipjwX

Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/l tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

compareTo drops every measure-scoped filter, so <measure>__compare silently reports a different measure than the column beside it

2 participants