EnxDev commented on code in PR #44715:
URL: https://github.com/apache/superset/pull/44715#discussion_r4137512752
##########
superset/models/helpers.py:
##########
@@ -2268,6 +2269,47 @@ def is_alias_used_in_orderby(col: ColumnElement) -> bool:
if is_alias_used_in_orderby(col):
col.name = f"{col.name}__"
+ def rename_shadowing_aliases(self, qry: Select) -> None:
+ """
+ Rename SELECT aliases that would shadow a source column, in place.
+
+ Some engines (e.g. ClickHouse) resolve an identifier to a SELECT alias
+ before a source column of the same name, in every clause. With an alias
+ like `DATE_TRUNC('day', ts) AS ts`, a `WHERE ts >= ...` then filters on
+ the truncated value and a `GROUP BY DATE_TRUNC('day', ts)` truncates
the
+ alias again. An alias is renamed when it names a column of the
+ datasource, or a column its own expression reads, and its expression is
+ not simply that column. The final output columns keep their names, as
+ they are updated by `labels_expected` after querying.
+ """
+ if not self.db_engine_spec.select_alias_shadows_source_column:
+ return
+
+ try:
+ column_names = set(self.column_names)
+ except NotImplementedError:
+ column_names = set()
+
+ def expression_text(element: ColumnElement) -> str | None:
+ try:
+ return str(element.compile(compile_kwargs={"literal_binds":
True}))
+ except Exception: # pylint: disable=broad-except
+ return None
+
+ quotes = "\"`'"
+ for select in [e for e in visitors.iterate(qry) if isinstance(e,
Select)]:
+ for col in select.selected_columns:
+ if not isinstance(col, Label) or not isinstance(col.name, str):
+ continue
+ name = col.name
+ expression = expression_text(col.element)
+ if expression is None or expression.strip().strip(quotes) ==
name:
+ continue
+ unquoted = re.sub(f"[{quotes}]", "", expression)
+ reads_it = re.search(rf"(?<![\w.]){re.escape(name)}(?!\w)",
unquoted)
+ if name in column_names or reads_it:
+ col.name = f"{name}__"
Review Comment:
`__` is also the series-limit subquery's inner suffix, and its ON clause
(line 5688) references that alias bare. With a series limit and an adhoc
dimension labelled after its column, your SQLite harness compiles to
`UPPER(label) AS label__ ... JOIN (SELECT UPPER(label) AS label__, ...) AS
series_limit ON UPPER(label) = label__`.
If ClickHouse binds that `label__` to the outer SELECT alias (haven't run it
live), the ON is always true and metrics come back multiplied by the series
limit. Could the join use `series_limit.c[col_name]`, or the rename pick a
suffix that can't collide? A series-limit case in
`select_alias_shadowing_test.py` would pin 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: [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]
