EnxDev commented on PR #38716: URL: https://github.com/apache/superset/pull/38716#issuecomment-5085026426
## EnxDev's Review Agent โ apache/superset#38716 ยท HEAD 9b52bbd **request changes** โ the WHERE fix works, but the duplicate-bookkeeping issue from my last pass is still present, and the ORDER BY half of the diff is unreachable. Supersedes my [earlier review](https://github.com/apache/superset/pull/38716#issuecomment-4809684381) at HEAD `71fda60`. Re-checked the diff at this HEAD: the `applied_adhoc_filters_columns.append(flt_col)` I asked to drop is still on line 4035 alongside the `applied_template_filters` append, so the duplicate is unchanged. CI is green; codecov reports 4.76% patch coverage at this HEAD. ### ๐ด Functional - **`superset/models/helpers.py:4035`** ยท _High_ โ the label is appended to *both* `applied_adhoc_filters_columns` and `applied_template_filters`, so it satisfies the `applied_filter_columns` comprehension (`helpers.py:4509`) **and** gets concatenated from `applied_adhoc_filters_columns` (`helpers.py:4510`) โ `applied_filter_columns == ["Id", "Id"]`. `query_actions.py:182` maps that straight into `payload["applied_filters"]`, so the dashboard filter indicator lists the column twice. Drop the line 4035 append โ a plain string label is handled correctly by the comprehension alone, and it still stays out of `rejected_filter_columns`. **regression test:** assert `get_sqla_query(...).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 `helpers.py:3666`, and the pre-existing `elif isinstance(col, str) and col in adhoc_columns_by_label` at `helpers.py:3711` (already on master) resolves ORDER BY labels before control ever reaches the new branch. The only input that reaches line 3725 is an adhoc column whose label is the empty string. Remove the branch. **regression test:** n/a โ but the PR adds no ORDER BY test at all, which is why this went unnoticed. - **`superset/models/helpers.py:4029`** ยท _Medium_ โ the helper discards the second return value, so `adhoc_generic_type` stays `None` and `target_generic_type` falls through to `GenericDataType.STRING` (`helpers.py:4097`). `filter_values_handler` only casts values when the target is NUMERIC or BOOLEAN (`helpers.py:2898-2918`), so a numeric adhoc column filtered by label yields `IN ('1','2')` instead of `IN (1,2)`. The sibling dict path avoids this deliberately via `force_type_check=True` (`helpers.py:3995`), and the comment at `helpers.py:4090` describes exactly this failure. Return the generic type from the helper and assign it. **regression test:** adhoc column `CAST(x AS BIGINT)` + `IN [1, 2]` filter by label โ assert the compiled SQL has unquoted values. - **`superset/models/helpers.py:4074`** ยท _Medium_ โ the label path never gets the `Grouping()` wrap: `flt_col` is a `str`, so `is_adhoc_column(flt_col)` is False and `col_obj` is None. An adhoc expression with a top-level `OR`/`AND` then breaks the surrounding AND-joined filters โ the precedence bug #38183 added this wrap for. Track that the column was resolved from an adhoc label and include it in the condition. **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: `helpers.py:3748` strips `__timestamp` and `helpers.py:3753` does `columns = groupby or columns`. In aggregate mode the lookup therefore searches `groupby`, not the query's `columns`. Using `adhoc_columns_by_label` (built at line 3666, before both rebindings) fixes this and removes the need for the new helper entirely: `adhoc_columns_by_label.get(flt_col)`. **regression test:** aggregate query where the adhoc column is in `columns` but `groupby` is set to something else โ filter by label still resolves. ### ๐ก Should-fix - **`superset/models/helpers.py:4001`** โ this append sits in the *pre-existing* adhoc-dict branch and is unrelated to the bug being fixed. It is a no-op for applied/rejected bookkeeping (adhoc dicts are excluded by `not is_adhoc_column(col)` in both comprehensions), but it does change the returned and cached `applied_template_filters` payload field, and it will mask a genuinely-rejected plain-string filter that happens to share an adhoc column's label. Drop it. - **`superset/models/helpers.py:3102`** โ `columns: list[Column]` uses the SQLAlchemy `Column` imported at `helpers.py:60`, but the argument is a query-object column list. Use `ColumnTyping` (`helpers.py:118`). - **`superset/models/helpers.py:3127`** โ `adhoc_column_to_sqla` raises `ColumnNotFoundException` when the adhoc column has `columnType: "BASE_AXIS"` and a `timeGrain` (`connectors/sqla/models.py:1872`). The dict path catches it and rejects the filter gracefully (`helpers.py:4002`); the new path lets it escape as a query failure. Catch it and return `None`. - **tests** โ nothing covers the ORDER BY call site or the applied/rejected bookkeeping this diff changes; one assertion on `applied_filter_columns` would have caught the finding above. Codecov reports 20 of the changed lines uncovered at this HEAD โ worth confirming the new unit tests actually run in the coverage job. ### ๐ต Nits - `tests/unit_tests/models/helpers_test.py` โ 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. Assert the rendered expression contains `CustomerId` / `CustomerName`. - The translation-regression bot flags fuzzy `.po` regressions in 6 locales, but this diff contains no translatable strings โ likely a stale-base artifact rather than a real regression; worth re-checking after the rebase. ### ๐ 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. <!-- enxdev-review-agent:9b52bbd --> _Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD 9b52bbd._ -- 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]
