bito-code-review[bot] commented on code in PR #37396:
URL: https://github.com/apache/superset/pull/37396#discussion_r4042145619
##########
superset/utils/core.py:
##########
@@ -1916,6 +1916,42 @@ def extract_dataframe_dtypes(
return generic_types
+def extract_display_labels(
+ label_map: dict[str, list[str]],
+ colnames: list[str],
+ datasource: Explorable | None = None,
+) -> list[str]:
+ """Extract display labels for a list of column names based on a label map
+ and an optional datasource.
+ """
+ if not colnames:
+ return []
+
+ # Build column -> label mapping (skip self-references)
+ columns_to_label = {}
+ if label_map:
+ for label, cols in label_map.items():
+ for col in cols:
+ if label != col and col not in columns_to_label:
+ columns_to_label[col] = label
+
+ # Build column -> object mapping
+ columns_by_name: dict[str, Any] = {}
+ if datasource:
+ for column in datasource.columns:
+ if isinstance(column, dict):
+ if column_name := column.get("column_name"):
+ columns_by_name[column_name] = column
+ else:
+ columns_by_name[column.column_name] = column
+
+ return [
+ columns_to_label.get(col)
+ or (get_column_name(columns_by_name[col]) if col in columns_by_name
else col)
+ for col in colnames
+ ]
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Undeclared ValueError from callee</b></div>
<div id="fix">
`extract_display_labels` declares `-> list[str]`, but the callee
`get_column_name` (core.py:1366-1391) raises `ValueError("Missing label")` for
a dict column lacking both `label` and `sqlExpression` — exactly the dict shape
this function indexes by `column_name`. Since `_materialize_full_payload` calls
this on every chart-data payload, an unlabeled dict column would crash payload
assembly. Consider a fallback to the raw column name.
</div>
</div>
<small><i>Code Review Run #177160</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/explore/components/DataTablesPane/components/useGridResultTable.tsx:
##########
@@ -69,7 +76,7 @@ export function useGridColumns(
};
})
: [],
- [colnames, data, coltypes, columnDisplayNames],
+ [colnames, data, coltypes, collabels],
);
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing unit tests for useGridColumns</b></div>
<div id="fix">
The `useGridColumns` function has no unit tests despite containing
refactored index-preservation logic and a new `collabels` parameter. Per rule
[6262], tests should verify the actual business logic — including the
map/filter/map chain, index alignment across `colnames`/`coltypes`/`collabels`,
and fallback when `collabels` is undefined or shorter than `colnames`.
</div>
</div>
<small><i>Code Review Run #e9dbd3</i></small>
</div><div>
<div id="suggestion">
<div id="issue"><b>Lost header label cleanup</b></div>
<div id="fix">
Old code stripped `__contribution` from headers and appended localized
`t('contribution')`, and JSON-decoded adhoc-metric labels via `getMetricLabel`.
Backend `extract_display_labels` (superset/utils/core.py:1935,1948)
self-reference-skips renamed contribution columns, so `collabels` carries the
raw `SUM(x)__contribution` string, which now renders verbatim. Restore
suffix/JSON-label cleanup on `headerLabel`, falling back to `key`.
</div>
</div>
<small><i>Code Review Run #177160</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
