gabotorresruiz commented on code in PR #43570:
URL: https://github.com/apache/superset/pull/43570#discussion_r4096780261
##########
superset/common/form_data_query_context.py:
##########
@@ -130,6 +130,23 @@ def freeform_where_having(form_data: dict[str, Any]) ->
dict[str, str]:
return extras
+def _as_column_list(value: Any) -> list[Any]:
+ """
+ Normalize a ``groupby``/``columns`` value into a list.
+
+ Single-select controls (e.g. the heatmap ``groupby`` Y axis, which is
+ ``multi: false``, and heatmap charts migrated via ``MigrateHeatmapChart``)
+ store the dimension as a bare string. Wrap a scalar in a one-element list,
+ mirroring ``chart_helpers.resolve_groupby``, so downstream list operations
+ (``.copy()``, ``.insert()``) do not blow up on a ``str``.
+ """
+ if value is None:
+ return []
+ if isinstance(value, str):
+ return [value]
+ return list(value)
Review Comment:
Not a blocker, and not something this PR broke. A note on the sibling of the
case you are fixing.
The same `multi: false` `groupby` control stores a bare **object**, not a
string, when the Y axis is an adhoc or calculated column:
`OptionSelector.getValues()` returns `getColumnNameOrAdhocColumn(values[0])`
when `multi` is false. `list(value)` on that object yields its keys, so the
coercion succeeds and hands the query three invented column names.
I ran `build_query_context_from_form_data` on a heatmap `form_data` whose
`groupby` is `{"expressionType": "SQL", "sqlExpression": ..., "label":
"hour_band"}`. At this head it builds `columns == ["day_of_week",
"expressionType", "sqlExpression", "label"]`; at the merge base `298aa2f0ba`
the same call raises `AttributeError: 'dict' object has no attribute 'insert'`.
So on the dashboard Excel export path it trades a loud crash for a wrong column
list. `chart_helpers.resolve_groupby`, the mirror this docstring cites, has the
same blind spot, so this is a pre-existing family gap and not a regression.
`ensureIsArray` semantics cover both shapes if you want it closed here:
```suggestion
if value is None:
return []
if isinstance(value, (list, tuple)):
return list(value)
return [value]
```
plus a `test_columns_adhoc_groupby_is_wrapped_not_expanded` alongside your
two scalar cases. Equally happy for you to call it out of scope, since nothing
the MCP mapper emits can reach it.
--
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]
