EnxDev commented on PR #38716: URL: https://github.com/apache/superset/pull/38716#issuecomment-5114579792
## EnxDev's Review Agent — apache/superset#38716 · HEAD 1c78843 **request changes** — third pass, code still unchanged; every finding stands. Supersedes my [previous review](https://github.com/apache/superset/pull/38716#issuecomment-5101879207) at HEAD `bc7730c`. `superset/models/helpers.py` and `tests/unit_tests/models/helpers_test.py` are **byte-identical** between `bc7730c` and `1c78843` (both files fetched at each ref and diffed — empty diff); the commits since are `master` merges. Findings below were re-derived from the file at this HEAD, not carried over. No failing CI checks. The branch is diverged from `master`: 4 ahead, 4 behind. The WHERE fix is correct for #38339 — a filter whose `col` is an adhoc column's label now resolves. The problems remain 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). It is a plain `str`, so it satisfies the comprehension at 4504-4509 (`not is_adhoc_column(col)` and `col in applied_template_filters`) **and** is concatenated again from `applied_adhoc_filters_columns` at 4510 → `applied_filter_columns == ["Id", "Id"]`, which `common/query_actions.py:182-183` maps straight into `payload["applied_filters"]` as two entries. 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 the new branch. The only input reaching 3725 is an adhoc column whose label is the empty string (3671 skips it). Remove the branch. **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`; with no `col_obj` there is no `col_spec`, 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 (`force_type_check=True` at 3997), 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-4076`** · _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` with `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`, 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, 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 changes 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]` resolves to the SQLAlchemy `Column` imported at line 60, but the argument is a query-object column list. Use `ColumnTyping` (aliased at line 118). - **tests** — still nothing covering 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:4510` — `assert "CUSTOMERID" in sql_upper` 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: 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:1c78843 --> _Reviewed by EnxDev's Review Agent — @EnxDev · HEAD 1c78843._ -- 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]
