bito-code-review[bot] commented on code in PR #37412:
URL: https://github.com/apache/superset/pull/37412#discussion_r2750134666
##########
superset-frontend/spec/fixtures/mockTimeSeries.ts:
##########
@@ -0,0 +1,251 @@
+import { DatasourceType } from '@superset-ui/core';
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing License Header</b></div>
<div id="fix">
New files must include the ASF license header. The provided patch adds it
before the imports.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
@@ -0,0 +1,20 @@
+/*
+ * 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.
+ */
```
</div>
</details>
</div>
<details>
<summary><b>Citations</b></summary>
<ul>
<li>
Rule Violated: <a
href="https://github.com/apache/superset/blob/078ca18/AGENTS.md#L90";>AGENTS.md:90</a>
</li>
</ul>
</details>
<small><i>Code Review Run #28d52a</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/explore/components/DataTablesPane/components/useResultsPane.test.tsx:
##########
@@ -0,0 +1,172 @@
+/**
+ * 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 { renderHook } from '@testing-library/react-hooks';
+import { useResultsPane } from './useResultsPane';
+import { TimeGranularity } from '@superset-ui/core';
+import { ResultsPaneProps } from '../types';
+import * as chartAction from 'src/components/Chart/chartAction';
+import { screen, waitFor } from '@testing-library/dom';
+import { render } from '@testing-library/react';
+import { ThemeProvider, supersetTheme } from '@apache-superset/core/ui';
+import { Provider } from 'react-redux';
+import { createStore } from 'redux';
+
+// Mock the Redux store
+const mockStore = createStore(() => ({}));
+
+const args: ResultsPaneProps = {
+ queryFormData: {
+ datasource: '2__table',
+ viz_type: 'echarts_timeseries_line',
+ slice_id: 346,
+ url_params: {
+ native_filters_key: 'QxTpEx9EJ00',
+ },
+ x_axis: 'ds',
+ time_grain_sqla: TimeGranularity.YEAR,
+ x_axis_sort_asc: true,
+ metrics: ['sum__num', 'testing_count'],
+ groupby: ['gender', 'state'],
+ adhoc_filters: [
+ {
+ clause: 'WHERE',
+ comparator: 'No filter',
+ expressionType: 'SIMPLE',
+ operator: 'TEMPORAL_RANGE',
+ subject: 'ds',
+ },
+ ],
+ },
+ queryForce: false,
+ isRequest: true,
+ dataSize: 20,
+ isVisible: true,
+ canDownload: true,
+};
+
+const mockedGetChartDataResponse = {
+ result: [
+ {
+ status: 'success',
+ rowcount: 44,
+ sql_rowcount: 968,
+ label_map: {
+ ds: ['ds'],
+ 'sum__num, boy, CA': ['sum__num', 'boy', 'CA'],
+ 'sum__num, boy, FL': ['sum__num', 'boy', 'FL'],
+ 'sum__num, girl, CA': ['sum__num', 'girl', 'CA'],
+ 'sum__num, girl, FL': ['sum__num', 'girl', 'FL'],
+ 'testing_count, boy, CA': ['testing_count', 'boy', 'CA'],
+ 'testing_count, boy, FL': ['testing_count', 'boy', 'FL'],
+ 'testing_count, girl, CA': ['testing_count', 'girl', 'CA'],
+ 'testing_count, girl, FL': ['testing_count', 'girl', 'FL'],
+ gender: ['gender'],
+ state: ['state'],
+ sum__num: ['sum__num'],
+ 'Testing count': ['testing_count'],
+ },
+ colnames: [
+ 'ds',
+ 'sum__num, boy, CA',
+ 'sum__num, boy, FL',
+ 'sum__num, girl, CA',
+ 'sum__num, girl, FL',
+ 'testing_count, boy, CA',
+ 'testing_count, boy, FL',
+ 'testing_count, girl, CA',
+ 'testing_count, girl, FL',
+ ],
+ indexnames: [0, 1, 2, 3, 4, 5, 6, 7, 8],
+ coltypes: [2, 0, 0, 0, 0, 0, 0, 0, 0],
+ data: [
+ {
+ ds: -157766400000,
+ 'sum__num, boy, CA': 110334,
+ 'sum__num, boy, FL': 30628,
+ 'sum__num, girl, CA': 81367,
+ 'sum__num, girl, FL': 23627,
+ 'testing_count, boy, CA': 65,
+ 'testing_count, boy, FL': 60,
+ 'testing_count, girl, CA': 73,
+ 'testing_count, girl, FL': 67,
+ },
+ {
+ ds: -126230400000,
+ 'sum__num, boy, CA': 106402,
+ 'sum__num, boy, FL': 30233,
+ 'sum__num, girl, CA': 78271,
+ 'sum__num, girl, FL': 22929,
+ 'testing_count, boy, CA': 66,
+ 'testing_count, boy, FL': 62,
+ 'testing_count, girl, CA': 75,
+ 'testing_count, girl, FL': 67,
+ },
+ ],
+ result_format: 'json',
+ detected_currency: null,
+ applied_filters: [
+ {
+ column: 'ds',
+ },
+ ],
+ rejected_filters: [],
+ },
+ ],
+};
+
+const getChartDataRequestSpy = jest.spyOn(chartAction, 'getChartDataRequest');
+
+test('useResultsPane should format table columns and props successfully',
async () => {
+ getChartDataRequestSpy.mockClear();
+
+ getChartDataRequestSpy.mockResolvedValue({
+ response: {},
+ json: mockedGetChartDataResponse,
+ });
+
+ const hookResult = renderHook(() => useResultsPane(args));
+
+ await waitFor(() => {
+ expect(getChartDataRequestSpy).toHaveBeenCalledWith({
+ force: false,
+ formData: args.queryFormData,
+ ownState: undefined,
+ resultFormat: 'json',
+ resultType: 'full',
+ });
+ });
+
+ render(
+ <Provider store={mockStore}>
+ <ThemeProvider theme={supersetTheme}>
+ {hookResult.result.all}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Wrong hook result property</b></div>
<div id="fix">
Using `hookResult.result.all` renders an array of results instead of the
current result, which may cause incorrect test behavior. Use `.current` for the
latest hook return value.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
{hookResult.result.current}
````
</div>
</details>
</div>
<small><i>Code Review Run #28d52a</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/packages/superset-ui-core/src/utils/addLabelMapToVerboseMap.ts:
##########
@@ -0,0 +1,161 @@
+/**
+ * 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.
+ */
+
+/**
+ * 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, '\\$&');
+
+/**
+ * Replaces a label inside a compound key only if it appears as a complete
+ * word/token and contains at least one alphabetic character.
+ *
+ * @param key - The source string (typically a compound field name)
+ * @param label - The label to search for as a standalone token
+ * @param replacement - The human-readable value to replace the label with
+ * @returns The transformed key if a valid match exists, otherwise the
original key
+ */
+const replaceLabelIfExists = (
+ key: string,
+ label: string,
+ replacement: string,
+) => {
+ /**
+ * Logic:
+ *
+ * This function is intentionally stricter than a simple substring replace:
+ * - The label must NOT be part of a larger word (e.g. "account" will NOT
match
+ * "testing_account").
+ * - Underscores (`_`) are treated as part of the word.
+ * - Numeric-only matches are ignored (e.g. "12" will NOT match "123").
+ *
+ * If the label is found, only the matched portion is replaced; otherwise,
+ * the original key is returned unchanged.
+ *
+ * Examples:
+ * - replaceLabelIfExists("testing_account 123", "testing_account",
"Account")
+ * → "Account 123"
+ * - replaceLabelIfExists("testing_account 123", "account", "Account")
+ * → "testing_account 123"
+ * - replaceLabelIfExists("123", "12", "X")
+ * → "123"
+ */
+
+ if (key === label) {
+ return replacement;
+ }
+
+ const escapedLabel = escapeRegex(label);
+ const regex = new RegExp(`(?<!\\w)${escapedLabel}(?!\\w)`, 'g');
+ return regex.test(key) ? key.replace(regex, replacement) : key;
+};
+
+/**
+ * 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
+ */
+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:
+ * - Each compound key is processed word-by-word (split on spaces).
+ * - Only labels that exist in `verboseMap` are considered.
+ * - Each word is replaced at most once, using `replaceLabelIfExists`, which:
+ * - Matches only full tokens (no partial matches).
+ * - Treats underscores (`_`) as part of a token.
+ * - Is 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 newKey = key
+ .split(' ')
+ .map(word => {
+ for (const label of Object.keys(newLabelMap)) {
+ const newWord = replaceLabelIfExists(
+ word,
+ label,
+ newLabelMap[label],
+ );
+
+ if (newWord !== word) {
+ return newWord;
+ }
+ }
+
+ return word;
+ })
+ .join(' ');
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Logic mismatch with example</b></div>
<div id="fix">
The code splits the compound key on spaces and processes words individually,
but the documented example expects replacements within the full key string
using word boundaries. This leads to incorrect output for keys like
"testing_count, 1 week ago".
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
const newKey = Object.keys(newLabelMap).reduce((acc, label) =>
replaceLabelIfExists(acc, label, newLabelMap[label]), key);
````
</div>
</details>
</div>
<small><i>Code Review Run #28d52a</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.test.ts:
##########
@@ -0,0 +1,64 @@
+/**
+ * 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 transformProps from './transformProps';
+import { mockedMixedTimeSeriesProps } from 'spec/fixtures/mockTimeSeries';
+
+test('MixedTimeseries should transform props correctly', () => {
+ const result = transformProps(mockedMixedTimeSeriesProps);
+
+ expect(result.coltypeMapping).toEqual({
+ 'SUM(money_for_learning)': 0,
+ 'SUM(money_for_learning), 1 day ago': 0,
+ 'SUM(money_for_learning), 1 week ago': 0,
+ 'SUM(money_for_learning), 1 year ago': 1,
+ testing_count: 0,
+ 'testing_count, 1 day ago': 0,
+ 'testing_count, 1 week ago': 0,
+ 'testing_count, 1 year ago': 1,
+ time_start: 2,
+ });
+
+ const legendData = (result.echartOptions.legend as { data: string[] }).data;
+
+ const expectedResult = [
+ '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',
+ 'time_start',
+ 'time_start',
+ ];
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incorrect test expectation for legend data</b></div>
<div id="fix">
The expectedResult in the legend assertion incorrectly includes 'time_start'
twice, but the echartOptions.legend.data only contains series names from
observation series, not the x-axis label. This will cause the test to fail.
Based on the transformProps code, legend.data is built from series names
filtered for observations, without including 'time_start'.
</div>
</div>
<small><i>Code Review Run #28d52a</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
