Copilot commented on code in PR #37869:
URL: https://github.com/apache/superset/pull/37869#discussion_r2791164313
##########
superset/viz.py:
##########
@@ -747,6 +747,8 @@ def get_data(self, df: pd.DataFrame) -> VizData:
pt = df.pivot_table(index=DTTM_ALIAS, columns=columns, values=values)
pt.index = pt.index.map(str)
pt = pt.sort_index()
+ if isinstance(pt.columns, pd.MultiIndex):
+ pt.columns = [", ".join(str(s) for s in col) for col in pt.columns]
Review Comment:
Flattening MultiIndex columns via `', '.join(...)` can create
ambiguous/duplicate column names when any group-by value contains a comma,
which can silently overwrite values when `pt.to_dict(orient='index')` is built.
The codebase already has MultiIndex-flattening helpers/constants
(`superset.utils.pandas_postprocessing.flatten.flatten` and
`escape_separator`/`FLAT_COLUMN_SEPARATOR`) that escape separators to avoid
collisions; consider reusing those here (or at least escaping the separator)
for consistent and safe serialization.
##########
tests/integration_tests/viz_tests.py:
##########
@@ -626,6 +626,39 @@ def test_get_data_group_by(self):
}
assert expected == data["records"]
+ def test_get_data_multiple_group_by(self):
+ form_data = {"metrics": ["sum__A"], "groupby": ["groupby1",
"groupby2"]}
+ datasource = self.get_datasource_mock()
+ raw = {}
+ t1 = pd.Timestamp("2000")
+ t2 = pd.Timestamp("2002")
+ raw[DTTM_ALIAS] = [t1, t1, t1, t1, t2, t2, t2, t2]
+ raw["sum__A"] = [15, 20, 25, 30, 35, 40, 45, 50]
+ raw["groupby1"] = ["a1", "a2", "a1", "a2", "a1", "a2", "a1", "a2"]
+ raw["groupby2"] = ["b1", "b1", "b2", "b2", "b1", "b1", "b2", "b2"]
+ df = pd.DataFrame(raw)
+ test_viz = viz.TimeTableViz(datasource, form_data)
+ data = test_viz.get_data(df)
+ # Columns should be flattened strings, not tuples
+ assert {"a1, b1", "a1, b2", "a2, b1", "a2, b2"} == set(data["columns"])
+ time_format = "%Y-%m-%d %H:%M:%S"
+ expected = {
+ t1.strftime(time_format): {
+ "a1, b1": 15,
+ "a1, b2": 25,
+ "a2, b1": 20,
+ "a2, b2": 30,
+ },
+ t2.strftime(time_format): {
+ "a1, b1": 35,
+ "a1, b2": 45,
+ "a2, b1": 40,
+ "a2, b2": 50,
Review Comment:
This test hard-codes the `", "` separator in expected column names. Since
the backend already defines `FLAT_COLUMN_SEPARATOR = ", "` for MultiIndex
flattening, consider referencing that constant (or building expectations from
it) to avoid the test becoming stale if the separator ever changes.
```suggestion
sep = viz.FLAT_COLUMN_SEPARATOR
assert {f"a1{sep}b1", f"a1{sep}b2", f"a2{sep}b1", f"a2{sep}b2"} ==
set(
data["columns"]
)
time_format = "%Y-%m-%d %H:%M:%S"
expected = {
t1.strftime(time_format): {
f"a1{sep}b1": 15,
f"a1{sep}b2": 25,
f"a2{sep}b1": 20,
f"a2{sep}b2": 30,
},
t2.strftime(time_format): {
f"a1{sep}b1": 35,
f"a1{sep}b2": 45,
f"a2{sep}b1": 40,
f"a2{sep}b2": 50,
```
--
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]
