EnxDev commented on PR #38716: URL: https://github.com/apache/superset/pull/38716#issuecomment-5133538167
## EnxDev's Review Agent — apache/superset#38716 · HEAD d41a4bc **request changes** — real progress: the type-check, exception handling and duplicate-label fixes are genuinely in. The applied-filter double-count and the dead ORDER BY branch are not. Supersedes my [previous review](https://github.com/apache/superset/pull/38716#issuecomment-5114579792) at HEAD `1c78843`. @MallikarjunaReddyN — checked each item against the diff at this HEAD rather than the "resolved" note. CI is green. ### ✅ Verified fixed since `1c78843` - `force_type_check=True` and `adhoc_generic_type` are now assigned on the label path (4079-4083), so numeric/temporal values are coerced from the probed type instead of falling back to `STRING` at 4147. `test_filter_adhoc_column_label_uses_force_type_check` guards it. - `ColumnNotFoundException` is handled in both the helper (returns `None`) and the filter path (4086-4088 rejects the filter instead of failing the query), with a test. - Duplicate labels now resolve last-wins in both lookups (`reversed(columns)`), matching `adhoc_columns_by_label` at 3712, with a test. - `test_filter_adhoc_column` now compiles with `literal_binds` and asserts `CUSTOMERID` and `C001%` inside the WHERE clause, so it can no longer pass on the SELECT alone. ### 🔴 Functional - **`superset/models/helpers.py:4084-4085`** · _High_ — the filter is still reported twice; only the shape of the second entry changed. 4085 puts `"Id"` in `applied_template_filters`, which makes the plain string `"Id"` satisfy the comprehension at 4556-4561, and 4084 appends the adhoc **dict** that is concatenated at 4562 → `applied_filter_columns == ["Id", {"label": "Id", …}]`. `common/query_actions.py:182-183` runs both through `get_column_name`, so the payload carries two identical `{"column": "Id"}` entries and the dashboard filter badge counts the filter twice. `test_filter_adhoc_column_label_tracks_applied_filter_as_adhoc_object` only asserts `applied_filter_columns[-1]`, so it passes with the duplicate present. Fix: drop 4085 **and** exclude the resolved label from `rejected_filter_columns` (4547-4553) — dropping 4085 alone flips `"Id"` into *rejected* while it is also reported applied, since it is in neither `self.column_names` nor `applied_template_filters`. **r egression test:** `assert len(result.applied_filter_columns) == 1` and `assert result.rejected_filter_columns == []`. - **`superset/models/helpers.py:3765-3773`** · _High_ — still dead code. `adhoc_columns_by_label` is built from the same `columns` list at 3706-3712 and the pre-existing `elif isinstance(col, str) and col in adhoc_columns_by_label` at 3751 resolves ORDER BY labels earlier in the same `elif` chain; 3712 now also resolves last-wins, so the two agree. The only input that reaches 3765 is an adhoc column whose label is the empty string (3711 skips those). Sorting by a renamed adhoc column already works on `master` — remove the branch, or show a case 3751 misses. - **`superset/models/helpers.py:4127`** · _Medium_ — the label path still 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 the label `str` with `col_obj is None`. An adhoc expression with a top-level `OR`/`AND` splices unparenthesized into the AND-joined WHERE — the precedence bug #38183 added this wrap for. `adhoc_col` is in hand at 4077; key the wrap off that. **regression test:** adhoc col `a = 1 OR b = 2` filtered by label alongside a second filter → assert the expression is parenthesized in the compiled WHERE. - **`superset/models/helpers.py:4069-4076`** · _Low_ — the lookup reads `columns` after it is rebound: 3788 strips `__timestamp` and 3793 does `columns = groupby or columns`. In aggregate mode with a legacy `groupby` payload it searches `groupby`, not the query's `columns`, and the label silently fails to resolve. `adhoc_columns_by_label` (built at 3706, before both rebindings) is already the right lookup for both call sites. **regression test:** aggregate query with the adhoc column in `columns` and a different `groupby` → filter by label still resolves. ### 🟡 Should-fix - **`superset/models/helpers.py:4041`** — this append is in the *pre-existing* adhoc-dict branch and unrelated to the bug. It is a no-op for both comprehensions (dicts are excluded by `not is_adhoc_column(col)`), so it only mutates the returned and cached `applied_template_filters` 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:3124`** — `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** — the ORDER BY call site added by this diff still has no test. One test ordering by an adhoc label would have shown the branch never runs. ### 🔵 Nits - `superset/models/helpers.py:4081` — `force_type_check=True` triggers a DB probe for genuinely computed expressions (a label pointing at a physical column short-circuits via metadata). For the server-pagination search box that is an extra round trip per search request. Same cost the adhoc-dict path already pays, so it is consistent — just worth knowing. ### 🙌 Praise - The rework is well targeted: `force_type_check` threaded through with the generic type, graceful `ColumnNotFoundException` handling on both paths, last-wins duplicate resolution, and each one landed with its own test. _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:d41a4bc --> _Reviewed by EnxDev's Review Agent — @EnxDev · HEAD d41a4bc._ -- 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]
