sadpandajoe commented on code in PR #44955:
URL: https://github.com/apache/superset/pull/44955#discussion_r4190201523
##########
superset-frontend/plugins/plugin-chart-table/src/transformProps.ts:
##########
@@ -237,6 +239,11 @@ const processColumns = memoizeOne(function processColumns(
const columnsByName = new Map(
(props.datasource.columns ?? []).map(col => [col.column_name, col]),
);
+ const normalizedColumnConfig = normalizeColumnConfigKeys(
Review Comment:
Legacy `column_config.Time` is now remapped at render time here, but I don't
see a test that goes through `transformProps` (or the ag-grid equivalent) with
that shape. Take a result with `__timestamp` and `column_config: { Time: {
customColumnName: 'Date', d3TimeFormat: '%Y' } }`: if this normalization call
were removed, every added test would still pass while saved charts silently
lose their settings. Could you add a `transformProps` test asserting the
`__timestamp` column picks up that config (and the same for ag-grid)?
##########
superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx:
##########
@@ -701,6 +702,10 @@ const config: ControlPanelConfig = {
coltypes,
childColumnMap,
timeComparisonColumnMap,
+ knownKeys: getKnownColumnConfigKeys(
Review Comment:
With time comparison on, `colnames` here has already been expanded to `Main
Time`, `# Time`, and so on, so an ad hoc metric labelled `Time` is no longer in
`knownKeys`. Ad hoc metrics also aren't in the datasource metrics list, so a
saved `Time` entry looks like a legacy `__timestamp` alias. For a chart with
`__timestamp` plus an ad hoc metric labelled `Time`, editing another column
would write that metric's `customColumnName` onto `__timestamp` and rename the
date column. Should `knownKeys` be built from the original query column names
before the comparison expansion (same in the ag-grid panel)?
--
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]
