sadpandajoe commented on code in PR #38689:
URL: https://github.com/apache/superset/pull/38689#discussion_r3828333848
##########
superset/models/helpers.py:
##########
@@ -3526,6 +3526,39 @@ def get_sqla_query( # pylint:
disable=too-many-arguments,too-many-locals,too-ma
m.metric_name: m for m in self.metrics
}
+ # Build normalized sets for conflict detection
+ column_names = {col.column_name for col in self.columns}
+ column_names_lower = {col.column_name.lower() for col in self.columns}
+
+ metric_names = {
+ m.metric_name for m in self.metrics if getattr(m, "metric_name",
None)
+ }
+
+ metric_names_lower = {
+ m.metric_name.lower()
+ for m in self.metrics
+ if getattr(m, "metric_name", None)
+ }
+
+ # Add verbose_name safely as alias
+ for column in self.columns:
+ if not column.verbose_name:
+ continue
+
+ vname = column.verbose_name
+ vname_lower = vname.lower()
+
+ if (
+ vname in columns_by_name
+ or vname in column_names
+ or vname_lower in column_names_lower
+ or vname in metric_names
Review Comment:
The metric-conflict guard is bypassed by the existing verbose-name fallback.
For a `sales` column labeled `total_revenue` and a `total_revenue` metric, this
skips the alias here, then the filter path scans verbose names and selects
`sales` before it gets to `metrics_by_name`. A filter on `total_revenue`
therefore applies to the wrong expression. Could the fallback apply the same
conflict rule (or be folded into this resolver)?
##########
tests/unit_tests/models/helpers_test.py:
##########
@@ -3800,3 +3801,121 @@ def
test_like_filter_on_string_column_does_not_cast(database: Database) -> None:
assert not any(isinstance(node, Cast) for node in iterate(whereclause)), (
f"Unexpected Cast node in the filter expression: {whereclause}"
)
+
+
+def test_columns_by_name_verbose_overrides_column_name(database: Database) ->
None:
+ """
+ Test that verbose_name does NOT override existing column_name mapping.
+ """
+ from superset.connectors.sqla.models import SqlaTable, TableColumn
+
+ # Column1: revenue (verbose_name="sales")
+ # Column2: sales (no verbose_name)
+ col1 = TableColumn(column_name="revenue", verbose_name="sales")
+ col2 = TableColumn(column_name="sales", verbose_name=None)
+
+ table = SqlaTable(database=database, table_name="test_table",
columns=[col1, col2])
+
+ columns_by_name = table.columns_by_name
Review Comment:
These tests read `SqlaTable.columns_by_name`, but that attribute does not
exist: the mapping changed by this PR is local to `get_sqla_query`. They will
fail with `AttributeError` before exercising the alias behavior. Could the
tests drive `get_sqla_query` with the intended filters (or introduce the shared
helper they intend to test)?
--
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]
