AmoghAtreya commented on code in PR #44955:
URL: https://github.com/apache/superset/pull/44955#discussion_r4200920642
##########
superset-frontend/packages/superset-ui-chart-controls/src/utils/headerGroups.ts:
##########
@@ -329,22 +358,52 @@ 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 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.entries(value).forEach(([key, config]) => {
+ if (colnamesSet.has(key)) {
+ if (!(key in next)) {
Review Comment:
Yes. A key that is itself a current column now always wins (`next[key] =
config`, no `!(key in next)` check). Tests cover leftover-first order for both
`{ Time, __timestamp }` and `{ Principale revenue, Main revenue }`.
--
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]
