sadpandajoe commented on code in PR #44955:
URL: https://github.com/apache/superset/pull/44955#discussion_r4334143673
##########
superset-frontend/packages/superset-ui-chart-controls/src/utils/headerGroups.ts:
##########
@@ -329,22 +358,50 @@ export function syncTimeComparisonGroups(
* Remap saved comparison-column config onto locale-independent keys.
*
* Charts created before comparison slots were stored as `Main …` may still
- * keep `t('Main') …` entries (for example `Principale revenue`). The column
- * editor lists the stored keys, so those legacy entries must be rewritten
- * before it filters `column_config` down to `colnames`.
+ * keep `t('Main') …` entries (for example `Principale revenue`). A buggy
+ * column editor also wrote `COLUMN_NAME_ALIASES` display names (for example
+ * `Time` for `__timestamp`). The column editor lists the stored keys, so
+ * those legacy entries must be rewritten before it filters `column_config`
+ * down to `colnames`.
*/
export function normalizeColumnConfigKeys<T>(
value: Record<string, T> | null | undefined,
colnames: string[],
+ knownKeys: string[] = colnames,
): Record<string, T> {
if (!value) {
return {};
}
const colnamesSet = new Set(colnames);
- const next: Record<string, T> = {};
+ const knownKeySet = new Set(knownKeys);
+ const aliasToKey = new Map<string, string>();
+ const ambiguousAliases = new Set<string>();
+ colnames.forEach(col => {
+ getColumnNameAliasLabels(col).forEach(alias => {
+ // A saved key that names a real column or metric is not a legacy alias.
+ if (!alias || knownKeySet.has(alias) || ambiguousAliases.has(alias)) {
+ return;
+ }
+ const existing = aliasToKey.get(alias);
+ if (existing && existing !== col) {
+ aliasToKey.delete(alias);
+ ambiguousAliases.add(alias);
+ return;
+ }
+ aliasToKey.set(alias, col);
+ });
+ });
+ const next: Record<string, T> = Object.create(null);
Object.entries(value).forEach(([key, config]) => {
+ if (colnamesSet.has(key)) {
Review Comment:
A saved key for a real dataset column or metric that looks like a comparison
slot still gets rewritten here, because the guard only protects keys present in
`colnames`, not `knownKeys`. Example: the dataset has a column `Previous
revenue`, the current result is `colnames = ['Main revenue', '# revenue']`, and
`column_config = { 'Previous revenue': { visible: false } }`.
`toStoredTimeComparisonColumnKey` infers a translated Main slot from the `#
revenue` sibling, so the entry moves onto `Main revenue` and Table/AG Grid
(which now run this at render) hide or restyle a column the user never
configured. Should the early return check `knownKeySet` as well, so a known
column or metric is never treated as a legacy slot key?
--
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]
