diff --git a/.changeset/merge-dimension-key-unambiguous.md b/.changeset/merge-dimension-key-unambiguous.md new file mode 100644 index 0000000000..fd673eee57 --- /dev/null +++ b/.changeset/merge-dimension-key-unambiguous.md @@ -0,0 +1,37 @@ +--- +"@objectstack/service-analytics": patch +--- + +fix(service-analytics): 维度合并键不再把「未分配」并进「空白」,并改为长度前缀消歧 (#4821) + +`mergeByDimensions` 是每一份多查询 dataset 结果的装配缝:主查询与每个带 `filter` +的 measure 的补充子查询在这里对齐,`compareTo` 窗口自 #4870 起也按 measure 扇出后 +经由同一个缝合并回来。这里一次键碰撞不会报错 —— 一个分组静默吸走另一个分组的数字, +网格仍然保持看起来合理的行数和列数。 + +**#4821 报告的机制与实际的缺陷不完全一致,先把这一点说清楚。** 原键是 +`String(row[d] ?? '')` 以一个**直接写进源码的裸 U+0001 字节**相连。裸控制字符渲染 +为空,所以 issue 正文读到的是 `join('')`,其头号复现(`['ab','c']` 与 `['a','bc']` +同键为 `"abc"`)其实并不成立 —— 分隔符一直在,只是看不见。真正咬人的是另外两条: + +- `?? ''` 让**真正为 null** 的维度与**空字符串**维度键成同一个值。于是「未分配」被 + 并进「空白」:一行吞掉另一行的 measure,另一行的列则整个缺失 —— 而 #4708 的空组 + 填充随后会给它填上一个理直气壮的 `0`。一个真实计数为 3 的分组因此显示为 0。 +- 单字符分隔符只在「没有任何维度**值**包含该字符」时才无歧义。维度值是用户数据 + (文本字段、导入记录),所以那是一个假设而非保证,且一旦不成立同样静默。 + +**改法:长度前缀 + 显式空值哨兵。** 每段编码为 `<长度>:<值>`,`2:ab1:c` 与 +`1:a2:bc` 对任意输入都不同,不再保留任何字符、也不再有看不见的字节留给下一个读者 +误读(本 issue 正是这样被误读出来的)。null/undefined 单独走一个哨兵段,与消歧这件 +事解耦。 + +**逐段的 `String()` 强制被刻意保留**,这与一文件之隔的 `cross-object-rebucket.ts` +的 JSON 键不是同一笔交易:后者重新分桶的是**同一个查询**的行,一列只有一种类型, +JSON 在那里免费且能换来真实的区分(空桶 `null` vs 字面量字符串 `"null"`)。本函数 +做的是相反的事 —— 跨**不同查询**对齐行,而驱动确实会对同一个分组返回不同的 JS 类型 +(本文件 `compareValues` 的注释即记着 "numeric strings, which is how some drivers +return SUM results")。改用 `JSON.stringify` 会把 `1` 与 `"1"` 渲染成两个键,让今天 +能正确合并的行不再合并 —— 用一个新的静默缺陷换掉旧的,不算修好。该行为已有回归钉 +测试锁住。 + +仅影响内部合并键,响应中的任何值都不改变。 diff --git a/packages/services/service-analytics/src/__tests__/dataset-compare-measure-filters.test.ts b/packages/services/service-analytics/src/__tests__/dataset-compare-measure-filters.test.ts index 03859c4f5a..ce6281fb0a 100644 --- a/packages/services/service-analytics/src/__tests__/dataset-compare-measure-filters.test.ts +++ b/packages/services/service-analytics/src/__tests__/dataset-compare-measure-filters.test.ts @@ -107,10 +107,11 @@ function runQuery(q: AnalyticsQuery): AnalyticsResult { for (const opp of OPPS) { if (range && (opp.close_date < range[0] || opp.close_date > range[1])) continue; if (!matches(opp, q.where)) continue; - // Keyed unambiguously on purpose: `mergeByDimensions` concatenates its own - // key with no delimiter (#4821, filed separately and deliberately NOT - // touched here), and this fake must not import that ambiguity — a test that - // measured two defects at once could not tell which one it caught. + // Keyed unambiguously on purpose, and independently of the executor's own + // key (#4821 — which turned out to be a null-vs-empty conflation rather + // than the missing delimiter it was reported as; the delimiter was a raw + // U+0001 byte, invisible in the issue body). A fake that imported the + // production key could not tell which defect a failure caught. const key = dims .map((d) => JSON.stringify((opp as unknown as Record)[d] ?? null)) .join('|'); diff --git a/packages/services/service-analytics/src/__tests__/dataset-merge-dimension-key.test.ts b/packages/services/service-analytics/src/__tests__/dataset-merge-dimension-key.test.ts new file mode 100644 index 0000000000..d8a2b6a40f --- /dev/null +++ b/packages/services/service-analytics/src/__tests__/dataset-merge-dimension-key.test.ts @@ -0,0 +1,250 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The dimension key `mergeByDimensions` aligns rows on (#4821). + * + * Every multi-query dataset result is assembled through that merge: the primary + * pass against each measure-scoped supplementary pass, and — since #4870 — the + * current window against the shifted `compareTo` window, which fans out per + * measure the same way. A key collision there does not fail: one group silently + * absorbs another's measures and the grid keeps a plausible shape. + * + * ## What was actually wrong, which is not quite what #4821 reported + * + * 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 the + * issue was filed reading `join('')` and its headline repro (`['ab','c']` vs + * `['a','bc']` both keying `"abc"`) never actually reproduced — the separator + * was there, just invisible. What did bite: + * + * - `?? ''` keyed a genuinely NULL dimension the same as an empty-string one, + * merging "unassigned" into "blank"; + * - a one-character separator is only unambiguous while no dimension VALUE + * contains it, and dimension values are user data. + * + * So the suite below pins the tuple-distinctness property directly — it holds + * for adjacent-value ambiguity, for a value carrying the old separator, and for + * null vs. empty — rather than pinning the particular encoding that delivers it. + * + * ## The pin that guards the rejected direction + * + * `numeric 1 and string '1' still key the SAME` is a REGRESSION PIN, not an + * incidental observation. The obvious "fix" (and #4821's own suggestion) is the + * `JSON.stringify` key `cross-object-rebucket.ts` uses, which would render those + * `1` and `"1"` and split them into two groups. That function re-buckets ONE + * query's rows, where a column has one type; this merge ALIGNS SEPARATE + * QUERIES, and drivers do return the same group differently typed across them + * (`compareValues`: "numeric strings, which is how some drivers return SUM + * results"). Adopting JSON here would trade a silent defect for a new one. + */ + +import { describe, it, expect, vi } from 'vitest'; +import type { IAnalyticsService, AnalyticsQuery, AnalyticsResult } from '@objectstack/spec/contracts'; +import { DatasetSchema } from '@objectstack/spec/ui'; +import { compileDataset } from '../dataset-compiler.js'; +import { DatasetExecutor, mergeByDimensions } from '../dataset-executor.js'; + +/** The separator the old key joined on, referenced only as an escape sequence. */ +const OLD_SEPARATOR = '\u0001'; + +const DIMS = ['region', 'segment']; + +describe('mergeByDimensions — distinct dimension tuples never share a key (#4821)', () => { + it('keeps the adjacent-value pair apart: {ab, c} is not {a, bc}', () => { + const base = [ + { region: 'ab', segment: 'c', revenue: 10 }, + { region: 'a', segment: 'bc', revenue: 20 }, + ]; + const extra = [ + { region: 'ab', segment: 'c', won: 1 }, + { region: 'a', segment: 'bc', won: 2 }, + ]; + + const rows = mergeByDimensions(base, extra, DIMS, ['won']); + + expect(rows).toHaveLength(2); + // Each group keeps its OWN number. The failure mode is not an error: it is + // `won: 2` landing on the {ab, c} row while {a, bc} is left without one. + expect(rows.find((r) => r.region === 'ab')).toMatchObject({ segment: 'c', revenue: 10, won: 1 }); + expect(rows.find((r) => r.region === 'a')).toMatchObject({ segment: 'bc', revenue: 20, won: 2 }); + }); + + it('cannot be forged by a value that CONTAINS the old separator', () => { + // The property a single separator character can only assume: this pair is + // indistinguishable once the tuple is joined on U+0001, and a text field + // really can carry one (imported records, pasted payloads). + const base = [ + { region: `a${OLD_SEPARATOR}b`, segment: 'c', revenue: 10 }, + { region: 'a', segment: `b${OLD_SEPARATOR}c`, revenue: 20 }, + ]; + // Addressed to the FIRST row on purpose. Under the old key both base rows + // index to one entry — the LAST one wins — so this merge landed on the + // second row instead: the measure of one group written onto another. + const extra = [{ region: `a${OLD_SEPARATOR}b`, segment: 'c', won: 7 }]; + + const rows = mergeByDimensions(base, extra, DIMS, ['won']); + + expect(rows).toHaveLength(2); + expect(rows.find((r) => r.region === `a${OLD_SEPARATOR}b`)?.won).toBe(7); + expect(rows.find((r) => r.region === 'a')?.won).toBeUndefined(); + }); + + it('keeps a NULL dimension apart from an empty-string one — "unassigned" is not "blank"', () => { + const base = [ + { region: null, segment: 'ent', revenue: 10 }, + { region: '', segment: 'ent', revenue: 20 }, + ]; + const extra = [ + { region: null, segment: 'ent', won: 1 }, + { region: '', segment: 'ent', won: 2 }, + ]; + + const rows = mergeByDimensions(base, extra, DIMS, ['won']); + + expect(rows).toHaveLength(2); + // `?? ''` keyed both as the empty tuple: the unassigned row came back + // without a `won` at all while the blank row absorbed both merges. + expect(rows.find((r) => r.region === null)?.won).toBe(1); + expect(rows.find((r) => r.region === '')?.won).toBe(2); + }); + + it('treats an ABSENT dimension column as the same "no value" as null', () => { + // Deliberate, and the opposite call from null-vs-empty: drivers omit null + // columns from row objects, so splitting here would re-create the + // cross-query mismatch the key exists to absorb. + const base = [{ region: null, segment: 'ent', revenue: 10 }]; + const extra = [{ segment: 'ent', won: 3 }]; + + const rows = mergeByDimensions(base, extra, DIMS, ['won']); + + expect(rows).toHaveLength(1); + expect(rows[0]).toMatchObject({ region: null, segment: 'ent', revenue: 10, won: 3 }); + }); + + it('REGRESSION PIN — numeric 1 and string "1" still key the SAME (no JSON.stringify key)', () => { + // The main query and a measure-scoped sub-query can type one group + // differently; `String()` per segment is what keeps them one row. A + // JSON-encoded key splits this into two rows, silently — which is why this + // test exists and why it must not be "corrected". + const base = [{ region: 1, segment: 2, revenue: 10 }]; + const extra = [{ region: '1', segment: '2', won: 5 }]; + + const rows = mergeByDimensions(base, extra, DIMS, ['won']); + + expect(rows).toHaveLength(1); + expect(rows[0]).toMatchObject({ region: 1, segment: 2, revenue: 10, won: 5 }); + }); +}); + +// ── the same properties through the real executor ─────────────────────────── + +function fakeService(handler: (q: AnalyticsQuery) => AnalyticsResult): IAnalyticsService { + return { query: vi.fn(async (q: AnalyticsQuery) => handler(q)), getMeta: async () => [] }; +} + +const matrix = DatasetSchema.parse({ + name: 'matrix', label: 'Matrix', object: 'opportunity', + dimensions: [ + { name: 'region', field: 'region', type: 'string' }, + { name: 'segment', field: 'segment', type: 'string' }, + { name: 'close_date', field: 'close_date', type: 'date' }, + ], + measures: [ + { name: 'revenue', aggregate: 'sum', field: 'amount' }, + { name: 'won_count', aggregate: 'count', filter: { stage: 'closed_won' } }, + ], +}); + +/** + * Four groups over `region × segment`, each with its own numbers: + * - the adjacent-value pair {ab, c} / {a, bc}; + * - the unassigned/blank pair {null, ent} / {'', ent}. + * + * `won_count` carries a filter, so it arrives as a SEPARATE grouped query that + * has to be merged back by dimension key — the seam under test. Every group + * reports a distinct `won_count`, so any collision shows up as one row wearing + * another's number rather than as a missing row. + */ +const GROUPS = [ + { region: 'ab', segment: 'c', revenue: 10, won_count: 1 }, + { region: 'a', segment: 'bc', revenue: 20, won_count: 2 }, + { region: null as string | null, segment: 'ent', revenue: 30, won_count: 3 }, + { region: '', segment: 'ent', revenue: 40, won_count: 4 }, +]; + +const isShifted = (q: AnalyticsQuery) => JSON.stringify(q.timeDimensions ?? []).includes('2025-12'); + +/** Previous-window numbers are the current ones ×10, so a mix-up is legible. */ +const matrixService = fakeService((q) => { + const scale = isShifted(q) ? 10 : 1; + const measure = q.measures[0]; + return { + rows: GROUPS.map((g) => ({ + region: g.region, + segment: g.segment, + [measure]: (measure === 'revenue' ? g.revenue : g.won_count) * scale, + })), + fields: [], + }; +}); + +const runMatrix = (compareTo?: boolean) => + new DatasetExecutor(matrixService).execute(compileDataset(matrix), { + dimensions: ['region', 'segment'], + measures: ['revenue', 'won_count'], + ...(compareTo + ? { + timeDimensions: [ + { dimension: 'close_date', dateRange: ['2026-01-01', '2026-01-31'] as [string, string] }, + ], + compareTo: { kind: 'previousPeriod' as const, dimension: 'close_date' }, + } + : {}), + }); + +const find = (rows: Record[], region: string | null) => + rows.find((r) => r.region === region && r.segment === (region === null || region === '' ? 'ent' : r.segment))!; + +describe('executor — a measure-scoped sub-query merges onto the right row (#4821)', () => { + it('gives every group its own won_count, unassigned and blank included', async () => { + const rows = (await runMatrix()).rows; + + expect(rows).toHaveLength(4); + expect(find(rows, 'ab')).toMatchObject({ segment: 'c', revenue: 10, won_count: 1 }); + expect(find(rows, 'a')).toMatchObject({ segment: 'bc', revenue: 20, won_count: 2 }); + expect(find(rows, null)).toMatchObject({ revenue: 30, won_count: 3 }); + expect(find(rows, '')).toMatchObject({ revenue: 40, won_count: 4 }); + }); + + it('does not render the unassigned group as an empty group', async () => { + const rows = (await runMatrix()).rows; + // The blank row used to absorb both merges, leaving the unassigned row's + // `won_count` ABSENT — which #4708 then fills with a confident 0. A count + // of 3 reported as 0 is the shape of this defect after that fill. + expect(find(rows, null).won_count).not.toBe(0); + expect(find(rows, null).won_count).toBe(3); + }); +}); + +describe('executor — the compareTo merge lands on the right row too (#4870 seam, #4821)', () => { + it('attaches each group its OWN __compare columns', async () => { + const rows = (await runMatrix(true)).rows; + + expect(rows).toHaveLength(4); + // Previous window = current ×10. A collision on this merge writes one + // group's history into another's row — the `__compare` column is what the + // reader subtracts, so a wrong one inverts the direction of the tile. + expect(find(rows, 'ab')).toMatchObject({ revenue__compare: 100, won_count__compare: 10 }); + expect(find(rows, 'a')).toMatchObject({ revenue__compare: 200, won_count__compare: 20 }); + expect(find(rows, null)).toMatchObject({ revenue__compare: 300, won_count__compare: 30 }); + expect(find(rows, '')).toMatchObject({ revenue__compare: 400, won_count__compare: 40 }); + }); + + it('does not append phantom rows for buckets that already exist', async () => { + // `mergeByDimensions` APPENDS an unmatched extra row, so a key that fails + // to match its own counterpart doubles the grid instead of merging it. + const rows = (await runMatrix(true)).rows; + const keys = rows.map((r) => `${String(r.region)}|${String(r.segment)}`); + expect(new Set(keys).size).toBe(keys.length); + }); +}); diff --git a/packages/services/service-analytics/src/dataset-executor.ts b/packages/services/service-analytics/src/dataset-executor.ts index c85bea3de5..aff378a66b 100644 --- a/packages/services/service-analytics/src/dataset-executor.ts +++ b/packages/services/service-analytics/src/dataset-executor.ts @@ -910,10 +910,91 @@ export class DatasetExecutor { } } +/** + * Key segment for a dimension whose value is null/undefined (#4821). + * + * Every other segment {@link dimensionKeyOf} emits begins with a decimal digit + * (its length prefix), so a segment beginning with anything else cannot be + * produced by any real value — which is the entire requirement for a sentinel. + * + * `undefined` keys the same as `null` deliberately: a row that OMITS the column + * and a row carrying an explicit null both mean "no value here", and drivers do + * omit null columns from row objects. Splitting on that would re-introduce, one + * level down, exactly the cross-query split this key exists to prevent. + */ +const NULL_DIMENSION_SEGMENT = '~'; + +/** + * The composite index key for one row's dimension tuple — **length-prefixed**, + * so no two distinct tuples can share a key and no character is reserved + * (#4821). + * + * ## What was actually wrong + * + * The previous key was `dimensions.map((d) => String(row[d] ?? '')).join(SOH)`, + * where SOH was a **raw U+0001 byte written literally into the source**. #4821 + * was filed against it reading `join('')`, because a raw control byte renders as + * nothing — in a terminal, in a GitHub issue body, and in this file. So the + * headline defect it reports (`['ab','c']` and `['a','bc']` both keying `"abc"`) + * did not in fact reproduce: the separator was present, merely invisible. Two + * things did: + * + * 1. `?? ''` keys a genuinely NULL dimension identically to an empty-string + * one, so "unassigned" merges into "blank" — one real group absorbing + * another's measures, silently, which is the outcome #4821 describes, + * reached through its second mechanism rather than its first. + * 2. A single-character separator is unambiguous only while no dimension VALUE + * contains that character. Dimension values are user data (text fields, + * imported records), so that is an assumption, not a guarantee — and it + * fails exactly as silently as the issue predicted. + * + * A length prefix settles (2) by construction — `2:ab1:c` and `1:a2:bc` differ + * for every possible input, nothing is reserved, and no invisible byte is left + * in the source for the next reader to misread (this comment's own issue was + * filed because of one). (1) is settled separately and explicitly, by + * {@link NULL_DIMENSION_SEGMENT}. + * + * ## Why each segment is still `String()`-coerced + * + * Deliberately — and it is NOT the trade-off {@link rebucketCrossObject} makes + * one file over, whose key JSON-encodes each part. That function re-buckets the + * rows of ONE aggregate result: every value in a column came back from a single + * query and so carries a single JS type, which makes JSON free there and buys a + * real distinction (the empty bucket `null` vs. the literal string `"null"`). + * + * This key does the opposite job — it ALIGNS rows across **different queries**: + * the primary pass against each measure-scoped supplementary pass, and (since + * #4870) the current window against the shifted `compareTo` window, which now + * fans out per measure the same way. Nothing guarantees two queries type the + * same group identically, and {@link compareValues} records this executor's own + * encounter with it: "numeric strings, which is how some drivers return SUM + * results". `String()` per segment is what keys numeric `1` and string `'1'` the + * same, so such rows keep merging. A `JSON.stringify` key renders them `1` vs + * `"1"` and would split a group that merges correctly today — trading one silent + * defect for a new one. Pinned by test; do not "simplify" it away. + */ +function dimensionKeyOf(row: Record, dimensions: string[]): string { + let key = ''; + for (const d of dimensions) { + const value = row[d]; + if (value == null) { + key += NULL_DIMENSION_SEGMENT; + continue; + } + const s = String(value); + key += `${s.length}:${s}`; + } + return key; +} + /** * Left-merge `extra` rows onto `base` rows by their dimension-key tuple, * copying the listed value columns. Rows in `extra` with no base match are * appended (outer-ish merge so comparison-only buckets still surface). + * + * Rows are matched by {@link dimensionKeyOf} — read its notes before changing + * how the key is built. Both the ambiguity it removes and the type coercion it + * keeps are load-bearing, and both fail silently when got wrong. */ export function mergeByDimensions( base: Record[], @@ -921,7 +1002,7 @@ export function mergeByDimensions( dimensions: string[], valueColumns: string[], ): Record[] { - const keyOf = (row: Record) => dimensions.map((d) => String(row[d] ?? '')).join(''); + const keyOf = (row: Record) => dimensionKeyOf(row, dimensions); const index = new Map>(); for (const row of base) index.set(keyOf(row), row);