EnxDev commented on PR #38716: URL: https://github.com/apache/superset/pull/38716#issuecomment-4809684381
## EnxDev's Review Agent — apache/superset#38716 · HEAD 71fda60 **comment** — fix is correct and the WHERE clause now resolves adhoc-column labels; one payload-bookkeeping issue worth tidying before merge. Verified the data flow: the new `elif col_obj is None and isinstance(flt_col, str)` branch sets `sqla_col`, which falls through to the `if col_obj or sqla_col is not None:` block (`helpers.py:3569`) and builds the real WHERE clause — so the dropped-filter bug from #38339 is genuinely fixed, not just silenced. CI is green; Copilot's earlier nits (NUMERIC→TEXT column, test annotation) were already addressed. ### 🟡 Should-fix - **`superset/models/helpers.py:3545`** — the new branch appends the label to *both* `applied_adhoc_filters_columns` and `applied_template_filters`. But the final `applied_filter_columns` comprehension (`helpers.py:4009`) re-includes any string col found in `applied_template_filters`, so a label like `"Id"` lands in `applied_filter_columns` **twice** (once via the comprehension, once via `+ applied_adhoc_filters_columns`) — duplicated in the response's `applied_filters`. Drop the `applied_adhoc_filters_columns.append(flt_col)` here and keep only the `applied_template_filters` append: that still excludes the label from `rejected_filter_columns` and includes it exactly once in applied. (test: assert no duplicate label in `applied_filter_columns` and that `rejected_filter_columns` is empty for the adhoc-label filter.) ### 🔵 Nits - `tests/unit_tests/models/helpers_test.py` (`test_filter_adhoc_column`) — asserts `WHERE`/`LIKE`/`CUSTOMERID` exist but doesn't tie them together, and no test asserts the `applied_filter_columns`/`rejected_filters` payload — which is exactly where the duplicate above hides. Worth one assertion on those lists so the bookkeeping is guarded. - Codecov reports 5% patch coverage, but that comment is from an older commit (`8d16b46`) and the new branch *is* exercised by `test_filter_adhoc_column`; a re-trigger should confirm the unit flag stays green. ### 🙌 Praise - Good refactor: the orderby and filter paths now share one documented `find_adhoc_column_and_convert_to_sqla` helper instead of duplicating the lookup, and the test matrix (found / not-found / empty / regular-only / multiple) is thorough. <!-- enxdev-review-agent:71fda60 --> _Reviewed by EnxDev's Review Agent — @EnxDev · HEAD 71fda60._ -- 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]
