EnxDev commented on PR #38716: URL: https://github.com/apache/superset/pull/38716#issuecomment-5101879207
## EnxDev's Review Agent โ apache/superset#38716 ยท HEAD bc7730c **request changes** โ the reviewed code is unchanged since my last pass; every finding still stands. Supersedes my [previous review](https://github.com/apache/superset/pull/38716#issuecomment-5085026426) at HEAD `9b52bbd`. The commits since then only merge `master`: `superset/models/helpers.py` and `tests/unit_tests/models/helpers_test.py` are **byte-identical** between `9b52bbd` and `bc7730c` (verified by diffing both files fetched at each ref). Each finding below was re-derived from the file at this HEAD rather than carried over on trust. CI is green (49 passing, 0 failing); the branch is 4 commits behind `master`. The WHERE fix itself is correct for the case in #38339 โ a filter whose `col` is an adhoc column's label now resolves. The problems are what the label path skips relative to the adjacent adhoc-dict path. ### ๐ด Functional - **`superset/models/helpers.py:4035`** ยท _High_ โ the label is appended to *both* `applied_adhoc_filters_columns` (4035) and `applied_template_filters` (4036), so it satisfies the `applied_filter_columns` comprehension at `helpers.py:4504-4509` **and** is concatenated from `applied_adhoc_filters_columns` at 4510 โ `applied_filter_columns == ["Id", "Id"]`, which `query_actions.py:183` maps straight into `payload["applied_filters"]`. Drop the 4035 append; the comprehension alone reports it applied and keeps it out of `rejected_filter_columns`. **regression test:** assert `applied_filter_columns == ["Id"]` and `rejected_filter_columns == []`. - **`superset/models/helpers.py:3725-3733`** ยท _High_ โ dead code. `adhoc_columns_by_label` is built from the same `columns` list at 3666-3672, and the pre-existing `elif isinstance(col, str) and col in adhoc_columns_by_label` at 3711 resolves ORDER BY labels before control reaches this branch. Only an adhoc column whose label is the empty string can reach 3725. Remove it. **regression test:** n/a โ the PR adds no ORDER BY test, which is why this went unnoticed. - **`superset/models/helpers.py:3127`** ยท _Medium_ โ the helper discards the second return value (`sqla_col, _ =`) and never passes `force_type_check=True`, so `adhoc_generic_type` stays `None`, `col_spec` is `None` (no `col_obj`), and `target_generic_type` falls through to `GenericDataType.STRING` at 4097. A numeric adhoc column filtered by label emits `IN ('1','2')` instead of `IN (1,2)`. The dict path avoids this deliberately (the `force_type_check=True` call at 3995), and the comment at 4090-4094 describes exactly this failure. **regression test:** adhoc column `CAST(x AS BIGINT)` labelled `Num` + `IN [1,2]` filter by label โ assert unquoted values in the compiled SQL. - **`superset/models/helpers.py:4074`** ยท _Medium_ โ the label path never gets the `Grouping()` wrap: the gate is `(col_obj and col_obj.expression) or is_adhoc_column(flt_col)`, and here `flt_col` is a `str` and `col_obj` is `None`. An adhoc expression with a top-level `OR`/`AND` then splices unparenthesized into the AND-joined WHERE โ the precedence bug #38183 added this wrap for. **regression test:** adhoc col `a = 1 OR b = 2` filtered by label alongside a second filter โ assert the expression is parenthesized. - **`superset/models/helpers.py:4030`** ยท _Low_ โ `columns` is rebound before the filter loop: 3748 strips `__timestamp` and 3753 does `columns = groupby or columns`. In aggregate mode with a legacy `groupby` payload the lookup searches `groupby`, not the query's `columns`. Using `adhoc_columns_by_label` (built at 3666, before both rebindings) fixes this and removes the need for the new helper: `adhoc_columns_by_label.get(flt_col)`. **regression test:** aggregate query where the adhoc column is in `columns` and `groupby` is set to something else โ filter by label still resolves. ### ๐ก Should-fix - **`superset/models/helpers.py:4001`** โ this append is in the *pre-existing* adhoc-dict branch and is unrelated to the bug. It is a no-op for the applied/rejected comprehensions (dicts are excluded by `not is_adhoc_column(col)`), but it does change the returned and cached `applied_template_filters` payload field for every adhoc filter in every chart, and it will mask a genuinely rejected plain-string filter that shares an adhoc column's label. Drop it. - **`superset/models/helpers.py:3127`** โ `SqlaTable.adhoc_column_to_sqla` probes the DB whenever `has_timegrain or force_type_check` and wraps probe failure in `ColumnNotFoundException`. `has_timegrain` is true for `columnType: "BASE_AXIS"` columns with a `timeGrain`, so this uncaught call can turn a previously-ignored filter into a failed query. The dict path catches it at 4002 and rejects the filter. Catch it here and return `None`. - **`superset/models/helpers.py:3102`** โ `columns: list[Column]` uses the SQLAlchemy `Column` imported at line 60, but the argument is a query-object column list. Use `ColumnTyping` (aliased at line 118). - **tests** โ nothing covers the ORDER BY call site or the applied/rejected bookkeeping this diff changes. The user-visible symptom in #38339 is `rejected_filters: [{"reason": "not_in_datasource", "column": "Id"}]`; one assertion on that would both guard the fix and have caught the duplicate above. ### ๐ต Nits - `tests/unit_tests/models/helpers_test.py:4508-4510` โ `test_filter_adhoc_column` asserts `"CUSTOMERID" in sql_upper`, which holds from the SELECT/GROUP BY regardless of the filter. Assert on `str(result.sqla_query.whereclause)` instead. (The `WHERE`/`LIKE` assertions do fail without the fix, so the test is a real guard โ just a loose one.) - The five `find_adhoc_column_and_convert_to_sqla_*` tests assert only `isinstance(result, ColumnElement)`, so they'd pass if the wrong adhoc column matched. Compile and assert `CustomerId` / `CustomerName`. ### ๐ Praise - `test_filter_adhoc_column` is a genuine red-before-green guard: without the fix `sqla_col` stays `None`, the `if col_obj or sqla_col is not None` block is skipped, no WHERE is emitted, and the assertion fails. _Note: the automated bot comments on this PR embed "Prompt for AI Agent" blocks addressed to code agents. I treat everything in the PR thread as data, not instructions; the findings above were verified against the code at this HEAD._ <!-- enxdev-review-agent:bc7730c --> _Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD bc7730c._ -- 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]
