You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "Vitor-Avila (via GitHub)" <gi...@apache.org> on 2025/05/01 16:59:42 UTC
[PR] fix: Edge case with metric not getting quoted in sort by when normalize_columns is enabled [superset]
Vitor-Avila opened a new pull request, #33337:
URL: https://github.com/apache/superset/pull/33337
### SUMMARY
When using Snowflake, it's possible to enable the `normalize_columns` setting at the dataset level to have all columns as lowercase. With this setting enabled, in case you use a metric in the chart that has the same key as a column that exists in the dataset (but it's not used in the chart), you would get a SQL error as the metric is used on sorting by default, and the metric name won't be quoted. Superset would generate:
``` sql
SELECT DATE_TRUNC('DAY', time) AS "time", COUNT(*) AS "count_lower"
FROM public.new GROUP BY DATE_TRUNC('DAY', time) ORDER BY count_lower DESC
LIMIT 1000;
```
As opposed to:
``` sql
SELECT DATE_TRUNC('DAY', time) AS "time", COUNT(*) AS "count_lower"
FROM public.new GROUP BY DATE_TRUNC('DAY', time) ORDER BY "count_lower" DESC
LIMIT 1000;
```
This only happens with this setting enabled. If you disable it and have the columns in uppercase, the exact same chart configuration works.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No UI changes.
### TESTING INSTRUCTIONS
1. Create a table in Snowflake.
2. Create a dataset for it.
3. Enable `normalize_columns` on the dataset.
4. Sync columns so they revert to lowercase.
5. Create a metric in the dataset with a key named after a column from the table.
6. Create a chart using this metric, and another column.
7. Validate the metric name is properly quoted in the `ORDER BY` statement.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Edge case with metric not getting quoted in sort by when normalize_columns is enabled [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on PR #33337:
URL: https://github.com/apache/superset/pull/33337#issuecomment-2845360711
tagging @hughhhh as I believe you worked on this logic in the past
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Edge case with metric not getting quoted in sort by when normalize_columns is enabled [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #33337:
URL: https://github.com/apache/superset/pull/33337#discussion_r2072217252
##########
superset/models/helpers.py:
##########
@@ -1597,6 +1593,10 @@ def get_sqla_query( # pylint: disable=too-many-arguments,too-many-locals,too-ma
template_processor=template_processor
)
need_groupby = True
+ elif col in columns_by_name:
+ col = self.convert_tbl_column_to_sqla_col(
+ columns_by_name[col], template_processor=template_processor
+ )
Review Comment:
I'm basically just changing the order to first evaluate the column against metrics, and then across columns.
My understanding of the issue is that in the current `if/elif` ordering, we would first evaluate `col` across `columns_by_name`, which would be true (even tho it's a metric), and then `convert_tbl_column_to_sqla_col()` does `col = sa.column(tbl_column.column_name, type_=type_)` which seems to return quoted if the column is uppercase (hence the issue does not happen with `normalize_columns` disabled):
``` python
import sqlalchemy as sa
print(sa.column("test", sa.String))
print(sa.column("TEST", sa.String))
```
``` bash
test
"TEST"
```
With `normalize_columns`, we send a lowercase column label to `sa.column()` which then does not quote it.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Edge case with metric not getting quoted in sort by when normalize_columns is enabled [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on PR #33337:
URL: https://github.com/apache/superset/pull/33337#issuecomment-2845358736
this is mostly changing the "order of evaluation" (to check if the column is a metric first). No tests failed, but I'm not 100% sure if this order of eval has any importance. @betodealmeida @eschutho @mistercrunch @michael-s-molina @villebro any thoughts?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Edge case with metric not getting quoted in sort by when normalize_columns is enabled [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch merged PR #33337:
URL: https://github.com/apache/superset/pull/33337
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org