fix(inputs): stop baking DISTINCT into data-driven option queries - #3328
Open
rinkeraven-helloprint wants to merge 3 commits into
Open
Conversation
The five data-driven inputs built their value column as the string
`DISTINCT <col> as value`. processColumnExpression keeps that whole string
as `sqlWithoutAlias`, and hasAgg only matches an aggregate name followed by
`(`, so the column was classified as a plain dimension and the quantifier
was copied into the GROUP BY:
SELECT DISTINCT store AS "value" FROM ga4_sessions
WHERE (store IS NOT NULL) GROUP BY DISTINCT store ORDER BY store
`GROUP BY DISTINCT <expr>` is legal Postgres 14+ (it de-duplicates grouping
sets), so most warehouses tolerated the redundant clause. Cube's
DataFusion-based parser rejects it — "Expected: joined table, found: store" —
so every data-driven dropdown rendered "No options found".
The GROUP BY that generateSQLQuery already emits for non-aggregate columns
is what makes the options distinct, so the keyword was redundant everywhere:
drop it and let the GROUP BY do the de-duplication.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A blanket `not.toMatch(/\bDISTINCT\b/)` over the whole query asserted more than the fix. DISTINCT is legal elsewhere: `shouldAddDistinct` exists to put one on the SELECT list, and `IS NOT DISTINCT FROM` is the codebase's own null-safe equality, which an author's `where` could contain. Assert the invariant that actually broke Cube — no quantifier in the GROUP BY. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…fier guard Two review findings. StringValueSelector's comment claimed the COUNT(*) forces the GROUP BY on the value column. It doesn't: willUseGroupBy is unconditional (sql-options.ts:532), so the GROUP BY is emitted whether or not an aggregate is present — as the other four call sites show. The COUNT(*) is there for minimum_records. Reworded to match its siblings. The call-site guard only matched a quantifier inline in the argument to processColumnExpression, so assigning the string to a variable first slipped past, as did the ALL quantifier (which fails in DataFusion identically). Match the construction shape instead, and strip comments first since the call sites legitimately discuss DISTINCT in prose. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@rinkeraven-helloprint is attempting to deploy a commit to the Evidence Team on Vercel. A member of the Team first needs to authorize it. |
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.
Personal Message
Hi @hughess posting this PR as a potential fix for an issue I also reported in Slack. When testing the new OSS Evidence I ran into the problem that dropdown filters do not work with the Cube SQL API because the generated SQL adds DISTINCT in the GROUP BY clause which Cube SQL API does not accept. Dropdown values do not populate because of this. Code in this PR is AI-driven but I did confirm locally that dropdowns work correctly after this fix. Let me know if creating PRs this way is desired or if you prefer me to only post issues if I run into future problems. Thanks!
Everything below AI-generated:
Problem
Every data-driven input is broken against Cube — dropdowns render "No options found"
and the options query 500s:
Root cause
The inputs build the value column with the quantifier baked into the string:
processColumnExpressionkeeps the remainder assqlWithoutAlias(DISTINCT store);hasAggonly matches an aggregate name followed by(, so the column is classified as aplain dimension; and
generateSQLQuerycopies every non-aggregate column'ssqlWithoutAliasinto the grouping list (sql-options.ts:583) — yieldingGROUP BY DISTINCT store.That form is valid in PostgreSQL 14+ (it de-duplicates grouping sets), so most engines
tolerated the redundant clause. Cube's DataFusion parser rejects it. It's also invalid
T-SQL, so Fabric was affected too.
Fix
The
GROUP BYwas already doing the de-duplication, so the keyword was pure redundancy.It's emitted unconditionally —
willUseGroupBy = !config.skipGroupBy(
sql-options.ts:532), and onlyimage/build-image-sql.tssetsskipGroupBy— and everyoption config keeps a non-aggregate column, so the clause is never empty.
Applied to all five call sites:
dropdown,button_group,input_tabs,table_filter(
StringValueSelector), andrepeat(build-repeat-query-config.ts).dimension_gridhand-writes its own SQL and is unaffected.
Behaviour is otherwise unchanged: the alias stays
value,hasAggstaysfalse, andORDER BY handling is unaffected. On
GROUP BY ALLdialects (ClickHouse, Snowflake,BigQuery, Databricks, MotherDuck) only the redundant
SELECT DISTINCTgoes; on explicitones (Postgres, Cube, Fabric) the grouping list loses the quantifier.
SQLQueryConfig.shouldAddDistinct(declared:146, honoured:679) would add aquery-level
DISTINCT, but it's dead code andSELECT DISTINCT … GROUP BY …is redundantby construction, so I left it unused. Happy to wire it up or delete it if maintainers
prefer.
Testing
New
core/src/user-components/common/option-query-group-by.test.tsasserts across alleight dialects that the options query emits a non-empty
GROUP BYwith no set quantifierin it. Scoped to that clause on purpose:
DISTINCTis legal elsewhere (shouldAddDistinct,and
IS NOT DISTINCT FROMfromnullSafeEqual). A second guard scans the call sites, sincethe expression is a template string no SQL assertion can see into; it matches the
construction shape and covers
ALLtoo, and is mutation-tested against four evasions.Expectations updated in
build-repeat-query-config.test.tsandstring-value-selector-order.test.ts.Known follow-up (not in this PR)
hasAggstill doesn't recognise a bare quantifier, so an author-written<BarChart x="distinct category" />still producesGROUP BY distinct categoryand thesame Cube error. Closing that class means stripping or rejecting the quantifier at
sql-options.ts:583— wider blast radius, so I kept it separate. Glad to follow up.