Copilot commented on code in PR #38716:
URL: https://github.com/apache/superset/pull/38716#discussion_r2955035679
##########
tests/unit_tests/models/helpers_test.py:
##########
@@ -1856,3 +1856,226 @@ def test_extras_having_is_parenthesized(
assert "(COUNT(*) > 0 OR 1 = 1)" in sql, (
f"extras.having should be wrapped in parentheses. Generated SQL:
{sql}"
)
+
+
+def test_filter_adhoc_column(database: Database) -> None:
+ """
+ Test that filter works with adhoc column labels.
+ When filter contains a string that matches the label of an adhoc column
+ in the columns list, it should correctly convert to a SQLAlchemy column
+ instead of raising QueryObjectValidationError.
+ """
+ from superset.connectors.sqla.models import SqlaTable, TableColumn
+
+ table = SqlaTable(
+ table_name="test_table",
+ database=database,
+ columns=[
+ TableColumn(column_name="CustomerId", type="NUMERIC"),
+ TableColumn(column_name="FullName", type="TEXT"),
+ ],
+ )
+
+ # Should not raise QueryObjectValidationError
+ result = table.get_sqla_query(
+ columns=[
+ {"expressionType": "SQL", "label": "Id", "sqlExpression":
"CustomerId"},
+ "FullName",
+ ],
+ orderby=[],
+ metrics=[],
+ extras={},
+ filter=[
+ {"col": "Id", "op": "ILIKE", "val": "C001%"}
+ ], # Filter by adhoc column label
+ granularity=None,
+ is_timeseries=False,
+ )
+ assert result is not None
+
+ # Verify the SQL contains the expression from the adhoc column
+ sql = str(result.sqla_query)
+ assert "WHERE" in sql.upper()
+ assert "LIKE" in sql.upper()
+
+
+def test_find_adhoc_column_and_convert_to_sqla_found(database: Database) ->
None:
+ """
+ Test find_adhoc_column_and_convert_to_sqla when adhoc column is found.
+
+ The method should find an adhoc column by label and return a SQLAlchemy
column.
+ """
+ from superset.connectors.sqla.models import SqlaTable, TableColumn
+
+ table = SqlaTable(
+ database=database,
+ schema=None,
+ table_name="t",
+ columns=[
+ TableColumn(column_name="CustomerId"),
+ TableColumn(column_name="CustomerName"),
+ ],
+ )
+
+ # List of columns including an adhoc column
+ columns = [
+ {"expressionType": "SQL", "label": "Id", "sqlExpression":
"CustomerId"},
+ "CustomerName",
+ ]
+
+ # Find the adhoc column by label
+ result = table.find_adhoc_column_and_convert_to_sqla(
+ columns=columns,
+ label="Id",
+ template_processor=None,
+ )
+
+ # Should return a SQLAlchemy column element
+ assert result is not None
+ from sqlalchemy.sql.elements import ColumnElement
+
+ assert isinstance(result, ColumnElement)
+
+
+def test_find_adhoc_column_and_convert_to_sqla_not_found(database: Database)
-> None:
+ """
+ Test find_adhoc_column_and_convert_to_sqla when adhoc column is not found.
+
+ The method should return None when no matching adhoc column exists.
+ """
+ from superset.connectors.sqla.models import SqlaTable, TableColumn
+
+ table = SqlaTable(
+ database=database,
+ schema=None,
+ table_name="t",
+ columns=[
+ TableColumn(column_name="CustomerId"),
+ TableColumn(column_name="CustomerName"),
+ ],
+ )
+
+ # List of columns without the target adhoc column
+ columns = [
+ {"expressionType": "SQL", "label": "salary", "sqlExpression":
"Salary"},
+ "CustomerName",
+ ]
+
+ # Try to find a non-existent adhoc column
+ result = table.find_adhoc_column_and_convert_to_sqla(
+ columns=columns,
+ label="nonexistent_col",
+ template_processor=None,
+ )
+
+ # Should return None
+ assert result is None
+
+
+def test_find_adhoc_column_and_convert_to_sqla_empty_columns(
+ database: Database,
+) -> None:
+ """
+ Test find_adhoc_column_and_convert_to_sqla with empty columns list.
+
+ The method should return None when the columns list is empty.
+ """
+ from superset.connectors.sqla.models import SqlaTable, TableColumn
+
+ table = SqlaTable(
+ database=database,
+ schema=None,
+ table_name="t",
+ columns=[
+ TableColumn(column_name="CustomerId"),
+ ],
+ )
+
+ # Empty columns list
+ columns: list[TableColumn] = []
+
+ # Try to find an adhoc column in empty list
+ result = table.find_adhoc_column_and_convert_to_sqla(
+ columns=columns,
+ label="any_label",
Review Comment:
This test uses `columns: list[TableColumn] = []`, but
`find_adhoc_column_and_convert_to_sqla()` expects the query-object column shape
(strings and/or adhoc column dicts), not model `TableColumn` objects. Tighten
the annotation to the correct type (eg `list[Column]` from
`superset.superset_typing`, or `list[AdhocColumn | str]`) to avoid confusing
type-checkers/readers.
##########
tests/unit_tests/models/helpers_test.py:
##########
@@ -1856,3 +1856,226 @@ def test_extras_having_is_parenthesized(
assert "(COUNT(*) > 0 OR 1 = 1)" in sql, (
f"extras.having should be wrapped in parentheses. Generated SQL:
{sql}"
)
+
+
+def test_filter_adhoc_column(database: Database) -> None:
+ """
+ Test that filter works with adhoc column labels.
+ When filter contains a string that matches the label of an adhoc column
+ in the columns list, it should correctly convert to a SQLAlchemy column
+ instead of raising QueryObjectValidationError.
+ """
+ from superset.connectors.sqla.models import SqlaTable, TableColumn
+
+ table = SqlaTable(
+ table_name="test_table",
+ database=database,
+ columns=[
+ TableColumn(column_name="CustomerId", type="NUMERIC"),
+ TableColumn(column_name="FullName", type="TEXT"),
+ ],
+ )
+
+ # Should not raise QueryObjectValidationError
+ result = table.get_sqla_query(
+ columns=[
+ {"expressionType": "SQL", "label": "Id", "sqlExpression":
"CustomerId"},
+ "FullName",
+ ],
+ orderby=[],
+ metrics=[],
+ extras={},
+ filter=[
+ {"col": "Id", "op": "ILIKE", "val": "C001%"}
+ ], # Filter by adhoc column label
+ granularity=None,
+ is_timeseries=False,
+ )
+ assert result is not None
+
+ # Verify the SQL contains the expression from the adhoc column
+ sql = str(result.sqla_query)
+ assert "WHERE" in sql.upper()
+ assert "LIKE" in sql.upper()
Review Comment:
`CustomerId` is declared as `NUMERIC` in this test but the filter uses
`ILIKE`, which is a string operation and can generate invalid SQL on some
engines (even if the test only stringifies the query). Consider making the
column `TEXT` (or using a non-like operator) and strengthening the assertion to
verify the WHERE clause actually references the adhoc expression (eg contains
`CustomerId`) rather than only checking for generic `WHERE`/`LIKE` substrings.
##########
superset/models/helpers.py:
##########
@@ -3029,6 +3058,14 @@ def get_sqla_query( # pylint:
disable=too-many-arguments,too-many-locals,too-ma
template_processor=template_processor
)
is_metric_filter = True
+ elif col_obj is None and isinstance(flt_col, str):
+ sqla_col = self.find_adhoc_column_and_convert_to_sqla(
+ columns=columns,
+ label=flt_col,
+ template_processor=template_processor,
+ )
+ if sqla_col is not None:
+ applied_adhoc_filters_columns.append(flt_col)
Review Comment:
When a string filter column is resolved to an adhoc column via
`find_adhoc_column_and_convert_to_sqla()`, the label (e.g. "Id") is appended to
`applied_adhoc_filters_columns`. However, the later `rejected_filter_columns`
computation only checks membership in `self.column_names` /
`applied_template_filters`, so the same label will still be reported as
rejected in the payload even though it was applied. Update the rejected/applied
filter column bookkeeping to treat resolved adhoc labels as applied (eg exclude
them from `rejected_filter_columns`).
--
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]
