Skip to content

fix(analytics): a dataset measure's result type stops contradicting its own value — min/max over a temporal field is time, not number - #16101

Merged
os-warren merged 1 commit into
mainfrom
claude/issue-15768-analytics-measure-result-type
Sep 6, 2026
Merged

fix(analytics): a dataset measure's result type stops contradicting its own value — min/max over a temporal field is time, not number#16101
os-warren merged 1 commit into
mainfrom
claude/issue-15768-analytics-measure-result-type

Conversation

@os-warren

Copy link
Copy Markdown
Collaborator

Refs #15768 — the services half only. The renderer half of that card is untouched here; see "Deliberately left alone" below.

What was wrong

POST /api/v1/analytics/dataset/query described every measure column as type: "number", including a min/max over a date / datetime / time field whose value in the same response is an ISO instant:

{"rows":[{"oldest_last_update_at":"2026-07-04T07:00:00.000Z","untouched_over_30d":3}],
 "fields":[{"name":"oldest_last_update_at","type":"number","label":"Oldest touch","format":"relative"},
           {"name":"untouched_over_30d","type":"number","label":"Untouched > 30 days"}]}

min and max return a value of the aggregated field's own type. The column carried an instant and the metadata beside it denied it — enough on its own to keep a formatter that branches on the declared type from ever reaching a temporal branch.

What changed

  • New packages/services/service-analytics/src/measure-result-type.tsmeasureResultType(aggregate, sourceFieldType), the enumerated per-aggregate verdict, answering undefined for "nothing to say, keep what the producer minted".
  • analytics-service.ts — two lines in queryDataset's ADR-0021 result-column enrichment, plus the import and the note explaining why the correction is made there.
  • packages/spec/src/contracts/analytics-service.ts + packages/spec/src/api/analytics.zod.tsAnalyticsResult.fields[].type and its wire schema now state the vocabulary this position speaks and what each aggregate answers. Neither declaration widens: the wire type was, and remains, a string.
  • A changeset, graded on the surface: minor for @objectstack/service-analytics (a published wire surface changes what it produces), patch for @objectstack/spec (declaration text only).

Row values are untouched on every path. computeDerived still coerces its operands with Number() and still sees exactly the values it saw before — this change moves column METADATA only.

Where the assembly point is, and how it was proved

The triage seat recorded that it had not located the production code behind fields[].type — its grep hit only test constants under src/__tests__/**. Treating that as not found rather than found, the search turned up four producers of the measure descriptor, each spelling { name: m, type: 'number' } and none of them knowing the aggregated field's type:

producer file
ObjectQLStrategy.buildFieldMeta strategies/objectql-strategy.ts
NativeSQLStrategy.buildFieldMeta strategies/native-sql-strategy.ts
evaluateAnalyticsQueryOverRows preview-evaluator.ts
DatasetExecutor.runMeasurePass (+ compare / derived appends) dataset-executor.ts

What all of them pass through is AnalyticsService.queryDataset's ADR-0021 enrichment block — the one that already resolves label / format / currency / percentScale / builtinAggregate from the authored measure plus sourceFieldMeta. The REST face relays that method's return verbatim: the POST {basePath}/analytics/dataset/query route in packages/rest/src/rest-server.ts ends res.json(result). So the enrichment block is the wire's last word on this key, and it is the only place holding both halves of the question — the authored measure (aggregate + field) and the source field's declared type. A per-producer copy would be four implementations of one rule, free to drift.

Proved by control, not by reading. Reverting only the two-line call site (leaving the rule module in place) and re-measuring:

  • Mutation confirmed on disk before the run: call-site occurrences 1 → 0, injected marker 0 → 1, file blob 978d82ea → d23ca934.
  • Result: Tests 5 failed | 20 passed (25), and the five are exactly the cases that assert time — the ObjectQL producer, the supplementary-sub-query producer, the primary buildFieldMeta producer, the __compare producer, and the two-strategy agreement case. The assertion diff reads - "type": "time" / + "type": "number".
  • Direction was predicted before running (recorded in the test file's header): 5 red, 20 green. Measured exactly that.
  • Restore verified: blob back to 978d82ea, byte-identical to the HEAD blob, git diff HEAD empty.

The subject is reached by a relative SOURCE import inside its own package, not through a dependency's exports map, so no build sits between the edit and the run — the implementation itself measured green with no service-analytics build after it was written, which is the same fact from the other side.

Two of the four producers are the two buildFieldMetas, and the test drives one selection down both of them and asserts identical column metadata. That is only possible if the value the wire carries is decided downstream of both, which is the claim.

The population, enumerated

AggregationFunction (packages/spec/src/data/query.zod.ts) is a closed vocabulary, so the population is finite and every member is answered. The test asserts the table's membership against AggregationFunction.options, so a member added to the spec fails here instead of silently inheriting the flat number.

aggregate what it returns verdict why
count a row count number — unchanged counting datetimes is still counting; typing it otherwise would be a new bug
count_distinct a cardinality number — unchanged same reason
sum backend-decided over a temporal column number — unchanged see below
avg backend-decided over a temporal column number — unchanged see below
min a value of the aggregated field's own type time when that field is temporal the measured defect
max a value of the aggregated field's own type time when that field is temporal same rule
(no aggregate = derived) a computed number number — unchanged computeDerived coerces operands with Number(), so it is numeric by construction

sum / avg over a temporal field was established, not assumed. Nothing in the shipped stack refuses the pair: DatasetMeasureSchema declares aggregate and field independently, dataset-compiler.aggregateToMetricType checks only vocabulary membership, the three source-field gates (assertMeasureFields / assertDimensionFields / assertWhereFields) check only that the column exists, and packages/lint carries no rule pairing the two. It therefore reaches the driver, where the answer is backend-decided — a mean of epoch integers on SQLite, an error on Postgres, which has no avg(timestamptz). There is no one value for a type to describe, so none is invented and the gap is reported as its own card rather than papered over.

The field-type axis is deliberately narrower than the aggregate axis. Only the temporal family — date, datetime, time — is corrected: the population the card measured and the triage ruled on. A min/max over a text / select / lookup field returns a string and is still described as number; that is the same defect over a different population, reported separately rather than absorbed here, because several members of the wider set (autonumber, boolean, formula) have genuinely uncertain answers that would ship as declarations if guessed.

Tiered "cannot answer, do not block". A host with no sourceFieldMeta wired, and a measure over a relationship PATH (sourceFieldMeta resolves a column on the BASE object, so a dotted field answers undefined), both leave the column exactly as the query layer produced it. Both are pinned.

Why time and not date / datetime

fields[].type is not a FieldType position. A temporal DIMENSION column in the very same response already carries timeDatasetDimensionSchema's type: 'date' compiles to a cube dimension of type: 'time' (dataset-compiler.dimensionType), and both buildFieldMetas copy it through. time is the DimensionType vocabulary this position already speaks (string / number / boolean / time / geo), so a consumer that can draw a date axis at all already has the branch. A second temporal word in one wire position would leave every existing time branch unreached. A test pins the dimension column's time beside the measure column's, so the two cannot drift into two spellings.

Deliberately left alone

Out-of-scope findings, filed unassigned

Duplicate-checked with one targeted search each; the search was verified live in this session by a control term that returned #15768 itself.

One further observation, not filed because it may well be deliberate: CubeRegistry.fieldTypeToDimensionType maps date and datetime to time but leaves Field.time on the default arm, so an auto-inferred cube types a clock-time dimension as string. A clock time has no calendar buckets, so that may be the intended answer.

Verification — measured, with exit codes

Every exit code was captured immediately after a single redirected command, never through a pipe. Exit 3 is PREREQUISITE NOT MET, not a pass: each one below was satisfied and the gate re-run.

what command exit
dependency closure pnpm --workspace-concurrency=2 --filter '@objectstack/service-analytics^...' build 0
the new test file pnpm --filter @objectstack/service-analytics exec vitest run --maxWorkers=2 src/__tests__/measure-result-type.test.ts 0 — Test Files 1 passed (1), Tests 25 passed (25)
the whole package pnpm --filter @objectstack/service-analytics test 0 — Test Files 93 passed (93), Tests 2027 passed (2027)
package typecheck pnpm --filter @objectstack/service-analytics typecheck 0
typecheck really reaches both new files tsc --noEmit --listFiles, grep 2 hits: measure-result-type.ts and __tests__/measure-result-type.test.ts — so the green above is not a skip
spec generated artifacts pnpm --filter @objectstack/spec build then check:generated 0 / 0 — "All 15 generated artifacts are up to date"; nothing needed regenerating
reverse verification call site reverted, rule module kept Tests 5 failed / 20 passed (25) — exactly the predicted five; restore verified byte-identical to the HEAD blob

The gate family was derived mechanically from the actual changed files, not hand-built: node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack. Its Reconciliation line reads 69 families (57 by path + 7 by change kind + 7 declared whole-tree, 2 reached both ways). All 69 were run at the final commit dacea1782, and all 69 exited 0. Three answered 3 on the first pass and were re-run green after their prerequisite:

  • check:doc-formula-expressions — needed turbo run build --filter=@objectstack/lint.
  • check:dual-build-cjs-loads and check:type-check-debt — needed the full closure, turbo run build --filter='./packages/*' --filter='./packages/*/*'; check:type-check-debt then needed @objectstack/service-analytics rebuilt on top, because this PR edits its sources.

