rusackas commented on code in PR #41961:
URL: https://github.com/apache/superset/pull/41961#discussion_r3564825739
##########
tests/unit_tests/models/helpers_test.py:
##########
@@ -3410,3 +3410,52 @@ def fake_engine(*args, **kwargs):
# ```forecasts.original`.`total_cost``` (the regression), so this negative
# assertion catches the actual failure mode, not just an exact-string
match.
assert "`forecasts.original`" not in sql
+
+
+def test_simple_metric_quotes_column_requiring_quoting(database: Database) ->
None:
+ """
+ Regression for #30637: a SIMPLE adhoc metric that aggregates a column whose
+ name requires identifier quoting (e.g. it contains a space) must render the
+ column with the dialect's quote characters, otherwise the emitted SQL is
+ invalid for dialects that need quoting.
+
+ This exercises the metric compilation path named in the issue
+ (``adhoc_metric_to_sqla`` -> ``TableColumn.get_sqla_col`` -> ``column()``),
+ asserting the identifier is quoted rather than emitted bare.
+ """
+ from superset.connectors.sqla.models import SqlaTable, TableColumn
+ from superset.utils.core import AdhocMetricExpressionType
+
+ column_name = "Amount HT"
+ table = SqlaTable(
+ database=database,
+ schema=None,
+ table_name="test_table",
+ columns=[TableColumn(column_name=column_name, type="INTEGER")],
+ )
+ columns_by_name = {col.column_name: col for col in table.columns}
Review Comment:
These are locals inside a test; we don't annotate every one, and mypy is
green after the real fix on line 3439. Leaving as-is, thanks.
##########
tests/unit_tests/models/helpers_test.py:
##########
@@ -3410,3 +3410,52 @@ def fake_engine(*args, **kwargs):
# ```forecasts.original`.`total_cost``` (the regression), so this negative
# assertion catches the actual failure mode, not just an exact-string
match.
assert "`forecasts.original`" not in sql
+
+
+def test_simple_metric_quotes_column_requiring_quoting(database: Database) ->
None:
+ """
+ Regression for #30637: a SIMPLE adhoc metric that aggregates a column whose
+ name requires identifier quoting (e.g. it contains a space) must render the
+ column with the dialect's quote characters, otherwise the emitted SQL is
+ invalid for dialects that need quoting.
+
+ This exercises the metric compilation path named in the issue
+ (``adhoc_metric_to_sqla`` -> ``TableColumn.get_sqla_col`` -> ``column()``),
+ asserting the identifier is quoted rather than emitted bare.
+ """
+ from superset.connectors.sqla.models import SqlaTable, TableColumn
+ from superset.utils.core import AdhocMetricExpressionType
+
+ column_name = "Amount HT"
+ table = SqlaTable(
+ database=database,
+ schema=None,
+ table_name="test_table",
+ columns=[TableColumn(column_name=column_name, type="INTEGER")],
+ )
+ columns_by_name = {col.column_name: col for col in table.columns}
+
+ metric: AdhocMetric = {
+ "expressionType": AdhocMetricExpressionType.SIMPLE,
+ "aggregate": "SUM",
+ "column": {"column_name": column_name},
+ "label": "total",
+ }
+
+ sqla_metric = table.adhoc_metric_to_sqla(metric, columns_by_name)
Review Comment:
Same here, this is just a test-local dict and we don't require type
annotations on those. mypy passes now that 3439 is fixed, so I'll leave it.
Thanks!
--
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]
