Skip to content

A number/string/boolean metric's SQL expression is replaced by COUNT(*) #4157

Description

@os-zhuang

Found while making the aggregate vocabularies lockstep (#4153). Left unfixed there on purpose — it is a behavioural change in an analytics SQL path, not a derivation cleanup.

The defect

NativeSQLStrategy.resolveMeasureSql wraps a measure's SQL according to its type. After #4153 that is a table lookup with a fallback:

const wrap = AGGREGATE_SQL[measure.type];
return wrap ? wrap(col) : `COUNT(*)`;

AGGREGATE_SQL covers the six aggregates. But AggregationMetricType (packages/spec/src/data/analytics.zod.ts:21) — the vocabulary Metric.type is validated against — has nine members:

'count', 'sum', 'avg', 'min', 'max', 'count_distinct',
'number',  // Custom SQL expression returning a number
'string',  // Custom SQL expression returning a string
'boolean'  // Custom SQL expression returning a boolean

For those last three the measure's sql is the whole expression — a ratio, a CASE, a window function. Nothing should be wrapped around it. Instead the expression is discarded and the query returns COUNT(*): a row count where the author declared a computed value. No error, no log.

Reachability

Not reachable from a dataset. dataset-compiler.ts's aggregateToMetricType only ever returns an AggregationFunction member, and throws for a measure with no aggregate — so a compiled dataset's metrics are always one of the six.

It needs a hand-authored Cube reaching NativeSQLStrategy with a type: 'number' measure. Whether any exists today is the first thing to establish; the spec explicitly invites them ("Custom SQL expression returning a number"), so the surface is advertised.

Suggested fix

Return col for number/string/boolean — the expression as authored, unwrapped — and reserve the fallback for a genuinely unknown type, where a throw is more honest than a plausible-looking number (the argument #3948 settled for the drivers).

The care needed is in aggregation context: a custom expression used as a measure in a GROUP BY query must itself be aggregate-shaped, or the SQL is invalid. So the fix probably wants a test matrix over grouped/ungrouped and the three types, which is exactly why it did not belong in #4153.

Worth checking at the same time whether the !measure early return (return 'COUNT(*)' for a member that resolves to no measure at all) should also be a throw. Same silent-plausible-answer shape.

Refs #4153, objectstack-ai/objectui#2945, #3948

Activity

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

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions