dosubot[bot] commented on issue #38913: URL: https://github.com/apache/superset/issues/38913#issuecomment-4164246834
<!-- Answer --> Good question — there is a viable SQLGlot-based fix here. The core problem is that [`adhoc_metric_to_sqla()`](https://github.com/apache/superset/blob/829e4d92d91ceae4b43b1ed3b063ffe45377799c/superset/models/helpers.py#L1120-L1128) wraps Custom SQL metrics as bare `literal_column(expression)` with zero aggregate validation, while SIMPLE metrics always go through `sqla_aggregations[aggregate](column)` (guaranteed SUM/COUNT/etc). When Deck.gl charts [add spatial columns to GROUP BY](https://github.com/apache/superset/blob/829e4d92d91ceae4b43b1ed3b063ffe45377799c/superset/viz.py#L1699-L1706), the non-aggregate Custom SQL breaks. SQLGlot 26.x (which Superset already pins) has `exp.AggFunc` as the base class for all aggregate functions. A new utility could leverage this: ```python # superset/sql/parse.py def sql_expression_has_aggregate(expression: str, engine: str = "base") -> bool: """Detect if a SQL expression contains a top-level aggregate function.""" try: dialect = SQLGLOT_DIALECTS.get(engine) parsed = sqlglot.parse_one(f"SELECT {expression}", dialect=dialect) for node in parsed.walk(): if isinstance(node, exp.Subquery): continue if isinstance(node, exp.AggFunc): return True return False except Exception: return True # conservative fallback ``` `isinstance(node, exp.AggFunc)` reliably catches SUM, COUNT, MAX, MIN, AVG, STDDEV, ArrayAgg, etc. — `GREATEST()` correctly returns `False` since it's a scalar function. This class [isn't used anywhere in the codebase today](https://github.com/apache/superset/blob/829e4d92d91ceae4b43b1ed3b063ffe45377799c/superset/sql/parse.py) but is fully available. With that utility, there are two complementary fix paths: **Option A — Validation error (stricter, general):** In `get_sqla_query()` around where [`need_groupby` is determined](https://github.com/apache/superset/blob/829e4d92d91ceae4b43b1ed3b063ffe45377799c/superset/models/helpers.py#L1504), validate that Custom SQL metrics contain an aggregate when GROUP BY is active, and raise a `QueryObjectValidationError` with a helpful message like *"Custom SQL expression 'GREATEST(confirmed, predicted)' does not contain an aggregate function. Wrap it in an aggregate, e.g.: MAX(GREATEST(confirmed, predicted))"*. This fixes it for all chart types. **Option B — Auto-wrap at the viz layer (backward-compatible, localized):** In `BaseDeckGLViz.query_obj()`, detect non-aggregate Custom SQL and auto-wrap it in `MAX()` with a `logger.warning`. This is less disruptive since it only affects Deck.gl charts and doesn't break existing saved charts. Though it's worth noting `BaseDeckGLViz` and subclasses are already marked `@deprecated(deprecated_in='3.0')`. Option A is the cleaner long-term fix since this same gap affects other Deck.gl chart types too. Option B is safest for a quick backward-compatible patch. Both could be combined — auto-wrap now, validate later. One caveat: the `sql_expression_has_aggregate()` utility would need to handle edge cases like window functions (`ROW_NUMBER() OVER (...)`) which aren't GROUP BY aggregates but could appear in Custom SQL. <!-- Dosu Comment Footer --> *To reply, just mention [@dosu](https://go.dosu.dev/dosubot?utm_source=github&utm_medium=bot-comment&utm_campaign=github-assistant&utm_content=brand-link).* --- <sup>How did I do? [Good](https://app.dosu.dev/response-feedback/481300e4-d70f-4354-ad93-79547d3b463d?feedback_type=great_response&utm_source=github&utm_medium=bot-comment&utm_campaign=github-assistant&utm_content=feedback-great_response) | [Irrelevant](https://app.dosu.dev/response-feedback/481300e4-d70f-4354-ad93-79547d3b463d?feedback_type=irrelevant_answer&utm_source=github&utm_medium=bot-comment&utm_campaign=github-assistant&utm_content=feedback-irrelevant_answer) | [Incorrect](https://app.dosu.dev/response-feedback/481300e4-d70f-4354-ad93-79547d3b463d?feedback_type=incorrect_sources&utm_source=github&utm_medium=bot-comment&utm_campaign=github-assistant&utm_content=feedback-incorrect_sources) | [Verbose](https://app.dosu.dev/response-feedback/481300e4-d70f-4354-ad93-79547d3b463d?feedback_type=too_verbose&utm_source=github&utm_medium=bot-comment&utm_campaign=github-assistant&utm_content=feedback-too_verbose) | [Hallucination](https://app.dosu.dev/response-feedback/481300e4-d70f -4354-ad93-79547d3b463d?feedback_type=hallucination&utm_source=github&utm_medium=bot-comment&utm_campaign=github-assistant&utm_content=feedback-hallucination) | [Report 🐛](https://app.dosu.dev/response-feedback/481300e4-d70f-4354-ad93-79547d3b463d?feedback_type=bug_report&utm_source=github&utm_medium=bot-comment&utm_campaign=github-assistant&utm_content=feedback-bug_report) | [Other](https://app.dosu.dev/response-feedback/481300e4-d70f-4354-ad93-79547d3b463d?feedback_type=other&utm_source=github&utm_medium=bot-comment&utm_campaign=github-assistant&utm_content=feedback-other)</sup> [](https://app.dosu.dev/a28d3c7e-a9d3-459e-9fb6-3a6f9ff4f357/ask?utm_source=github&utm_medium=bot-comment&utm_campaign=github-assistant&utm_content=chat-badge) [](https://cursor.com/link/prompt?text=Good%20question%20%E2%80%94%20there%20is%20a%20viable%20SQLGlot-based%20fix%20here.%20The%20core%20problem%20is%20that%20%5B%60adhoc_metric_to_sqla%28%29%60%5D%28https%3A//github.com/apache/superset/blob/829e4d92d91ceae4b43b1ed3b063ffe45377799c/superset/models/helpers.py%23L1120-L1128%29%20wraps%20Custom%20SQL%20metrics%20as%20bare%20%60literal_column%28expression%29%60%20with%20zero%20aggregate%20validation%2C%20while%20SIMPLE%20metrics%20always%20go%20through%20%60sqla_aggregations%5Baggregate%5D%28column%29%60%20%28guaranteed%20SUM/COUNT/etc%29.%20When%20Deck.gl%20charts%20%5Badd%20spatial%20columns%20to%20GROUP%20BY%5D%28https%3A//github.com/apache/superset/blob/829e4d9 2d91ceae4b43b1ed3b063ffe45377799c/superset/viz.py%23L1699-L1706%29%2C%20the%20non-aggregate%20Custom%20SQL%20breaks.%0A%0ASQLGlot%2026.x%20%28which%20Superset%20already%20pins%29%20has%20%60exp.AggFunc%60%20as%20the%20base%20class%20for%20all%20aggregate%20functions.%20A%20new%20utility%20could%20leverage%20this%3A%0A%0A%60%60%60python%0A%23%20superset/sql/parse.py%0Adef%20sql_expression_has_aggregate%28expression%3A%20str%2C%20engine%3A%20str%20%3D%20%22base%22%29%20-%3E%20bool%3A%0A%20%20%20%20%22%22%22Detect%20if%20a%20SQL%20expression%20contains%20a%20top-level%20aggregate%20function.%22%22%22%0A%20%20%20%20try%3A%0A%20%20%20%20%20%20%20%20dialect%20%3D%20SQLGLOT_DIALECTS.get%28engine%29%0A%20%20%20%20%20%20%20%20parsed%20%3D%20sqlglot.parse_one%28f%22SELECT%20%7Bexpression%7D%22%2C%20dialect%3Ddialect%29%0A%20%20%20%20%20%20%20%20for%20node%20in%20parsed.walk%28%29%3A%0A%20%20%20%20%20%20%20%20%20%20%20%20if%20isinstance%28node%2C%20exp.Subquery%29%3A%0A%20%20%20%20%20%20%20%20 %20%20%20%20%20%20%20%20continue%0A%20%20%20%20%20%20%20%20%20%20%20%20if%20isinstance%28node%2C%20exp.AggFunc%29%3A%0A%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20return%20True%0A%20%20%20%20%20%20%20%20return%20False%0A%20%20%20%20except%20Exception%3A%0A%20%20%20%20%20%20%20%20return%20True%20%20%23%20conservative%20fallback%0A%60%60%60%0A%0A%60isinstance%28node%2C%20exp.AggFunc%29%60%20reliably%20catches%20SUM%2C%20COUNT%2C%20MAX%2C%20MIN%2C%20AVG%2C%20STDDEV%2C%20ArrayAgg%2C%20etc.%20%E2%80%94%20%60GREATEST%28%29%60%20correctly%20returns%20%60False%60%20since%20it%27s%20a%20scalar%20function.%20This%20class%20%5Bisn%27t%20used%20anywhere%20in%20the%20codebase%20today%5D%28https%3A//github.com/apache/superset/blob/829e4d92d91ceae4b43b1ed3b063ffe45377799c/superset/sql/parse.py%29%20but%20is%20fully%20available.%0A%0AWith%20that%20utility%2C%20there%20are%20two%20complementary%20fix%20paths%3A%0A%0A%2A%2AOption%20A%20%E2%80%94%20Validation%20error%20%28stricter%2C%20general%29% 3A%2A%2A%20In%20%60get_sqla_query%28%29%60%20around%20where%20%5B%60need_groupby%60%20is%20determined%5D%28https%3A//github.com/apache/superset/blob/829e4d92d91ceae4b43b1ed3b063ffe45377799c/superset/models/helpers.py%23L1504%29%2C%20validate%20that%20Custom%20SQL%20metrics%20contain%20an%20aggregate%20when%20GROUP%20BY%20is%20active%2C%20and%20raise%20a%20%60QueryObjectValidationError%60%20with%20a%20helpful%20message%20like%20%2A%22Custom%20SQL%20expression%20%27GREATEST%28confirmed%2C%20predicted%29%27%20does%20not%20contain%20an%20aggregate%20function.%20Wrap%20it%20in%20an%20aggregate%2C%20e.g.%3A%20MAX%28GREATEST%28confirmed%2C%20predicted%29%29%22%2A.%20This%20fixes%20it%20for%20all%20chart%20types.%0A%0A%2A%2AOption%20B%20%E2%80%94%20Auto-wrap%20at%20the%20viz%20layer%20%28backward-compatible%2C%20localized%29%3A%2A%2A%20In%20%60BaseDeckGLViz.query_obj%28%29%60%2C%20detect%20non-aggregate%20Custom%20SQL%20and%20auto-wrap%20it%20in%20%60MAX%28%29%60%20with%20a%20%60logger.warn ing%60.%20This%20is%20less%20disruptive%20since%20it%20only%20affects%20Deck.gl%20charts%20and%20doesn%27t%20break%20existing%20saved%20charts.%20Though%20it%27s%20worth%20noting%20%60BaseDeckGLViz%60%20and%20subclasses%20are%20already%20marked%20%60%40deprecated%28deprecated_in%3D%273.0%27%29%60.%0A%0AOption%20A%20is%20the%20cleaner%20long-term%20fix%20since%20this%20same%20gap%20affects%20other%20Deck.gl%20chart%20types%20too.%20Option%20B%20is%20safest%20for%20a%20quick%20backward-compatible%20patch.%20Both%20could%20be%20combined%20%E2%80%94%20auto-wrap%20now%2C%20validate%20later.%0A%0AOne%20caveat%3A%20the%20%60sql_expression_has_aggregate%28%29%60%20utility%20would%20need%20to%20handle%20edge%20cases%20like%20window%20functions%20%28%60ROW_NUMBER%28%29%20OVER%20%28...%29%60%29%20which%20aren%27t%20GROUP%20BY%20aggregates%20but%20could%20appear%20in%20Custom%20SQL.) [](https://go. dosu.dev/discord-bot?utm_source=github&utm_medium=bot-comment&utm_campaign=github-assistant&utm_content=join-discord) [](https://twitter.com/intent/tweet?text=%40dosu_ai%20helped%20me%20solve%20this%20issue!&url=https%3A//github.com/apache/superset/issues/38913) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
