codeant-ai-for-open-source[bot] commented on code in PR #38716:
URL: https://github.com/apache/superset/pull/38716#discussion_r3662057004
##########
tests/unit_tests/models/helpers_test.py:
##########
@@ -4465,3 +4465,228 @@ def
test_get_sqla_query_calculated_column_inlined_in_raw_records(
assert "CASE WHEN a > 0" in sql
assert "'positive'" in sql
assert "'non-positive'" in 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="TEXT"),
+ 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)
+ sql_upper = sql.upper()
+ assert "WHERE" in sql_upper
+ assert " LIKE " in sql_upper
+ assert "CUSTOMERID" in sql_upper
Review Comment:
**Suggestion:** The regression test does not verify that the filter uses the
resolved adhoc expression: checking only for `CUSTOMERID` would also pass if
the implementation generated an unrelated or incorrectly quoted predicate.
Assert the compiled predicate against the expected resolved expression and
bound value so this test catches the original behavior rather than merely
confirming that some `WHERE` clause exists. [code quality]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Regression test may miss incorrect adhoc filter resolution.
- ⚠️ Table-chart search behavior can regress undetected.
- ⚠️ SQL predicate and selected expression are not distinguished.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run `test_filter_adhoc_column` in
`tests/unit_tests/models/helpers_test.py:4470`, which
builds a query with the adhoc label `Id` mapped to the SQL expression
`CustomerId` at
lines 4490-4492 and filters on `Id` at lines 4497-4499.
2. `get_sqla_query` processes unresolved filter labels through
`find_adhoc_column_and_convert_to_sqla` at
`superset/superset/models/helpers.py:4029-4032`, so the production behavior
depends on the
returned expression being used in the predicate.
3. The test converts the entire query to a string at
`tests/unit_tests/models/helpers_test.py:4505-4507` and only checks that
some `WHERE`,
`LIKE`, and `CUSTOMERID` text exists at lines 4508-4510.
4. Change the implementation or test setup so `CustomerId` appears in the
selected SQL
while the filter predicate uses another expression; these assertions can
still pass
because they do not inspect the compiled `whereclause` or bind value.
Compile the
predicate with literal binds, or assert the compiled `whereclause`, to prove
that
`CustomerId LIKE 'C001%'` is the actual filter.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1e92b79c29f64b05989be12ac815a2ce&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=1e92b79c29f64b05989be12ac815a2ce&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:** 4508:4510
**Comment:**
*Code Quality: The regression test does not verify that the filter uses
the resolved adhoc expression: checking only for `CUSTOMERID` would also pass
if the implementation generated an unrelated or incorrectly quoted predicate.
Assert the compiled predicate against the expected resolved expression and
bound value so this test catches the original behavior rather than merely
confirming that some `WHERE` clause exists.
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%2F38716&comment_hash=b225198acbdc607305fc747cec18109afae3d668c0b33dde0db4a30be35046ef&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38716&comment_hash=b225198acbdc607305fc747cec18109afae3d668c0b33dde0db4a30be35046ef&reaction=dislike'>👎</a>
##########
tests/unit_tests/models/helpers_test.py:
##########
@@ -4465,3 +4465,228 @@ def
test_get_sqla_query_calculated_column_inlined_in_raw_records(
assert "CASE WHEN a > 0" in sql
assert "'positive'" in sql
assert "'non-positive'" in 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="TEXT"),
+ 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)
+ sql_upper = sql.upper()
+ assert "WHERE" in sql_upper
+ assert " LIKE " in sql_upper
+ assert "CUSTOMERID" 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)
Review Comment:
**Suggestion:** This test only checks that the result is a `ColumnElement`,
which does not prove that the matching adhoc column's `sqlExpression` was
selected. A resolver returning any SQLAlchemy column expression would satisfy
the test, so assert the compiled expression or SQL text for `CustomerId` to
validate the lookup contract. [code quality]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Incorrect adhoc label resolution can pass unit tests.
- ❌ Filters may target the wrong database column.
- ⚠️ Table-chart search results can become inaccurate.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run `test_find_adhoc_column_and_convert_to_sqla_found` in
`tests/unit_tests/models/helpers_test.py:4513`, where the columns list maps
label `Id` to
`CustomerId` at lines 4531-4534.
2. The test calls `find_adhoc_column_and_convert_to_sqla` with label `Id` at
`tests/unit_tests/models/helpers_test.py:4538-4542`; this helper scans adhoc
labels and
delegates conversion to `adhoc_column_to_sqla` at
`superset/superset/models/helpers.py:3100-3144`.
3. The assertion at `tests/unit_tests/models/helpers_test.py:4544-4548`
checks only that
the result is a non-null `ColumnElement`. Any unrelated SQLAlchemy
expression, including
an expression for another adhoc column, satisfies this contract.
4. Return or select an expression such as `CustomerName` while retaining the
`ColumnElement` type; the test still passes even though the `Id` lookup is
wrong. Compile
`result` and assert that it contains the resolved `CustomerId` expression to
validate the
lookup contract.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=082b85b94681488491ab96ed1834d4e2&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=082b85b94681488491ab96ed1834d4e2&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:** 4544:4548
**Comment:**
*Code Quality: This test only checks that the result is a
`ColumnElement`, which does not prove that the matching adhoc column's
`sqlExpression` was selected. A resolver returning any SQLAlchemy column
expression would satisfy the test, so assert the compiled expression or SQL
text for `CustomerId` to validate the lookup contract.
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%2F38716&comment_hash=f8f8914ce66abdc133e40fc3bf5ba1b47b6544c68a70b25935a8c335fcd34b78&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38716&comment_hash=f8f8914ce66abdc133e40fc3bf5ba1b47b6544c68a70b25935a8c335fcd34b78&reaction=dislike'>👎</a>
##########
superset/models/helpers.py:
##########
@@ -3097,6 +3097,40 @@ def adhoc_column_to_sqla(
) -> tuple[ColumnElement, Optional[GenericDataType]]:
raise NotImplementedError()
+ def find_adhoc_column_and_convert_to_sqla(
+ self,
+ columns: list[Column],
+ label: str,
+ template_processor: Optional[BaseTemplateProcessor] = None,
+ ) -> Optional[ColumnElement]:
+ """
+ Find an adhoc column by its label and convert it to a SQLAlchemy
column.
+
+ This helper method searches through a list of columns for an adhoc
column
+ with a matching label and converts it to a SQLAlchemy column
expression.
+
+ Args:
+ columns: List of columns to search through
+ label: The label to match against adhoc columns
+ template_processor: Optional template processor for SQL templating
+
+ Returns:
+ SQLAlchemy column element if found, None otherwise
+ """
+ adhoc_col = None
+ for c in columns:
+ if utils.is_adhoc_column(c):
+ if c.get("label") == label:
+ adhoc_col = c
+ break
Review Comment:
**Suggestion:** The helper returns the first adhoc column with a matching
label, while the existing `adhoc_columns_by_label` map used by the order-by
path overwrites duplicate labels and resolves them to the last definition. When
duplicate labels are present, filtering and ordering therefore use different
SQL expressions, yielding inconsistent query results. Make both paths use the
same duplicate-label resolution rule. [inconsistent naming]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Duplicate labels can produce inconsistent WHERE and ORDER BY expressions.
- ⚠️ Table results may be sorted by a different adhoc expression.
- ⚠️ Query behavior becomes dependent on duplicate-column ordering.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Submit a chart query whose `columns` list contains two adhoc column
definitions with
the same `label` but different `sqlExpression` values, and include that
label in both
`filter` and `orderby`; `get_sqla_query()` receives both lists in the
table-chart query
path around `superset/models/helpers.py:3678`.
2. During order-by preparation, `adhoc_columns_by_label` is populated at
`superset/models/helpers.py:3666-3672`; assigning the same key repeatedly
means the last
duplicate definition wins.
3. The order-by branch at `superset/models/helpers.py:3711-3715` therefore
converts the
last expression for the duplicate label.
4. The label-based filter branch instead calls the new helper at
`superset/models/helpers.py:4029-4033`; its loop at
`superset/models/helpers.py:3121-3125`
stops on the first matching label, so the WHERE expression can differ from
the ORDER BY
expression and produce inconsistent filtered and sorted results.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=648c1e12b09e44dea9b2eeead06fc2a2&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=648c1e12b09e44dea9b2eeead06fc2a2&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:** superset/models/helpers.py
**Line:** 3121:3125
**Comment:**
*Inconsistent Naming: The helper returns the first adhoc column with a
matching label, while the existing `adhoc_columns_by_label` map used by the
order-by path overwrites duplicate labels and resolves them to the last
definition. When duplicate labels are present, filtering and ordering therefore
use different SQL expressions, yielding inconsistent query results. Make both
paths use the same duplicate-label resolution rule.
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%2F38716&comment_hash=d432bb7418d916f01a061c059006dc3b4f95acd30ddfc6df9b272f77569a6e44&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38716&comment_hash=d432bb7418d916f01a061c059006dc3b4f95acd30ddfc6df9b272f77569a6e44&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]
