sadpandajoe commented on code in PR #44955:
URL: https://github.com/apache/superset/pull/44955#discussion_r4191685749
##########
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:
Now that every render goes through `normalizeColumnConfigKeys`, its existing
time-comparison key remap also runs on charts that have no time comparison.
Take raw-mode result columns `Main revenue`, `Previous revenue`, `# revenue`
with `column_config: { "Previous revenue": { customColumnName: "Prior" } }`: `#
revenue` makes `Previous revenue` look like a translated Main slot, so its
config is moved onto `Main revenue`. The wrong column gets the "Prior" header
and `Previous revenue` loses its settings, whereas the old direct
`columnConfig[key]` read was correct. Should keys that are already real result
columns be left alone before the compatibility remap runs? The AG Grid
`transformProps` has the same call.
##########
superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx:
##########
@@ -390,6 +390,26 @@ describe('plugin-chart-table', () => {
expect(String(parsedDate)).toBe('2020-01-01 12:34:56');
expect(parsedDate.getTime()).toBe(1577882096000);
});
+
+ test('applies legacy Time column_config to the __timestamp column', () => {
Review Comment:
This test uses a datasource with empty `columns` and `metrics`, so it still
passes if `transformProps` stops passing the datasource-derived known keys (the
`getKnownColumnConfigKeys(colnames, props.datasource)` argument). Could a case
be added with a datasource column or metric named `Time` that is not in the
result `colnames`, a result containing `__timestamp`, and `column_config: {
Time: {...} }`, asserting the `__timestamp` column does not pick up that
config? The same gap applies to the AG Grid `transformProps` test.
--
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]
