EnxDev commented on code in PR #37229:
URL: https://github.com/apache/superset/pull/37229#discussion_r4403097921
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/forecast.ts:
##########
@@ -32,6 +32,106 @@ import {
} from '../types';
import { sanitizeHtml } from './series';
+/**
+ * Escapes RegExp metacharacters in a string so it can be safely used in a
+ * dynamically created regular expression.
+ *
+ * @param value - The raw string to escape
+ * @returns The escaped string safe for use in `new RegExp(...)`
+ */
+const escapeRegex = (value: string) =>
+ value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
+
+/**
+ * Enriches the verbose map by creating human-readable versions of compound
field names.
+ *
+ * @param label_map — a mapping of compound keys to arrays of component labels
(e.g., { "revenue_total_usd": ["revenue", "total", "usd"] })
+ * @param verboseMap — the existing mapping of field names to their display
labels
+ * @returns an updated verbose map that includes human-readable versions of
the compound keys
+ */
+export const addLabelMapToVerboseMap = (
+ label_map: Record<string, string[]>,
Review Comment:
This still has no default, so `Object.entries(label_map)` throws if a
payload ever comes through without one. Mixed passes
`rawLabelMap`/`rawLabelMapB` straight in, unlike Timeseries which defaults it
at destructure.
This is the one @rusackas asked for back in July, and it's a one-liner:
```suggestion
label_map: Record<string, string[]> = {},
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/forecast.ts:
##########
@@ -32,6 +32,106 @@ import {
} from '../types';
import { sanitizeHtml } from './series';
+/**
+ * Escapes RegExp metacharacters in a string so it can be safely used in a
+ * dynamically created regular expression.
+ *
+ * @param value - The raw string to escape
+ * @returns The escaped string safe for use in `new RegExp(...)`
+ */
+const escapeRegex = (value: string) =>
+ value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
+
+/**
+ * Enriches the verbose map by creating human-readable versions of compound
field names.
+ *
+ * @param label_map — a mapping of compound keys to arrays of component labels
(e.g., { "revenue_total_usd": ["revenue", "total", "usd"] })
+ * @param verboseMap — the existing mapping of field names to their display
labels
+ * @returns an updated verbose map that includes human-readable versions of
the compound keys
+ */
+export const addLabelMapToVerboseMap = (
+ label_map: Record<string, string[]>,
+ verboseMap: Record<string, string> = {},
+): Record<string, string> => {
+ /**
+ * Logic:
+ *
+ * This function takes a mapping of compound field names to their component
labels
+ * and replaces those labels with their corresponding human-readable values
from
+ * `verboseMap`, producing display-friendly versions of the compound keys.
+ *
+ * Replacement behavior:
+ * - Only labels that exist in `verboseMap` are considered.
+ * - Every match is found in a single pass over the whole key, using one
+ * combined regex alternation of all its labels (longest first, so a
+ * multi-word label like "1 week ago" is matched as a whole phrase
+ * rather than, say, just "ago"). Scanning word-by-word instead (the
+ * previous approach) could never match a multi-word label at all,
+ * since no single space-delimited word equals the whole phrase.
+ * Matching everything in one pass over the original string also means
+ * a label's own replacement text is never re-scanned by a later label
+ * in the same key.
+ * - Each match must be a complete token: it must NOT be part of a larger
+ * word (e.g. "account" will NOT match "testing_account"), with
+ * underscores (`_`) treated as part of the word. Case-sensitive.
+ * - Labels not found in `verboseMap` are left unchanged.
+ *
+ * The original `verboseMap` is preserved and extended with the newly
generated
+ * human-readable entries.
+ *
+ * Example:
+ * ```ts
+ * label_map = {
+ * "testing_count, 1 week ago": ["testing_count", "1 week ago"]
+ * }
+ *
+ * verboseMap = {
+ * testing_count: "Testing Count"
+ * }
+ *
+ * Result:
+ * {
+ * testing_count: "Testing Count",
+ * "testing_count, 1 week ago": "Testing Count, 1 week ago"
+ * }
+ * ```
+ */
+ const newVerboseMap: Record<string, string> = {};
+
+ Object.entries(label_map).forEach(([key, labels]) => {
+ if (labels) {
+ const newLabelMap: Record<string, string> = labels
+ .filter(l => verboseMap[l])
+ .reduce(
+ (acc, label) => ({
+ ...acc,
+ [label]: verboseMap[label],
+ }),
+ {},
+ );
+
+ const labelsToReplace = Object.keys(newLabelMap).sort(
+ (a, b) => b.length - a.length,
+ );
+
+ const newKey =
+ labelsToReplace.length === 0
+ ? key
+ : key.replace(
+ new RegExp(
+
`(?<!\\w)(?:${labelsToReplace.map(escapeRegex).join('|')})(?!\\w)`,
Review Comment:
The `\w` boundary lets a label match inside another segment of the key. With
metric `count` (verbose "Total Count") grouped by a dimension whose value is
"Head count", the key `count, Head count` comes out as `Total Count, Head Total
Count`, since the space before "count" counts as a boundary.
Anchoring on the `, ` separators keeps replacements to whole segments, and
the existing tests should still pass. Worth a test with that key too.
```suggestion
`(?<=^|,
)(?:${labelsToReplace.map(escapeRegex).join('|')})(?=, |$)`,
```
##########
superset-frontend/spec/fixtures/mockTimeSeries.ts:
##########
@@ -0,0 +1,269 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { DatasourceType } from '@superset-ui/core';
+import { EchartsTimeseriesChartProps } from
'@superset-ui/plugin-chart-echarts';
+import { EchartsMixedTimeseriesProps } from
'plugins/plugin-chart-echarts/src/MixedTimeseries/types';
+
+export const mockedTimeSeriesProps: EchartsTimeseriesChartProps = {
+ // Datasource metadata
+ datasource: {
+ id: 1,
+ name: 'test_datasource',
+ type: DatasourceType.Table,
+ columnFormats: {},
+ currencyFormats: {},
+ verboseMap: {
+ testing_count: 'Testing count',
+ 'SUM(money_for_learning)': 'SUM(money_for_learning)',
+ time_start: 'time_start',
+ },
+ },
+
+ // Query results
+ queriesData: [
+ {
+ label_map: {
+ time_start: ['time_start'],
+ 'SUM(money_for_learning)': ['SUM(money_for_learning)'],
+ 'SUM(money_for_learning), 1 day ago': [
+ 'SUM(money_for_learning)',
+ '1 day ago',
+ ],
+ 'SUM(money_for_learning), 1 week ago': [
+ 'SUM(money_for_learning)',
+ '1 week ago',
+ ],
+ 'SUM(money_for_learning), 1 year ago': [
+ 'SUM(money_for_learning)',
+ '1 year ago',
+ ],
+ testing_count: ['testing_count'],
+ 'testing_count, 1 day ago': ['testing_count', '1 day ago'],
+ 'testing_count, 1 week ago': ['testing_count', '1 week ago'],
+ 'testing_count, 1 year ago': ['testing_count', '1 year ago'],
+ 'Testing count': ['testing_count'],
+ },
+ colnames: [
+ 'time_start',
+ 'SUM(money_for_learning)',
+ 'SUM(money_for_learning), 1 day ago',
+ 'SUM(money_for_learning), 1 week ago',
+ 'SUM(money_for_learning), 1 year ago',
+ 'testing_count',
+ 'testing_count, 1 day ago',
+ 'testing_count, 1 week ago',
+ 'testing_count, 1 year ago',
+ ],
+ indexnames: [0, 1, 2],
+ coltypes: [2, 0, 0, 0, 1, 0, 0, 0, 1],
+ data: [
+ {
+ time_start: 1533081600001,
+ 'SUM(money_for_learning)': 101,
+ 'SUM(money_for_learning), 1 day ago': null,
+ 'SUM(money_for_learning), 1 week ago': null,
+ 'SUM(money_for_learning), 1 year ago': null,
+ testing_count: 1,
+ 'testing_count, 1 day ago': null,
+ 'testing_count, 1 week ago': null,
+ 'testing_count, 1 year ago': null,
+ },
+ {
+ time_start: 1533168000001,
+ 'SUM(money_for_learning)': 5791,
+ 'SUM(money_for_learning), 1 day ago': 101,
+ 'SUM(money_for_learning), 1 week ago': null,
+ 'SUM(money_for_learning), 1 year ago': null,
+ testing_count: 131,
+ 'testing_count, 1 day ago': 11,
+ 'testing_count, 1 week ago': null,
+ 'testing_count, 1 year ago': null,
+ },
+ ],
+ result_format: 'json',
+ applied_filters: [
+ {
+ column: 'time_start',
+ },
+ ],
+ rejected_filters: [],
+ },
+ ],
+
+ // Filter and legend state
+ filterState: {
+ selectedValues: [],
+ },
+ legendState: {},
+
+ // Form data (chart configuration)
+ formData: {
+ metrics: [
+ {
+ aggregate: 'SUM',
+ column: {
+ column_name: 'money_for_learning',
+ filterable: true,
+ groupby: true,
+ id: 460,
+ is_dttm: false,
+ type: 'DOUBLE PRECISION',
+ type_generic: 0,
+ },
+ expressionType: 'SIMPLE',
+ hasCustomLabel: false,
+ label: 'SUM(money_for_learning)',
+ optionName: 'metric_olnflt1ak9g_z1vjg3dhmnf',
+ },
+ 'testing_count',
+ ],
+ },
+
+ // Hooks
+ hooks: {
+ setDataMask: () => {},
+ setControlValue: () => {},
+ onContextMenu: () => {},
+ onLegendStateChanged: () => {},
+ onLegendScroll: () => {},
+ },
+
+ // UI state
+ inContextMenu: false,
+ emitCrossFilters: true,
+ legendIndex: 0,
+
+ // Raw form data (needed for additional validation)
+ rawFormData: {
+ datasource: '1__table',
+ viz_type: 'echarts_timeseries',
+ x_axis: 'ds',
Review Comment:
`x_axis` is `ds` but every row keys time on `time_start`, so transformProps
treats `time_start` as a metric series. That's why both legend tests expect
`time_start` in the legend.
Could we set this to `time_start` and drop it from the expected legends? As
it stands the tests pass against a chart that wouldn't render that way.
--
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]