Final read of the ledger gate: check-type-check-coverage --re-measure: OK — 12 ledger entries re-measured, 140 raw tsc errors total, none above its recorded number. surplus: none.

Six further families this PR's paths reach take a value from the workflow ($RUNNER_TEMP, ${{ matrix.shard }}) and have no local invocation. They are UNMEASURED here — the derivation names them and refuses to invent a command, and so does this note.

Heavy runs went through scripts/pm/os-verify-lock.sh; the verdict line, not a bare $?, is what each exit above was read from.


🤖 Generated with Claude Code

https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y


Generated by Claude Code

…ts own value

Every producer of `AnalyticsResult.fields` minted `{ name, type: 'number' }`
for a measure — both strategies' `buildFieldMeta`, the draft-preview evaluator,
and `DatasetExecutor.runMeasurePass`'s supplementary / compare / derived
appends. That is right for most of the closed `AggregationFunction` vocabulary
and wrong for `min`/`max` over a `date`/`datetime`/`time` field, which return a
value OF THE AGGREGATED FIELD'S OWN TYPE: the response then carried an ISO
instant and a `type: "number"` describing it, in one line.

The measure column's type is now resolved in `queryDataset`'s ADR-0021
result-column enrichment — the block that already resolves `label` / `format` /
`currency` / `percentScale` from the authored measure plus `sourceFieldMeta`,
and the one seam every producer passes through on the way to a route that
relays the return verbatim. The rule itself lives in `measure-result-type.ts`
so the per-aggregate verdict has one home rather than four copies.

The corrected spelling is `time`, the `DimensionType` word a temporal DIMENSION
column in the same response has always carried; a second temporal word in one
wire position would leave every existing consumer branch unreached.

Only `min`/`max` move. `count`/`count_distinct` are numeric however temporal
the column they read is; `sum`/`avg` over a temporal column are refused by no
layer and answered by the backend, so no type is invented for them; a derived
measure is numeric by construction. Tiered "cannot answer, do not block": a
host with no source-field metadata, and a measure over a relationship path,
leave the column exactly as produced. Row values are untouched on every path.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
@github-actions github-actions Bot added the size/l label Sep 6, 2026
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 2 package(s): @objectstack/service-analytics, @objectstack/spec, touching 8 documentable anchor(s).

1 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/api/data-api.mdx (via AnalyticsResult (symbol, a top-level interface), AnalyticsResultResponseSchema (symbol, a top-level const))

3 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx (via queryDataset (symbol, a method of class AnalyticsService))
  • content/docs/releases/v17.mdx (via AnalyticsResult (symbol, a top-level interface))
  • content/docs/releases/v9.mdx (via AnalyticsResult (symbol, a top-level interface), queryDataset (symbol, a method of class AnalyticsService))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • the SDK route bridge reached 61 of 219 client-bound route-ledger rows — the other 158 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 158: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 130 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 33e939ff318ffecbaf9fe4dd4401dee679e08698packageMentionDocs.

Which tree this was computed on

This run read content/docs from 464ab7a88b5a70bf82f4f8e434764809d7c7d45b — the merge of head dacea17828c14d802f7d1752a30185b188409f03 into base 33e939ff318ffecbaf9fe4dd4401dee679e08698, 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 464ab7a88b5a70bf82f4f8e434764809d7c7d45b && git checkout 464ab7a88b5a70bf82f4f8e434764809d7c7d45b
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 33e939ff318ffecbaf9fe4dd4401dee679e08698 dacea17828c14d802f7d1752a30185b188409f03 && git checkout -B drift-repro 33e939ff318ffecbaf9fe4dd4401dee679e08698 && git merge --no-ff dacea17828c14d802f7d1752a30185b188409f03

node scripts/docs-audit/affected-docs.mjs --json 33e939ff318ffecbaf9fe4dd4401dee679e08698

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

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 33e939ff318ffecbaf9fe4dd4401dee679e08698 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

Copy link
Copy Markdown
Collaborator Author

PM 验收 · 独立复核了一处,结论:成立。⛔ 其余未重测。

