bito-code-review[bot] commented on code in PR #37756:
URL: https://github.com/apache/superset/pull/37756#discussion_r4137552791
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/controlPanel.tsx:
##########
@@ -874,29 +872,37 @@ const config: ControlPanelConfig = {
})),
]
: [];
- const numericColumns = hasColumns
+ // Paired with its dataType by original index before
+ // filtering, so a column's type stays correct even when
+ // colnames has duplicates (filtering first and re-deriving
+ // the type afterwards, by value or by post-filter index,
+ // both break on a duplicate colname). String/Boolean columns
+ // are excluded during time comparison: the synthetic
+ // Main/#/△/% columns processComparisonColumns() generates
+ // below don't carry a categorical dataType of their own.
+ const eligibleColumns = hasColumns
? colnames
- .filter(
- (colname: string, index: number) =>
- coltypes[index] === GenericDataType.Numeric,
- )
- .map((colname: string) => ({
+ .map((colname: string, index: number) => ({
value: colname,
label: Array.isArray(verboseMap)
? colname
: (verboseMap?.[colname] ?? colname),
- // Every entry here already passed the Numeric filter
- // above, so the type is always Numeric — no need to
- // re-look it up (which breaks on duplicate colnames).
- dataType: GenericDataType.Numeric,
+ dataType: coltypes[index],
}))
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicated column mapping</b></div>
<div id="fix">
The new `eligibleColumns` map (884-891) duplicates the identical
`colnames.map(...)` mapping already in `allColumns` (866-872). Two parallel
mappings in the same `mapStateToProps` can silently diverge when one is
updated. Consider a shared helper or deriving `eligibleColumns` from
`allColumns` (minus the `ENTIRE_ROW` entry) before filtering by type.
</div>
</div>
<small><i>Code Review Run #3f5d00</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]
