codeant-ai-for-open-source[bot] commented on code in PR #42216:
URL: https://github.com/apache/superset/pull/42216#discussion_r3649484417
##########
tests/unit_tests/models/helpers_test.py:
##########
@@ -3800,3 +3800,213 @@ 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_filter_by_adhoc_column_label_resolves_to_sql_expression(
+ database: Database,
+) -> None:
+ """
+ Regression for #38339: when a column label is renamed in the Table chart
+ (e.g. "CustomerID" → "Id"), the search filter sends {"col":"Id",...} but
+ the backend ``columns_by_name`` dict is keyed by physical names. The filter
+ must resolve the adhoc column label to its underlying SQL expression.
+ """
+ from superset.connectors.sqla.models import SqlaTable, TableColumn
+
+ table = SqlaTable(
+ database=database,
+ schema=None,
+ table_name="employees",
+ columns=[
+ TableColumn(column_name="CustomerID", type="TEXT"),
+ TableColumn(column_name="FirstName", type="TEXT"),
+ TableColumn(column_name="LastName", type="TEXT"),
+ ],
+ )
+
+ # Simulates the Table chart sending a renamed column as an adhoc column
+ # and a search filter using the display label.
+ sqla_query = table.get_sqla_query(
+ columns=[
+ {"label": "Id", "sqlExpression": "CustomerID", "expressionType":
"SQL"},
+ "FirstName",
+ "LastName",
+ ],
+ filter=[{"col": "Id", "op": "ILIKE", "val": "C001%"}],
+ is_timeseries=False,
+ row_limit=10,
+ )
+
+ with database.get_sqla_engine() as engine:
+ sql = str(
+ sqla_query.sqla_query.compile(
+ dialect=engine.dialect,
+ compile_kwargs={"literal_binds": True},
+ )
+ )
+
+ # The filter should resolve to the underlying column name
+ assert "WHERE" in sql, f"Expected WHERE clause, got SQL: {sql}"
+ assert "CustomerID" in sql, f"Expected filter on 'CustomerID', got SQL:
{sql}"
+ assert "'C001%'" in sql, f"Expected filter value 'C001%', got SQL: {sql}"
+ # The filter should NOT be rejected
+ assert not sqla_query.rejected_filter_columns, (
+ f"Expected no rejected filters, got:
{sqla_query.rejected_filter_columns}"
+ )
+ assert "Id" in sqla_query.applied_filter_columns, (
+ f"Expected 'Id' in applied filters, got:
{sqla_query.applied_filter_columns}"
+ )
+
+
+def test_adhoc_column_label_filter_not_in_rejected_columns(
+ database: Database,
+) -> None:
+ """
+ Regression for #38339: filter columns that match an adhoc column label
+ must not appear in rejected_filter_columns.
+ """
+ from superset.connectors.sqla.models import SqlaTable, TableColumn
+
+ table = SqlaTable(
+ database=database,
+ schema=None,
+ table_name="t",
+ columns=[TableColumn(column_name="real_col", type="TEXT")],
+ )
+
+ sqla_query = table.get_sqla_query(
+ columns=[
+ {"label": "MyLabel", "sqlExpression": "real_col",
"expressionType": "SQL"},
+ ],
+ filter=[{"col": "MyLabel", "op": "==", "val": "x"}],
+ is_timeseries=False,
+ row_limit=10,
+ )
+
+ assert "MyLabel" not in sqla_query.rejected_filter_columns
+ assert "MyLabel" in sqla_query.applied_filter_columns
+
+
+def test_unknown_column_still_rejected_without_adhoc_match(
+ database: Database,
+) -> None:
+ """
+ A filter on a column that matches neither a physical name nor an adhoc
+ label should still be rejected.
+ """
+ from superset.connectors.sqla.models import SqlaTable, TableColumn
+
+ table = SqlaTable(
+ database=database,
+ schema=None,
+ table_name="t",
+ columns=[TableColumn(column_name="a", type="TEXT")],
+ )
+
+ sqla_query = table.get_sqla_query(
+ columns=[
+ {"label": "X", "sqlExpression": "a", "expressionType": "SQL"},
+ ],
+ filter=[{"col": "nonexistent", "op": "==", "val": "y"}],
+ is_timeseries=False,
+ row_limit=10,
+ )
+
+ assert "nonexistent" in sqla_query.rejected_filter_columns
+ assert "nonexistent" not in sqla_query.applied_filter_columns
+
+
+def test_mixed_adhoc_and_physical_column_filters(
+ database: Database,
+) -> None:
+ """
+ Both physical-column and adhoc-label filters should work simultaneously.
+ """
+ from superset.connectors.sqla.models import SqlaTable, TableColumn
+
+ table = SqlaTable(
+ database=database,
+ schema=None,
+ table_name="t",
+ columns=[
+ TableColumn(column_name="name", type="TEXT"),
+ TableColumn(column_name="status", type="TEXT"),
+ ],
+ )
+
+ sqla_query = table.get_sqla_query(
+ columns=[
+ {"label": "DisplayName", "sqlExpression": "name",
"expressionType": "SQL"},
+ "status",
+ ],
+ filter=[
+ {"col": "DisplayName", "op": "==", "val": "Alice"},
+ {"col": "status", "op": "==", "val": "active"},
+ ],
+ is_timeseries=False,
+ row_limit=10,
+ )
+
+ with database.get_sqla_engine() as engine:
+ sql = str(
+ sqla_query.sqla_query.compile(
+ dialect=engine.dialect,
+ compile_kwargs={"literal_binds": True},
+ )
+ )
+
+ assert "WHERE" in sql
+ assert "name" in sql
+ assert "status" in sql
+ assert not sqla_query.rejected_filter_columns
+ assert "DisplayName" in sqla_query.applied_filter_columns
+ assert "status" in sqla_query.applied_filter_columns
+
+
+def test_failed_adhoc_resolution_not_in_applied_columns(
+ database: Database,
+) -> None:
+ """
+ When an adhoc column label matches but ``adhoc_column_to_sqla`` raises
+ ``ColumnNotFoundException``, the label must appear in
+ ``rejected_filter_columns`` and NOT in ``applied_filter_columns``.
+ """
+ from unittest.mock import patch as _patch
+
+ from superset.connectors.sqla.models import SqlaTable, TableColumn
+ from superset.exceptions import ColumnNotFoundException
+
+ table = SqlaTable(
+ database=database,
+ schema=None,
+ table_name="t",
+ columns=[TableColumn(column_name="real_col", type="TEXT")],
+ )
+
+ def raise_on_bad_label(col, force_type_check=False,
template_processor=None):
+ if getattr(col, "label", None) == "BadLabel":
Review Comment:
**Suggestion:** The mocked argument `col` is a dictionary, as shown by the
subsequent `col.get(...)` call, so `getattr(col, "label", None)` always returns
`None`. Consequently, the `BadLabel` branch never raises
`ColumnNotFoundException`, and this test does not exercise failed adhoc-column
resolution; inspect the label using the dictionary key instead. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Failed adhoc-resolution regression path is not exercised.
- ⚠️ Future resolution regressions may pass this test undetected.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run `test_failed_adhoc_resolution_not_in_applied_columns()` at
`tests/unit_tests/models/helpers_test.py:3966`.
2. The test patches `SqlaTable.adhoc_column_to_sqla` with
`raise_on_bad_label()` at
`tests/unit_tests/models/helpers_test.py:3994-3996`.
3. `get_sqla_query()` is called with the `BadLabel` adhoc column and filter
at
`tests/unit_tests/models/helpers_test.py:3997-4008`, and the mocked `col`
value is later
treated as a dictionary by `col.get("sqlExpression", "")` at line 3991.
4. At line 3987, `getattr(col, "label", None)` reads an attribute rather
than the
dictionary key, so it returns `None` and never raises
`ColumnNotFoundException` for
`BadLabel`; the test therefore does not exercise the intended
failed-resolution path
before checking the applied and rejected columns at lines 4010-4012.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=760c3027073f4b0d8786fb0bbf24adde&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=760c3027073f4b0d8786fb0bbf24adde&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/models/helpers_test.py
**Line:** 3987:3987
**Comment:**
*Possible Bug: The mocked argument `col` is a dictionary, as shown by
the subsequent `col.get(...)` call, so `getattr(col, "label", None)` always
returns `None`. Consequently, the `BadLabel` branch never raises
`ColumnNotFoundException`, and this test does not exercise failed adhoc-column
resolution; inspect the label using the dictionary key instead.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42216&comment_hash=cb347aae14f635eab9d9418386880ab4081bb3c05d0838210b2fff8ffefde961&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42216&comment_hash=cb347aae14f635eab9d9418386880ab4081bb3c05d0838210b2fff8ffefde961&reaction=dislike'>👎</a>
--
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]