本席位只重测了一件事 —— 正文里那个唯一的设计选择(为什么是 time),因为 dev 自己在 out-of-scope 里留了一条看起来会削弱它的观察。⇒ 追下去了,不削弱

我担心的是什么

dev 记了一条未立卡的观察:

CubeRegistry.fieldTypeToDimensionType maps date and datetime to time but leaves Field.time on the default arm

而正文用「同一响应里的时间维度列本来就是 time」来论证度量列也该是 time。⇒ 若维度侧对 Field.timestring,那么一个 min over Field.time 会被本 PR 定为 time,同一响应里两个位置对同一字段类型给出两种拼写 —— 正文的「one position keeps one vocabulary」就会是一句比事实更宽的话。

实测:两个函数,两条路径,不是同一件事

packages/services/service-analytics/src/cube-registry.ts:132   fieldTypeToDimensionType(fieldType)
packages/services/service-analytics/src/dataset-compiler.ts:184 dimensionType(d: DatasetDimension)

正文引的是后者,而 dev 的观察说的是前者。⇒ 两者服务不同的构造(自动推断的 cube vs 已授权的 dataset),⛔ 不构成同一响应内的矛盾。

关键读数 —— dataset-compiler.ts:184:

switch (d.type) {
  case 'date':    return 'time';
  case 'number':  return 'number';
  case 'boolean': return 'boolean';
  case 'lookup':  return 'string';
  case 'string':  return 'string';
  default:        return 'string';
}

DatasetDimension.type 的词表里根本没有 time 这个成员 —— 只有 date | number | boolean | lookup | string。⇒ 在 dataset/query 这条线上,任何时间维度都只能被授权为 date,因而一律编译成 time。⇒ 正文那句话在它自己引的路径上逐字成立,而且 Field.time 根本无法在这条路径上产生一个不同拼写的维度列。

⇒ dev 那条观察谈的是自动推断 cube 的响应,另一种响应形状。⛔ 不与本 PR 冲突,保留为未立卡观察是对的。

⛔ 本席位没有重测的部分

  • 69 个门族全 0(含三个先答 3 = PREREQUISITE NOT MET、补齐前置后转绿)。⇒ 按 dev 报告记,未复跑
  • 反向验证(改回两行调用点 → 5 红 20 绿)。磁盘态证据、blob 前后值、git diff HEAD 为空的还原验证都在正文里,⇒ 未复跑
  • 那六个取 workflow 变量、本地无从调用的门族 —— 正文记为 UNMEASURED,⭐ 这是正确的记法(⛔ 未测量不等于绿),本席位同意其为未测量,不同意任何把它读成绿的说法。

派单口径核对

Refs #15768(⛔ 非 Fixes)、只做服务端半边、未开 objectui worktree、未碰 #16020 占用的四个文件 —— 与派单一致。⚠️ 卡片 #15768 因此不会因本 PR 落地而关闭:落地时按部分落地释放,剩下的渲染端半边需要改道(objectui,或经版本错位确认后作废),届时加 pm:retriage

⛔ 本 PR 仍为 draft、未 arm —— 按维护者裁决,等总监契约复审,CI 绿本身不是门槛。

domain:services PM 席位 · 只复核一处,复核结论与 dev 一致


Generated by Claude Code

@os-warren
os-warren marked this pull request as ready for review September 6, 2026 03:46
@os-warren
os-warren enabled auto-merge September 6, 2026 03:46
@os-warren
os-warren added this pull request to the merge queue Sep 6, 2026
Merged via the queue into main with commit 07f40e5 Sep 6, 2026
35 of 36 checks passed
@os-warren
os-warren deleted the claude/issue-15768-analytics-measure-result-type branch September 6, 2026 04:25
os-warren pushed a commit that referenced this pull request Sep 6, 2026
…s columns like the live one (#16097)

`queryDataset`'s ADR-0037 P3 preview branch returned ~250 lines before the
ADR-0021 result-column enrichment, so a response over drafted seed rows carried
no `label`, `format`, `currency`, `percentScale`, `builtinAggregate` and no
`type` correction — on measure and dimension columns alike. The same dataset in
the same widget described its columns differently depending only on whether a
pending seed draft existed.

Every key that block writes is read off the authored dataset and
`sourceFieldMeta`, never off `result.rows`, so it is extracted into one
`enrichResultColumns` seam that both paths call — one rule, not a per-path copy
free to drift, the same argument #15768/#16101's `type` correction already makes
for living there.

Dimension VALUE label resolution stays skipped on the preview path on purpose;
the standing comment is narrowed to say that it is a statement about row values
and never covered the column descriptors.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
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.

2 participants