From 4df5628e0517ec4ca4f519202d921fcfaa610a37 Mon Sep 17 00:00:00 2001 From: os-dev Date: Sun, 6 Sep 2026 06:10:52 +0000 Subject: [PATCH 1/2] fix(service-analytics): a draft-preview min/max answers the operand's own type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `evaluateAnalyticsQueryOverRows` coerced every aggregate operand with `Number()` and dropped the non-finite ones, so `min`/`max` over a non-numeric field answered `0` on the draft-preview path while the live path answered the value itself — a different, wrong answer to the same query, with no refusal and no warning. Every dimension column it minted was typed `'string'` for the same reason (the producer assumed both). `min`/`max` now return the winning operand in its own type, ordered by this file's shared `compare` with a numeric arm for numeric operands, and a group with only nulls answers `null` (`emptyGroupValueFor`) instead of `0`. `count_distinct`'s arm was unreachable — the switch spelled it `countDistinct`, which no producer mints — so it fell to the numeric default and answered a row count; it is now spelled as the spec spells it. A dimension column takes the cube dimension's own type, the same expression the live producers use. `sum`/`avg` over a temporal operand is deliberately unchanged: there is no defined answer and #16099 owns refusing the pair. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y --- .../preview-aggregate-operand-type.test.ts | 332 ++++++++++++++++++ .../src/preview-evaluator.ts | 133 ++++++- 2 files changed, 455 insertions(+), 10 deletions(-) create mode 100644 packages/services/service-analytics/src/__tests__/preview-aggregate-operand-type.test.ts diff --git a/packages/services/service-analytics/src/__tests__/preview-aggregate-operand-type.test.ts b/packages/services/service-analytics/src/__tests__/preview-aggregate-operand-type.test.ts new file mode 100644 index 0000000000..907cfd0db9 --- /dev/null +++ b/packages/services/service-analytics/src/__tests__/preview-aggregate-operand-type.test.ts @@ -0,0 +1,332 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #16203 — the draft-preview evaluator assumed every measure is numeric and + * every dimension is a string. + * + * `evaluateAnalyticsQueryOverRows` (`preview-evaluator.ts`) produces its result + * BEFORE any descriptor pass runs, so neither symptom is reachable from an + * enrichment step: + * + * (a) `aggregate()` coerced every operand with `Number()` and dropped the + * non-finite ones, so `min`/`max` over a NON-NUMERIC field answered `0` + * — a different, wrong VALUE for the same query, with no refusal and no + * warning; + * (b) the `fields` it minted typed EVERY dimension column `'string'`, so a + * time dimension was `'string'` on preview and `'time'` on live. + * + * ## The instrument + * + * One dataset, one row set, two `AnalyticsService` instances differing in + * exactly one config key — `draftRowsResolver` — so a difference between the + * two responses is a difference the preview evaluator caused. The LIVE half is + * not a model of an engine: it is `NativeSQLStrategy`'s generated SQL executed + * on a real SQLite (sql.js) whose table is seeded from {@link ROWS}, the same + * rows the resolver hands the preview. `date` is stored as TEXT, which is this + * platform's canonical storage form for it (ADR-0053 D-B). + * + * ## Per aggregate — the whole closed vocabulary, no member left unconsidered + * + * `AggregationFunction` (`spec/data/query.zod.ts`) is CLOSED, so this table has + * a finite answer, and the sibling ruling for the DESCRIPTOR half of the same + * question (`measure-result-type.ts`, #15768/#16101) answers it the same way: + * + * | aggregate | preview answers | + * |:-----------------|:-------------------------------------------------------| + * | `count` | a row count — numeric, unchanged | + * | `count_distinct` | a cardinality — numeric; the arm was UNREACHABLE (the | + * | | switch spelled it `countDistinct`, a word no producer | + * | | mints) so it fell to the numeric `default` and answered | + * | | a SUM / a row count instead | + * | `sum` | numeric, unchanged | + * | `avg` | numeric, unchanged | + * | `min` / `max` | the winning operand IN ITS OWN TYPE — the fix | + * + * ⛔ `sum`/`avg` over a TEMPORAL operand is deliberately left exactly as it is: + * there is no defined answer (the two faces below do not agree on one either), + * and #16099 is the open card for REFUSING an incoherent aggregate/field-type + * pair. Inventing a semantic here would pre-empt that ruling; the case below + * pins today's behaviour so the ruling lands visibly. + * + * ⛔ Derived measures are not touched: `computeDerived` (`dataset-executor.ts`) + * coerces every operand with `Number()` and answers `null` when one is not + * finite. This change does not reach that function — it changes what the + * function READS, and the case below measures which way that goes. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { DatasetSchema } from '@objectstack/spec/ui'; +import type { Cube } from '@objectstack/spec/data'; +import { AnalyticsService } from '../analytics-service.js'; +import { evaluateAnalyticsQueryOverRows } from '../preview-evaluator.js'; + +// ── one fixture, two paths ────────────────────────────────────────────────── + +/** + * `payer` is the nullable, duplicate-bearing column (the `AGGREGATION_ROWS` + * shape): `travel` holds two distinct payers over three rows, `meals` one over + * two, with a null — so `count(*)`, `count_distinct(payer)` and a per-group + * answer are three different numbers and a face that collapsed one into + * another cannot pass by coincidence. + */ +const ROWS: Record[] = [ + { id: '1', category: 'travel', payer: 'ann', amount: 1200, spent_on: '2026-05-03' }, + { id: '2', category: 'travel', payer: 'ann', amount: 800, spent_on: '2026-05-12' }, + { id: '3', category: 'travel', payer: 'bob', amount: 500, spent_on: '2026-04-21' }, + { id: '4', category: 'meals', payer: 'bob', amount: 60, spent_on: '2026-06-01' }, + { id: '5', category: 'meals', payer: null, amount: 140, spent_on: '2026-06-02' }, +]; + +const DATASET = DatasetSchema.parse({ + name: 'expense_ds', + label: 'Expense', + object: 'expense', + dimensions: [ + { name: 'category', field: 'category', type: 'string', label: 'Category' }, + // No `dateGranularity`: the raw column groups on both faces, so the only + // thing that can differ about this column is how it is DESCRIBED. + { name: 'spent_on', field: 'spent_on', type: 'date', label: 'Spent On' }, + ], + measures: [ + { name: 'expense_count', aggregate: 'count' }, + { name: 'distinct_payers', aggregate: 'count_distinct', field: 'payer' }, + { name: 'total_amount', aggregate: 'sum', field: 'amount' }, + { name: 'avg_amount', aggregate: 'avg', field: 'amount' }, + { name: 'min_amount', aggregate: 'min', field: 'amount' }, + { name: 'max_amount', aggregate: 'max', field: 'amount' }, + // ⭐ the card's measured pair: `'2026-05-12'` live, `0` on preview. + { name: 'latest_spend', aggregate: 'max', field: 'spent_on' }, + { name: 'earliest_spend', aggregate: 'min', field: 'spent_on' }, + // the TEXT population (#16098's population, on the preview path). + { name: 'first_payer', aggregate: 'min', field: 'payer' }, + { name: 'last_payer', aggregate: 'max', field: 'payer' }, + // ⛔ #16099 owns this pair — pinned as-is, not fixed. + { name: 'sum_spent_on', aggregate: 'sum', field: 'spent_on' }, + { name: 'avg_spent_on', aggregate: 'avg', field: 'spent_on' }, + // derived: one over numeric operands, one over a temporal `max`. + { name: 'amount_per_expense', derived: { op: 'ratio', of: ['total_amount', 'expense_count'] } }, + { name: 'latest_per_expense', derived: { op: 'ratio', of: ['latest_spend', 'expense_count'] } }, + ], +}); + +const MEASURES = [ + 'expense_count', 'distinct_payers', 'total_amount', 'avg_amount', 'min_amount', 'max_amount', + 'latest_spend', 'earliest_spend', 'first_payer', 'last_payer', + 'sum_spent_on', 'avg_spent_on', 'amount_per_expense', 'latest_per_expense', +]; + +let db: any; + +const runSql = (sql: string, params: unknown[]): Record[] => { + const stmt = db.prepare(sql.replace(/\$\d+/g, '?')); + stmt.bind(params as any[]); + const rows: Record[] = []; + while (stmt.step()) rows.push(stmt.getAsObject()); + stmt.free(); + return rows; +}; + +async function locateWasm(): Promise<((file: string) => string) | undefined> { + try { + const { createRequire } = await import('node:module'); + const require = createRequire(import.meta.url); + const pkgJsonPath = require.resolve('sql.js/package.json'); + const { dirname, join } = await import('node:path'); + return (file: string) => join(dirname(pkgJsonPath), 'dist', file); + } catch { + return undefined; + } +} + +/** + * The two services differ in ONE key. Everything else — the dataset, the rows, + * the capabilities, the SQL engine behind `executeRawSql` — is shared. + */ +function svc(preview: boolean) { + return new AnalyticsService({ + queryCapabilities: () => ({ nativeSql: true, objectqlAggregate: false, inMemory: false }), + executeRawSql: async (_object: string, sql: string, params: unknown[]) => runSql(sql, params), + ...(preview ? { draftRowsResolver: async () => ROWS } : {}), + }); +} + +type Grid = { byCategory: Record>; fields: Record }; + +async function grid(preview: boolean, dimensions: string[] = ['category']): Promise { + const result = await svc(preview).queryDataset( + DATASET, + { dimensions, measures: MEASURES }, + undefined, + preview ? { previewDrafts: true } : undefined, + ); + return { + byCategory: Object.fromEntries(result.rows.map((r) => [String(r[dimensions[0]]), r])), + fields: Object.fromEntries(result.fields.map((f) => [f.name, f as { name: string; type: string }])), + }; +} + +beforeAll(async () => { + const mod: any = await import('sql.js'); + const initSqlJs = mod.default ?? mod; + const locateFile = await locateWasm(); + const SQL = await initSqlJs(locateFile ? { locateFile } : undefined); + db = new SQL.Database(); + db.run(`CREATE TABLE "expense" ("id" TEXT PRIMARY KEY, "category" TEXT, "payer" TEXT, "amount" REAL, "spent_on" TEXT);`); + const insert = db.prepare(`INSERT INTO "expense" ("id","category","payer","amount","spent_on") VALUES (?,?,?,?,?)`); + for (const r of ROWS) insert.run([r.id, r.category, r.payer, r.amount, r.spent_on] as any[]); + insert.free(); +}); + +afterAll(() => db?.close()); + +describe('#16203 (a) — a `min`/`max` over a non-numeric field is a VALUE of that field\'s type', () => { + it('the temporal pair the card measured: the same answer on both paths', async () => { + const live = await grid(false); + const preview = await grid(true); + // Pre-fix: preview answered 0 for both of these on both groups. + expect(preview.byCategory.travel.latest_spend).toBe('2026-05-12'); + expect(preview.byCategory.travel.earliest_spend).toBe('2026-04-21'); + expect(preview.byCategory.meals.latest_spend).toBe('2026-06-02'); + // …and the live path, executed on a real SQLite over the same rows, agrees. + expect(preview.byCategory.travel.latest_spend).toBe(live.byCategory.travel.latest_spend); + expect(preview.byCategory.travel.earliest_spend).toBe(live.byCategory.travel.earliest_spend); + expect(preview.byCategory.meals.latest_spend).toBe(live.byCategory.meals.latest_spend); + }); + + it('the TEXT population too — the same defect over a `text`/`select`/`lookup` operand', async () => { + const live = await grid(false); + const preview = await grid(true); + expect(preview.byCategory.travel.first_payer).toBe('ann'); + expect(preview.byCategory.travel.last_payer).toBe('bob'); + expect(preview.byCategory.travel.first_payer).toBe(live.byCategory.travel.first_payer); + expect(preview.byCategory.travel.last_payer).toBe(live.byCategory.travel.last_payer); + }); + + it('a NUMERIC operand is untouched — still a number, still the same number', async () => { + const live = await grid(false); + const preview = await grid(true); + expect(preview.byCategory.travel.min_amount).toBe(500); + expect(preview.byCategory.travel.max_amount).toBe(1200); + expect(typeof preview.byCategory.travel.min_amount).toBe('number'); + expect(preview.byCategory.travel.min_amount).toBe(live.byCategory.travel.min_amount); + expect(preview.byCategory.travel.max_amount).toBe(live.byCategory.travel.max_amount); + }); + + it('a group whose operand is null throughout answers null, never 0 (`emptyGroupValueFor`)', () => { + const CUBE = { + name: 'e', sql: 'expense', + dimensions: { category: { name: 'category', type: 'string', sql: 'category' } }, + measures: { latest: { name: 'latest', type: 'max', sql: 'spent_on' } }, + } as unknown as Cube; + const r = evaluateAnalyticsQueryOverRows( + { measures: ['latest'], dimensions: ['category'] }, + CUBE, + [{ category: 'x', spent_on: null }, { category: 'x' }], + ); + // `aggregation-policy.emptyGroupValueFor` rules `min`/`max` over nothing + // NULL — "there is nothing to minimise" — never the `0` that reads as a + // real measurement. + expect(r.rows).toEqual([{ category: 'x', latest: null }]); + }); + + it('a `Date` operand (the mongo storage form) comes back as the Date, ordered as an instant', () => { + const CUBE = { + name: 'e', sql: 'expense', + dimensions: {}, + measures: { latest: { name: 'latest', type: 'max', sql: 'at' } }, + } as unknown as Cube; + const early = new Date('2026-05-03T00:00:00.000Z'); + const late = new Date('2026-07-27T00:00:00.000Z'); + const r = evaluateAnalyticsQueryOverRows({ measures: ['latest'], dimensions: [] }, CUBE, [ + { at: early }, { at: late }, + ]); + // `String(new Date())` sorts AFTER every '2026-…' string, which is why the + // ordering goes through `compare`'s instant arm rather than String(). + expect(r.rows[0].latest).toBe(late); + }); +}); + +describe('#16203 — the rest of the closed vocabulary, answered rather than assumed', () => { + it('`count` — a row count, numeric, both paths', async () => { + const live = await grid(false); + const preview = await grid(true); + expect(preview.byCategory.travel.expense_count).toBe(3); + expect(preview.byCategory.meals.expense_count).toBe(2); + expect(preview.byCategory.travel.expense_count).toBe(live.byCategory.travel.expense_count); + }); + + it('`count_distinct` — a cardinality, nulls excluded; the arm used to be unreachable', async () => { + const live = await grid(false); + const preview = await grid(true); + // Pre-fix the switch spelled this `countDistinct`, which no producer mints + // (`dataset-compiler` copies the spec's `count_distinct` through), so the + // measure fell to the numeric `default` and answered a SUM of the payer + // strings' coercions — i.e. the row count — instead of the cardinality. + expect(preview.byCategory.travel.distinct_payers).toBe(2); + expect(preview.byCategory.meals.distinct_payers).toBe(1); + expect(preview.byCategory.travel.distinct_payers).toBe(live.byCategory.travel.distinct_payers); + expect(preview.byCategory.meals.distinct_payers).toBe(live.byCategory.meals.distinct_payers); + }); + + it('`sum` / `avg` over a NUMERIC operand — untouched', async () => { + const live = await grid(false); + const preview = await grid(true); + expect(preview.byCategory.travel.total_amount).toBe(2500); + expect(preview.byCategory.travel.avg_amount).toBeCloseTo(2500 / 3, 10); + expect(preview.byCategory.travel.total_amount).toBe(live.byCategory.travel.total_amount); + expect(preview.byCategory.travel.avg_amount).toBe(live.byCategory.travel.avg_amount); + }); + + it('⛔ `sum` / `avg` over a TEMPORAL operand is left EXACTLY as it was — #16099 owns it', async () => { + const live = await grid(false); + const preview = await grid(true); + // What the code does today, stated rather than defended: the preview drops + // every non-finite operand, so the sum of a date column is the identity `0` + // and its average is `0` as well. + expect(preview.byCategory.travel.sum_spent_on).toBe(0); + expect(preview.byCategory.travel.avg_spent_on).toBe(0); + // The live face answers a DIFFERENT number (SQLite applies numeric affinity + // to the TEXT column and sums the leading years). Neither number is a date, + // and no layer refuses the pair — which is #16099, not this card. The + // inequality is asserted rather than the dialect's exact figure, so this + // stays a statement about the MISSING REFUSAL rather than a pin on SQLite. + expect(live.byCategory.travel.sum_spent_on).not.toBe(preview.byCategory.travel.sum_spent_on); + }); + + it('derived measures — this change does not reach `computeDerived`, it changes what it reads', async () => { + const live = await grid(false); + const preview = await grid(true); + // A derived ratio over numeric operands is unaffected on either path. + expect(preview.byCategory.travel.amount_per_expense).toBeCloseTo(2500 / 3, 10); + expect(preview.byCategory.travel.amount_per_expense).toBe(live.byCategory.travel.amount_per_expense); + // A derived ratio whose operand is a temporal `max` is `null` — `num()` in + // `computeDerived` answers null for a non-finite operand, and a missing + // operand makes the whole ratio null. Pre-fix the preview handed it the + // spurious `0` and it computed `0` there while the live path said null. + expect(preview.byCategory.travel.latest_per_expense).toBeNull(); + expect(preview.byCategory.travel.latest_per_expense).toBe(live.byCategory.travel.latest_per_expense); + }); +}); + +describe('#16203 (b) — a dimension column is described by its OWN type, not always `string`', () => { + it('a time dimension is `time` on preview, the same word the live producer mints', async () => { + const live = await grid(false, ['spent_on']); + const preview = await grid(true, ['spent_on']); + expect(preview.fields.spent_on.type).toBe('time'); + expect(preview.fields.spent_on.type).toBe(live.fields.spent_on.type); + }); + + it('a string dimension keeps `string`, and a measure column keeps the `number` its producer mints', async () => { + const live = await grid(false); + const preview = await grid(true); + expect(preview.fields.category.type).toBe('string'); + expect(preview.fields.category.type).toBe(live.fields.category.type); + // ⛔ Deliberately NOT corrected here: a MEASURE column's type is minted + // `number` by every producer in the platform and corrected at the + // descriptor pass by `measureResultType` (#15768/#16101). Putting a second + // copy of that rule in the evaluator would be two implementations of one + // rule, free to drift — and the live producer beside it does the same. + expect(preview.fields.latest_spend.type).toBe('number'); + expect(preview.fields.latest_spend.type).toBe(live.fields.latest_spend.type); + }); +}); diff --git a/packages/services/service-analytics/src/preview-evaluator.ts b/packages/services/service-analytics/src/preview-evaluator.ts index cbe2c62df4..2edec66595 100644 --- a/packages/services/service-analytics/src/preview-evaluator.ts +++ b/packages/services/service-analytics/src/preview-evaluator.ts @@ -12,7 +12,7 @@ // $between/$in/$nin/$contains, $and/$or/$not) // • timeDimensions date-range filtering + granularity bucketing // (day/week/month/quarter/year) -// • group-by dimensions; count / countDistinct / sum / avg / min / max +// • group-by dimensions; count / count_distinct / sum / avg / min / max // • order + limit/offset // Anything beyond (joins via `include`, raw SQL) falls back to the caller's // normal execution path — the preview simply doesn't claim it. @@ -154,20 +154,121 @@ export function bucketDate(value: unknown, granularity: string, timezone?: strin // ── Aggregation ───────────────────────────────────────────────────────────── -function aggregate(rows: Row[], metricType: string, field: string): number { - if (metricType === 'count' || field === '*') { - if (metricType === 'countDistinct') { - return new Set(rows.map((r) => r[field]).filter((v) => v != null)).size; +/** + * Read an operand as a number, or `null` for "this is not a number" — the + * question `Number()` cannot be asked, because it answers `NaN` for a date and + * `0` for `''` and `null` alike. + * + * Numeric TEXT counts (`'800'`): a seed row carries whatever the draft was + * authored with, and a numeric column written as text is still a numeric + * column — ordering it lexicographically would put `'1200'` before `'800'`, + * which no backend does for that column. A `Date` is deliberately NOT numeric + * here: `Number(new Date())` is an epoch integer, and comparing one against a + * money amount is a category error rather than an ordering. + */ +function numericOperand(v: unknown): number | null { + if (typeof v === 'number') return Number.isFinite(v) ? v : null; + if (typeof v === 'string' && v.trim() !== '') { + const n = Number(v); + return Number.isFinite(n) ? n : null; + } + return null; +} + +/** + * Order two operands of ONE measure for `min`/`max`. + * + * Numbers (in either spelling) order numerically; everything else falls to + * {@link compare}, this file's shared ordering — so ISO dates order as dates, + * a BSON `Date` orders as its instant against wire text, and text orders the + * way `MIN(text_col)` orders on a SQL face. + * + * Deliberately NOT folded into {@link compare} itself: that primitive also + * decides `where` filtering and `order`, whose comparand comes from the QUERY + * rather than from a sibling row, so widening it would move populations this + * card never measured. + */ +function compareOperands(a: unknown, b: unknown): number { + const an = numericOperand(a); + const bn = numericOperand(b); + if (an !== null && bn !== null) return an - bn; + return compare(a, b); +} + +/** + * `min`/`max` over one group: the winning operand **in its own type**. + * + * NULLs are skipped (every SQL face aggregates over non-null values), and a + * group with nothing left answers `null` — `emptyGroupValueFor` + * (`@objectstack/spec/data`) rules `min`/`max` over nothing unanswerable, never + * the `0` that reads as a measurement somebody made. + */ +function extremumOf(rows: Row[], field: string, kind: 'min' | 'max'): unknown { + let winner: unknown; + let seen = false; + for (const r of rows) { + const v = r[field]; + if (v == null) continue; + if (!seen) { + winner = v; + seen = true; + continue; } - return rows.length; + const c = compareOperands(v, winner); + if (kind === 'min' ? c < 0 : c > 0) winner = v; } + return seen ? winner : null; +} + +/** + * One measure over one group. + * + * `metricType` is the cube metric's `type` — `AggregationMetricType` + * (`spec/data/analytics.zod.ts`), which `dataset-compiler` fills with the + * dataset measure's own `aggregate` verbatim. That vocabulary is CLOSED, so + * every member is answered here rather than left to fall through (#16203): + * + * | metric type | answer | + * |:-----------------|:---------------------------------------------------------| + * | `count` | the row count — numeric whatever it counted | + * | `count_distinct` | the cardinality of the non-null values — numeric | + * | `sum` / `avg` | arithmetic over the operands that read as numbers | + * | `min` / `max` | the winning operand, IN ITS OWN TYPE ({@link extremumOf}) | + * | `number` / `string` / `boolean` | a custom-SQL metric the dataset path never mints — left on the historical numeric `default` | + * + * ⭐ `min`/`max` are why this function stopped returning `number`. Coercing + * every operand with `Number()` and dropping the non-finite ones made a + * `max` over a `date` field answer `0` — not a mislabelled column but a + * DIFFERENT, WRONG ANSWER to the same query, silently, on the draft-preview + * path only. The same value comes back as `'2026-05-12'` from the live path + * over the same rows. `cross-object-rebucket.ts` settled the identical + * question for the recombination path (#3797): the value `min`/`max` picks is + * a value OF the column, so it has to come back in the shape the row carried. + * + * ⛔ `sum`/`avg` over a TEMPORAL operand is left exactly as it was — the + * non-finite operands drop and the answer is the numeric identity. There is no + * defined answer to invent (the SQL faces disagree with each other on it), and + * #16099 is the open card for REFUSING an incoherent aggregate/field-type pair + * — the layer that refuses is that card's ruling, not this file's. + * + * ⛔ `count`/`count_distinct` stay numeric. Counting `date`s is still counting; + * the sibling descriptor rule (`measure-result-type.ts`) answers the same way. + */ +function aggregate(rows: Row[], metricType: string, field: string): unknown { + // `count`, and any metric aggregating over rows rather than a column. + if (metricType === 'count' || field === '*') return rows.length; const nums = rows.map((r) => Number(r[field])).filter((n) => Number.isFinite(n)); switch (metricType) { - case 'countDistinct': return new Set(rows.map((r) => r[field]).filter((v) => v != null)).size; + // The spec's spelling (`AggregationFunction`), which is what the compiler + // copies through. It used to be spelled `countDistinct` here — a word no + // producer mints — so the arm was UNREACHABLE and the measure fell to the + // numeric `default` below, answering a sum of coerced values (or a row + // count) under the author's `count_distinct` name. + case 'count_distinct': return new Set(rows.map((r) => r[field]).filter((v) => v != null)).size; case 'sum': return nums.reduce((a, b) => a + b, 0); case 'avg': return nums.length ? nums.reduce((a, b) => a + b, 0) / nums.length : 0; - case 'min': return nums.length ? Math.min(...nums) : 0; - case 'max': return nums.length ? Math.max(...nums) : 0; + case 'min': return extremumOf(rows, field, 'min'); + case 'max': return extremumOf(rows, field, 'max'); default: return nums.length ? nums.reduce((a, b) => a + b, 0) : rows.length; } } @@ -250,7 +351,19 @@ export function evaluateAnalyticsQueryOverRows( return { rows: limited, fields: [ - ...dimensions.map((d) => ({ name: d, type: 'string' })), + // A dimension column is described by the CUBE dimension's own type — the + // same expression `NativeSQLStrategy.buildFieldMeta` and its ObjectQL + // sibling use (`d?.type || 'string'`), so a `date` dataset dimension is + // `'time'` here exactly as it is on the live path. Minting `'string'` for + // every dimension made the same column two different things depending + // only on whether a pending seed draft existed (#16203 (b)). + ...dimensions.map((d) => ({ name: d, type: String(cube.dimensions?.[d]?.type || 'string') })), + // ⛔ A MEASURE column keeps the `'number'` every producer in the platform + // mints for it, live faces included. Correcting it is one rule owned by + // `measureResultType` (#15768/#16101) and applied at the ADR-0021 + // descriptor pass; a second copy of it here would be two implementations + // free to drift, over a question this producer cannot answer anyway (it + // has the cube, not the source object's declared field types). ...query.measures.map((m) => ({ name: m, type: 'number' })), ], }; From 00e99d9d7561f8c744d0c098f3bc3de0b26aa340 Mon Sep 17 00:00:00 2001 From: os-dev Date: Sun, 6 Sep 2026 06:41:18 +0000 Subject: [PATCH 2/2] chore(changeset): minor for the draft-preview aggregate operand type fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Graded `minor` on the precedent of the descriptor half of the same question (the `measureResultType` changeset, also `minor`): this changes what `POST /api/v1/analytics/dataset/query` RETURNS on the draft-preview path — a `min`/`max` over a non-numeric field, a `count_distinct`, and a dimension column's declared type. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y --- .../analytics-preview-min-max-operand-type.md | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 .changeset/analytics-preview-min-max-operand-type.md diff --git a/.changeset/analytics-preview-min-max-operand-type.md b/.changeset/analytics-preview-min-max-operand-type.md new file mode 100644 index 0000000000..bcf28144a4 --- /dev/null +++ b/.changeset/analytics-preview-min-max-operand-type.md @@ -0,0 +1,25 @@ +--- +"@objectstack/service-analytics": minor +--- + +A draft-preview `min`/`max` answers the operand's own type instead of `0`, and a preview dimension column is described by its own type + +`POST /api/v1/analytics/dataset/query` has two producers of one response: the engine, and — when the request renders the as-if-published world over a pending seed draft (ADR-0037 P3) — `evaluateAnalyticsQueryOverRows`. The second one coerced every aggregate operand with `Number()` and dropped the non-finite ones, so a `min` / `max` over a non-numeric field answered `0`. Measured on one dataset and one row set, with two services differing only in whether a pending seed draft exists: + +``` +live {"category":"travel","latest_spend":"2026-05-12"} +preview {"category":"travel","latest_spend":0} +``` + +That is not a mislabelled column: it is a different, wrong answer to the same query, with no refusal and no warning, on the path an author is looking at *while* authoring the dataset. + +What changed, per member of the closed `AggregationFunction` vocabulary: + +- **`min` / `max` return the winning operand in its own type.** Ordering goes through this file's shared `compare` — so an ISO date orders as a date, a BSON `Date` orders as its instant against wire text, and text orders the way `MIN(text_col)` does on a SQL face — with a numeric arm so a numeric column written as text (`'800'`) still orders numerically. `cross-object-rebucket.ts` settled the identical question for the recombination path: the value these two pick is a value OF the column, so it must come back in the shape the row carried. +- **A group whose operand is null throughout answers `null`, not `0`** — `emptyGroupValueFor` (`@objectstack/spec/data`) rules `min` / `max` over nothing unanswerable, and `0` reads as a measurement nobody made. +- **`count_distinct` answers a cardinality again.** Its arm was spelled `countDistinct`, a word no producer mints (`dataset-compiler` copies the spec's `count_distinct` through), so it was unreachable and the measure fell to the numeric default — answering a row count under the author's `count_distinct` name (measured: `3` where the live path says `2`). +- **`count` stays a row count and `sum` / `avg` stay arithmetic.** Counting dates is still counting. +- **`sum` / `avg` over a TEMPORAL operand is deliberately unchanged.** There is no defined answer — the SQL faces do not agree on one either — and refusing an incoherent aggregate/field-type pair is an open decision, not this fix's to invent. +- **A dimension column is typed from the cube dimension**, the same expression both live producers use (`d?.type || 'string'`), so a `date` dataset dimension is `time` on the preview path as it already was on the live one. A MEASURE column keeps the `number` every producer mints; correcting that is the ADR-0021 descriptor pass's one rule, not a second copy here. + +Derived measures are untouched: `computeDerived` still coerces with `Number()` and answers `null` for a non-finite operand — but a derived ratio over a temporal `min` / `max` now sees a date instead of the spurious `0`, so it answers `null` on the preview path exactly as it already did on the live one.