fix(analytics)!: a measure emits what it declares, instead of COUNT(*) (#4157) - #4184
Merged
Conversation
#4157) resolveMeasureSql answered `COUNT(*)` to three questions it could not otherwise answer, each time aliased under the name the caller asked for so the result looked like an answer: 1. A measure the cube does not declare. lookupMember's synthetic relation fallback is dimension-only, so any undeclared or mistyped measure landed here — `measures: ['revenue']` returned `COUNT(*) AS "revenue"`. 2. A number/string/boolean metric. AggregationMetricType documents these as "Custom SQL expression returning a …": the measure's `sql` IS the computation. It was discarded and replaced by a row count. 3. An unrecognised `type`. Same silent substitution. Now the first and third throw — naming the declared measures, and both accepted vocabularies — and a custom-expression type emits its expression unwrapped. The six aggregates are unchanged. Also: a dot no longer implies a relationship hop. qualifyAndRegisterJoin split any dotted string into a join chain, so `SUM(account.amount)` became `"SUM(account"."amount)"` plus a `LEFT JOIN "SUM(account"` — invalid SQL naming a table that does not exist. Harmless only while the result was discarded for COUNT(*). A dotted string is a path only when every segment is a bare identifier, so `account.amount` still lowers to a qualified column and a join. That also fixes the same mangling for an AGGREGATE measure whose sql is an expression. Breaking, narrowly: two inputs that used to produce SQL now raise. Both were returning a wrong number rather than data, so nothing correct can depend on them — the trade #3948 settled for the drivers. Datasets are unaffected; aggregateToMetricType only ever emits an AggregationFunction member, so the reachable path is a hand-authored Cube. metric-type-coverage.test.ts asserts the aggregate and expression sets PARTITION AggregationMetricType, so a tenth type fails a test rather than reaching the throw. Both sets are named rather than derived as each other's complement — deriving would classify a new aggregate as an expression and emit a bare column. 460 tests / 35 files green, including the four suites asserting COUNT(*) (all use a declared `type: 'count'` metric). The 14 new tests were confirmed failing against the old behaviour before the fix. Closes #4157. Refs #4153, #3948, objectstack-ai/objectui#2945 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📓 Docs Drift CheckThis PR changes 1 package(s): 6 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #4157.
COUNT(*)was the answer to three different questionsNativeSQLStrategy.resolveMeasureSqlcould not answer three inputs, and returnedCOUNT(*)to each — aliased under the name the caller asked for, so the result looked like an answer rather than a failure:COUNT(*) AS "revenue"— a row count presented as revenuenumber/string/booleanmetrictypeThe first is the one most likely to bite:
lookupMember's synthetic relation fallback is dimension-only (if (kind === 'dimension')), so any undeclared or mistyped measure name reached it.The second is what the spec explicitly invites —
AggregationMetricTypedocuments those three as "Custom SQL expression returning a number / string / boolean". The measure'ssqlis the computation: a ratio, aCASE, a window function. There is nothing to wrap; the only correct emission is the expression.Now
A dot no longer implies a relationship hop
qualifyAndRegisterJoinsplit any dotted string into a join chain. So the expressionSUM(account.amount)became:Invalid SQL plus a phantom join. That was harmless only while the result was being thrown away for
COUNT(*)— emitting the expression makes it matter, so the fix is a precondition, not a bonus.A dotted string is now a path only when every segment is a bare identifier.
account.amountstill lowers to"account"."amount"with itsLEFT JOIN; an expression is emitted as written. This also fixes the same mangling for an aggregate measure whosesqlis an expression —type: 'sum'withsql: 'SUM(account.amount)'was producing the same garbage, independently of #4157.Breaking, narrowly — and deliberately
Two inputs that used to produce SQL now raise: a query naming an undeclared measure, and a cube measure whose type is outside
AggregationMetricType.Both were returning a wrong number rather than data, so nothing correct can depend on them. A caller who was silently getting row counts will now see an error — which is the point, and the same trade #3948 settled for the drivers.
Datasets are unaffected.
aggregateToMetricTypeonly ever emits anAggregationFunctionmember and throws for a measure with no aggregate, so a compiled dataset never had a custom-expression measure or an unknown type. The reachable path is a hand-authored Cube.The guard
metric-type-coverage.test.tsasserts the aggregate and expression sets partitionAggregationMetricType, so a tenth metric type fails a test rather than reaching the throw.Both sets are named, not derived as each other's complement. Deriving would have classified a new aggregate the spec grows (
median, …) as an expression and emitted a bare column — a different silent wrong answer. Same reasoning as #4153, which named the aggregate set for the same reason.Verification
COUNT(*). All four use a declaredtype: 'count'metric ({ type: 'count', sql: '*' }), so none of them relied on a fallback; that is what makes this safe.tsc --noEmit: 0 errors in the three touched files, with workspace deps built with types.eslint --no-inline-config: clean.One judgement left in place on purpose: in a grouped query a custom expression must itself be aggregate-shaped, because measures never join
GROUP BY(only dimensions do). That is the author's contract to keep — silently substitutingCOUNT(*)was not keeping it for them, and the code comment now says so.Refs #4153, #3948, objectstack-ai/objectui#2945
🤖 Generated with Claude Code