-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Questions referencing another question with a GROUP BY on a custom column generate invalid SQL #23862
Comments
thanks, which version did you upgrade from? |
We noticed this issue when we upgraded from v0.41.6 to v0.43.4 |
Not flagging as a regression since I went all the way back to 41.7 and hit the bug there as well |
@GriffinSchneider thank you for concise reproduction steps using the Sample Database. I was able to reproduce the issue and am now creating an E2E test following those steps. |
The reason we push expression definitions into a nested query in the first place is that a lot of DB engines have problems if you try to use expression definitions directly in a SELECT
"source"."size" AS "size",
"source"."sum" AS "sum"
FROM (
SELECT
CASE WHEN "PUBLIC"."ORDERS"."TOTAL" > 10 THEN ? ELSE ? END AS "size",
sum("PUBLIC"."ORDERS"."TOTAL") AS "sum"
FROM "PUBLIC"."ORDERS"
GROUP BY CASE WHEN "PUBLIC"."ORDERS"."TOTAL" > 10 THEN ? ELSE ? END
ORDER BY CASE WHEN "PUBLIC"."ORDERS"."TOTAL" > 10 THEN ? ELSE ? END ASC
) "source" LIMIT 2000 I'm guessing the parameters are indeed all the same but it's not smart enough to figure out that We've probably never ran into this bug in the past because it would only pop up when you use a parameterized expression like Anyways as @metamben suggested the root problem here is that we should be recursively pushing expressions into subselects but we're currently only doing it at the top level -- seems like a clear bug |
Describe the bug
If I create a query builder question with a custom column like
case(something, "A", "B")
andGROUP BY
on that custom column, and then make another query builder question referencing the first one, the second question generates invalid SQL.Logs
JavaScript console logs
Server logs
To Reproduce
Steps to reproduce the behavior:
In the sample database, create a question with the query builder like this:
The custom
size
column is:case([TOTAL] > 10, "Large", "Small")
.This question works as expected.
Create another question that just references the first one and doesn't do anything else:
and click "visualize".
See this error:
Expected behavior
The second question should return the same results as the first one, since it didn't do anything besides reference the first question.
Information about your Metabase Installation:
Severity
A number of our existing questions were broken when we upgraded to v0.43.4 and we haven't found a workaround yet besides converting all affected questions to SQL, which we'd like to avoid. We're considering whether we need to downgrade.
Additional context
In my reproduction example, the query generated by the second question looks like this:
If I run this query replacing all the
?
parameters with the same value, then the query runs successfully. But if I replace the?
with different values such that the values in theSELECT
don't match the values in theGROUP BY
, then I get the error I see in my example. Based on my question, it seems like those?
s should all be getting the same value when Metabase runs the query, but maybe they don't? Or maybe Postgres doesn't know that the values are going to match when it's figuring out whether the query is valid?Also this is a minimal reproduction of the issue, in our actual use case the second referencing question is actually trying to do some data processing, sometimes there's more than one level of question references going on, etc.
Thanks for your help!
The text was updated successfully, but these errors were encountered: