You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "alex241728 (via GitHub)" <gi...@apache.org> on 2025/10/30 17:26:28 UTC
[PR] feat: Floating Point Formatting for Scatter Point Chart [superset]
alex241728 opened a new pull request, #35915:
URL: https://github.com/apache/superset/pull/35915
### SUMMARY
According to the issue https://github.com/apache/superset/issues/32902, the scatter point charts lack of generic axes, which means the scatter point charts only allow x-axis to be formatted with time formats. So, to fix the problem mentioned in this issue, our team adds a new floating-point formats for x-axis.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
The before screenshot can be checked in issue https://github.com/apache/superset/issues/32902
The after screenshot:
<img width="319" height="476" alt="Screenshot 2025-10-30 at 13 24 10" src="https://github.com/user-attachments/assets/26086241-58da-4c57-8ab4-156f1c4ab74d" />
### TESTING INSTRUCTIONS
1. Create a scatter point chart or open an existing scatter point chart.
2. Select an "X-axis" in "Data" tab.
3. Check and select the format of "X-axis" in "Customize" tab. If the x-axis has a floating point data type (e.g. FLOAT, DOUBLE, REAL, NUMERIC, DECIMAL), the user interface will show "X Axis Number Format" and you can select formats for floating point numbers. If the x-axis has a time data type (e.g. TIMESTAMP WITHOUT TIME ZONE, TIMESTAMP WITH TIME ZONE), the user interface will show "Time Format" and you can select formats for time. The floating point formats are new features for x-axis, but the time formats are old features.
### ADDITIONAL INFORMATION
- [x] Has associated issue: https://github.com/apache/superset/issues/32902
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [x] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Floating Point Formatting for Scatter Point Chart [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35915:
URL: https://github.com/apache/superset/pull/35915#issuecomment-3469213285
<details open>
<summary><strong>Interaction Diagram by <a href="https://bito.ai#sequence_diagram">Bito</a></strong></summary>
```mermaid
sequenceDiagram
participant Dev as Developer
participant SharedCtrl as sharedControls.tsx<br/>🟩 Added | ●●● High
participant CtrlPanel as controlPanel.tsx<br/>🔄 Updated | ●●● High
participant Types as types.ts<br/>🔄 Updated | ●●○ Medium
participant Constants as constants.ts<br/>🔄 Updated | ●●○ Medium
participant TransformProps as transformProps.ts<br/>🔄 Updated | ●●● High
participant EchartsUI as EchartsTimeseries Component
Dev->>SharedCtrl: Define x_axis_number_format control
SharedCtrl-->>CtrlPanel: Export new control config
CtrlPanel->>CtrlPanel: Add visibility logic for numeric X-axis
CtrlPanel->>Types: Pass xAxisNumberFormat field
Types->>Constants: Initialize with SMART_NUMBER default
Constants->>TransformProps: Provide default format value
TransformProps->>TransformProps: Check if X-axis is Numeric type
TransformProps->>TransformProps: Apply getNumberFormatter(xAxisNumberFormat)
TransformProps-->>EchartsUI: Return formatted echartOptions
```
Critical path: Developer->sharedControls.tsx->controlPanel.tsx->transformProps.ts->EchartsTimeseries Component
> **Note:** The diff adds support for numeric X-axis formatting in Timeseries charts. A new shared control 'x_axis_number_format' is defined, integrated into the control panel with conditional visibility for numeric columns, and used in transformProps to format numeric X-axis values alongside existing time-format support.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Floating Point Formatting for Scatter Point Chart [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #35915:
URL: https://github.com/apache/superset/pull/35915#issuecomment-3488606460
Re-running CI 🤞
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Floating Point Formatting for Scatter Point Chart [superset]
Posted by "alex241728 (via GitHub)" <gi...@apache.org>.
alex241728 commented on PR #35915:
URL: https://github.com/apache/superset/pull/35915#issuecomment-3469435250
> @alex241728 thanks for creating the PR. Do you mind adding tests to your new feature?
You are welcome. Also, feel free to tell me the problems in your testing.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Floating Point Formatting for Scatter Point Chart [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35915:
URL: https://github.com/apache/superset/pull/35915#issuecomment-3469177632
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Floating Point Formatting for Scatter Point Chart [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #35915:
URL: https://github.com/apache/superset/pull/35915#discussion_r2519203224
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/transformProps.test.ts:
##########
@@ -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 { ChartProps, SMART_DATE_ID, supersetTheme } from '@superset-ui/core';
+import transformProps from '../../../src/Timeseries/transformProps';
+import { DEFAULT_FORM_DATA } from '../../../src/Timeseries/constants';
+import {
+ EchartsTimeseriesSeriesType,
+ EchartsTimeseriesFormData,
+ EchartsTimeseriesChartProps,
+} from '../../../src/Timeseries/types';
+import { GenericDataType } from '@apache-superset/core/api/core';
+import {
+ D3_FORMAT_OPTIONS,
+ D3_TIME_FORMAT_OPTIONS,
+} from '@superset-ui/chart-controls';
+
+describe('Scatter Chart X-axis Time Formatting', () => {
+ const baseFormData: EchartsTimeseriesFormData = {
+ ...DEFAULT_FORM_DATA,
+ colorScheme: 'supersetColors',
+ datasource: '1__table',
+ granularity_sqla: '__timestamp',
+ metric: ['column 1'],
+ groupby: [],
+ viz_type: 'echarts_timeseries_scatter',
+ seriesType: EchartsTimeseriesSeriesType.Scatter,
+ };
+
+ const timeseriesData = [
+ {
+ data: [
+ { column_1: 0.72099, __timestamp: 1609459200000 },
+ { column_1: 0.77954, __timestamp: 1612137600000 },
+ { column_1: 2.83434, __timestamp: 1614556800000 },
+ ],
+ colnames: ['column_1', '__timestamp'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Temporal],
+ },
+ ];
+
+ const baseChartPropsConfig = {
+ width: 800,
+ height: 600,
+ queriesData: timeseriesData,
+ theme: supersetTheme,
+ };
+
+ test('xAxisTimeFormat has no default formatter', () => {
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ formData: baseFormData,
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+
+ expect(transformedProps.echartOptions.xAxis).toHaveProperty('axisLabel');
+ const xAxis = transformedProps.echartOptions.xAxis as any;
+ expect(xAxis.axisLabel).toHaveProperty('formatter');
+ expect(xAxis.axisLabel.formatter).toBeUndefined();
+ });
+
+ test.each(D3_TIME_FORMAT_OPTIONS)('should handle %s format', format => {
Review Comment:
The test.each is using an array of tuples but only destructuring the first element. The test title placeholder `%s` will display the entire tuple instead of just the format ID. Either destructure both elements (e.g., `([formatId, label])`) and use `formatId` in the test, or map the options to extract just the format IDs before passing to test.each. For example:
```typescript
test.each(D3_TIME_FORMAT_OPTIONS.map(([id]) => id))('should handle %s format', format => {
```
```suggestion
test.each(D3_TIME_FORMAT_OPTIONS.map(([id]) => id))('should handle %s format', format => {
```
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/controlPanel.test.ts:
##########
@@ -0,0 +1,156 @@
+/**
+ * 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 { ControlPanelsContainerProps } from '@superset-ui/chart-controls/types';
+import controlPanel from '../../../src/Timeseries/Regular/Scatter/controlPanel';
+
+const config = controlPanel;
+
+const getControl = (controlName: string) => {
+ for (const section of config.controlPanelSections) {
+ if (section && section.controlSetRows) {
+ for (const row of section.controlSetRows) {
+ for (const control of row) {
+ if (
+ typeof control === 'object' &&
+ control !== null &&
+ 'name' in control &&
+ control.name === controlName
+ ) {
+ return control;
+ }
+ }
+ }
+ }
+ }
+
+ return null;
+};
+
+const mockControls = (
+ xAxisColumn: string | null,
+ xAxisType: string | null,
+): ControlPanelsContainerProps => {
+ const options = xAxisType
+ ? [{ column_name: xAxisColumn, type: xAxisType }]
+ : [];
+
+ return {
+ controls: {
+ // @ts-ignore
+ x_axis: {
+ value: xAxisColumn,
+ options: options,
+ },
+ },
+ };
+};
+
+// tests for x_axis_time_format control
+const timeFormatControl: any = getControl('x_axis_time_format');
+
+test('scatter chart control panel should include x_axis_time_format control in the panel', () => {
+ expect(timeFormatControl).toBeDefined();
+});
+
+test('scatter chart control panel should have correct default value for x_axis_time_format', () => {
+ expect(timeFormatControl).toBeDefined();
+ expect(timeFormatControl.config).toBeDefined();
+ expect(timeFormatControl.config.default).toBe('smart_date');
+});
+
+test('scatter chart control panel should have visibility function for x_axis_time_format', () => {
+ expect(timeFormatControl).toBeDefined();
+ expect(timeFormatControl.config.visibility).toBeDefined();
+ expect(typeof timeFormatControl.config.visibility).toBe('function');
+
+ // The visibility function exists - the exact logic is tested implicitly through UI behavior
+ // The important part is that the control has proper visibility configuration
+});
+
+const isTimeVisible = (
+ xAxisColumn: string | null,
+ xAxisType: string | null,
+): boolean => {
+ const props = mockControls(xAxisColumn, xAxisType);
+ const visibilityFn = timeFormatControl?.config?.visibility;
+ return visibilityFn ? visibilityFn(props) : false;
+};
+
+test('x_axis_time_format control should be visible for any data types include TIME', () => {
+ expect(isTimeVisible('time_column', 'TIME')).toBe(true);
+ expect(isTimeVisible('time_column', 'TIME WITH TIME ZONE')).toBe(true);
+ expect(isTimeVisible('time_column', 'TIMESTAMP WITH TIME ZONE')).toBe(true);
+ expect(isTimeVisible('time_column', 'TIMESTAMP WITHOUT TIME ZONE')).toBe(
+ true,
+ );
+});
+
+test('x_axis_time_format control should be hidden for any data types include TIME', () => {
Review Comment:
The test description is misleading. It says "should be hidden for any data types include TIME" but the test cases are actually checking for non-time data types (null, FLOAT). The description should say "should be hidden for data types that do NOT include TIME" or "should be hidden for non-time data types".
```suggestion
test('x_axis_time_format control should be hidden for data types that do NOT include TIME', () => {
```
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/transformProps.test.ts:
##########
@@ -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 { ChartProps, SMART_DATE_ID, supersetTheme } from '@superset-ui/core';
+import transformProps from '../../../src/Timeseries/transformProps';
+import { DEFAULT_FORM_DATA } from '../../../src/Timeseries/constants';
+import {
+ EchartsTimeseriesSeriesType,
+ EchartsTimeseriesFormData,
+ EchartsTimeseriesChartProps,
+} from '../../../src/Timeseries/types';
+import { GenericDataType } from '@apache-superset/core/api/core';
+import {
+ D3_FORMAT_OPTIONS,
+ D3_TIME_FORMAT_OPTIONS,
+} from '@superset-ui/chart-controls';
+
+describe('Scatter Chart X-axis Time Formatting', () => {
+ const baseFormData: EchartsTimeseriesFormData = {
+ ...DEFAULT_FORM_DATA,
+ colorScheme: 'supersetColors',
+ datasource: '1__table',
+ granularity_sqla: '__timestamp',
+ metric: ['column 1'],
+ groupby: [],
+ viz_type: 'echarts_timeseries_scatter',
+ seriesType: EchartsTimeseriesSeriesType.Scatter,
+ };
+
+ const timeseriesData = [
+ {
+ data: [
+ { column_1: 0.72099, __timestamp: 1609459200000 },
+ { column_1: 0.77954, __timestamp: 1612137600000 },
+ { column_1: 2.83434, __timestamp: 1614556800000 },
+ ],
+ colnames: ['column_1', '__timestamp'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Temporal],
+ },
+ ];
+
+ const baseChartPropsConfig = {
+ width: 800,
+ height: 600,
+ queriesData: timeseriesData,
+ theme: supersetTheme,
+ };
+
+ test('xAxisTimeFormat has no default formatter', () => {
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ formData: baseFormData,
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+
+ expect(transformedProps.echartOptions.xAxis).toHaveProperty('axisLabel');
+ const xAxis = transformedProps.echartOptions.xAxis as any;
+ expect(xAxis.axisLabel).toHaveProperty('formatter');
+ expect(xAxis.axisLabel.formatter).toBeUndefined();
+ });
+
+ test.each(D3_TIME_FORMAT_OPTIONS)('should handle %s format', format => {
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ formData: {
+ ...baseFormData,
+ xAxisTimeFormat: format,
+ },
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+
+ const xAxis = transformedProps.echartOptions.xAxis as any;
+ expect(xAxis.axisLabel).toHaveProperty('formatter');
+ if (format === SMART_DATE_ID) {
+ expect(xAxis.axisLabel.formatter).toBeUndefined();
+ } else {
+ expect(typeof xAxis.axisLabel.formatter).toBe('function');
+ expect(xAxis.axisLabel.formatter.id).toBe(format);
+ }
+ });
+});
+
+describe('Scatter Chart X-axis Number Formatting', () => {
+ const baseFormData: EchartsTimeseriesFormData = {
+ ...DEFAULT_FORM_DATA,
+ colorScheme: 'supersetColors',
+ datasource: '1__table',
+ metric: ['column_1'],
+ x_axis: 'column_2',
+ groupby: [],
+ viz_type: 'echarts_timeseries_scatter',
+ seriesType: EchartsTimeseriesSeriesType.Scatter,
+ };
+
+ const timeseriesData = [
+ {
+ data: [
+ { column_1: 0.72099, column_2: 3.01699 },
+ { column_1: 0.77954, column_2: 3.44802 },
+ { column_1: 2.83434, column_2: 3.58095 },
+ ],
+ colnames: ['column_1', 'column_2'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Numeric],
+ },
+ ];
+
+ const baseChartPropsConfig = {
+ width: 800,
+ height: 600,
+ queriesData: timeseriesData,
+ theme: supersetTheme,
+ };
+
+ test('should use SMART_NUMBER as default xAxisNumberFormat', () => {
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ formData: baseFormData,
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+
+ expect(transformedProps.echartOptions.xAxis).toHaveProperty('axisLabel');
+ const xAxis = transformedProps.echartOptions.xAxis as any;
+ expect(xAxis.axisLabel).toHaveProperty('formatter');
+ expect(typeof xAxis.axisLabel.formatter).toBe('function');
+ expect(xAxis.axisLabel.formatter.id).toBe('SMART_NUMBER');
+ });
+
+ test.each(D3_FORMAT_OPTIONS)('should handle %s format', format => {
Review Comment:
The test.each is using an array of tuples but only destructuring the first element. The test title placeholder `%s` will display the entire tuple instead of just the format ID. Either destructure both elements (e.g., `([formatId, label])`) and use `formatId` in the test, or map the options to extract just the format IDs before passing to test.each. For example:
```typescript
test.each(D3_FORMAT_OPTIONS.map(([id]) => id))('should handle %s format', format => {
```
```suggestion
test.each(D3_FORMAT_OPTIONS.map(([id]) => id))('should handle %s format', format => {
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Floating Point Formatting for Scatter Point Chart [superset]
Posted by "alex241728 (via GitHub)" <gi...@apache.org>.
alex241728 commented on PR #35915:
URL: https://github.com/apache/superset/pull/35915#issuecomment-3513716759
> For tests, we should move away from `it()` and use `test()` as done in this PR: https://github.com/apache/superset/pull/35305/files
@sadpandajoe @rusackas The test format has been fixed.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Floating Point Formatting for Scatter Point Chart [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #35915:
URL: https://github.com/apache/superset/pull/35915#issuecomment-3498802598
For tests, we should move away from `it()` and use `test()` as done in this PR: https://github.com/apache/superset/pull/35305/files
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Floating Point Formatting for Scatter Point Chart [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #35915:
URL: https://github.com/apache/superset/pull/35915
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Floating Point Formatting for Scatter Point Chart [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35915:
URL: https://github.com/apache/superset/pull/35915#issuecomment-3530327267
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/19350124814) for [feeb12a](https://github.com/apache/superset/commit/feeb12a)
• **Environment:** http://35.89.8.79:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Floating Point Formatting for Scatter Point Chart [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35915:
URL: https://github.com/apache/superset/pull/35915#issuecomment-3530290934
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/19350124814) for [feeb12a](https://github.com/apache/superset/commit/feeb12a)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Floating Point Formatting for Scatter Point Chart [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #35915:
URL: https://github.com/apache/superset/pull/35915#issuecomment-3481900154
Superset uses Git pre-commit hooks courtesy of [pre-commit](https://pre-commit.com/). To install run the following:
```
pip3 install -r requirements/development.txt
pre-commit install
```
A series of checks will now run when you make a git commit.
Alternatively it is possible to run pre-commit by running pre-commit manually:
```
pre-commit run --all-files
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Floating Point Formatting for Scatter Point Chart [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #35915:
URL: https://github.com/apache/superset/pull/35915#discussion_r2478949749
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx:
##########
@@ -112,8 +112,49 @@ const config: ControlPanelConfig = {
...sharedControls.x_axis_time_format,
default: 'smart_date',
description: `${D3_TIME_FORMAT_DOCS}. ${TIME_SERIES_DESCRIPTION_TEXT}`,
+ visibility: ({ controls }: ControlPanelsContainerProps) => {
+ // check if x axis is a time column
+ const xAxisColumn = controls?.x_axis?.value;
+ const xAxisOptions = controls?.x_axis?.options;
+
+ if (!xAxisColumn || !Array.isArray(xAxisOptions))
+ {
+ return false;
+ }
+
+ const xAxisType = xAxisOptions.find(option => option.column_name === xAxisColumn)?.type;
+
+ return typeof xAxisType === 'string' && xAxisType.toUpperCase().includes('TIME');
Review Comment:
### Inefficient linear search in visibility function <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The visibility function performs a linear search through xAxisOptions array on every render to find the matching column type.
###### Why this matters
This creates O(n) time complexity that executes repeatedly during UI interactions, potentially causing performance degradation with large datasets or frequent re-renders.
###### Suggested change ∙ *Feature Preview*
Cache the result or use a Map/object lookup for O(1) access. Consider memoizing the visibility function result:
```typescript
const xAxisTypeMap = useMemo(() =>
new Map(xAxisOptions?.map(option => [option.column_name, option.type]) || []),
[xAxisOptions]
);
const xAxisType = xAxisTypeMap.get(xAxisColumn);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/07378c51-9930-426d-9d6b-6edd60f2f393/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/07378c51-9930-426d-9d6b-6edd60f2f393?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/07378c51-9930-426d-9d6b-6edd60f2f393?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/07378c51-9930-426d-9d6b-6edd60f2f393?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/07378c51-9930-426d-9d6b-6edd60f2f393)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:f2b38014-c44f-4595-b9ae-e6086b6c3c03 -->
[](f2b38014-c44f-4595-b9ae-e6086b6c3c03)
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx:
##########
@@ -112,8 +112,49 @@ const config: ControlPanelConfig = {
...sharedControls.x_axis_time_format,
default: 'smart_date',
description: `${D3_TIME_FORMAT_DOCS}. ${TIME_SERIES_DESCRIPTION_TEXT}`,
+ visibility: ({ controls }: ControlPanelsContainerProps) => {
+ // check if x axis is a time column
+ const xAxisColumn = controls?.x_axis?.value;
+ const xAxisOptions = controls?.x_axis?.options;
+
+ if (!xAxisColumn || !Array.isArray(xAxisOptions))
+ {
+ return false;
+ }
+
+ const xAxisType = xAxisOptions.find(option => option.column_name === xAxisColumn)?.type;
+
+ return typeof xAxisType === 'string' && xAxisType.toUpperCase().includes('TIME');
+ },
},
},
+ {
+ name: 'x_axis_number_format',
+ config: {
+ ...sharedControls.x_axis_number_format,
+ visibility: ({ controls }: ControlPanelsContainerProps) => {
+ // check if x axis is a floating-point column
+ const xAxisColumn = controls?.x_axis?.value;
+ const xAxisOptions = controls?.x_axis?.options;
+
+ if (!xAxisColumn || !Array.isArray(xAxisOptions))
+ {
+ return false;
+ }
+
+ const xAxisType = xAxisOptions.find(option => option.column_name === xAxisColumn)?.type;
+
+ if (typeof xAxisType !== 'string')
+ {
+ return false;
+ }
+
+ const typeUpper = xAxisType.toUpperCase();
+
+ return ['FLOAT', 'DOUBLE', 'REAL', 'NUMERIC', 'DECIMAL'].some(t => typeUpper.includes(t));
Review Comment:
### Overly permissive numeric type detection <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The type checking logic uses string inclusion which can produce false positives for database types that contain these keywords as substrings.
###### Why this matters
This could incorrectly show the number format control for non-numeric types like 'FLOAT_TEXT' or 'DOUBLE_PRECISION_STRING', leading to unexpected UI behavior and potential formatting errors.
###### Suggested change ∙ *Feature Preview*
Use exact type matching or more precise pattern matching:
```typescript
const numericTypes = ['FLOAT', 'DOUBLE', 'REAL', 'NUMERIC', 'DECIMAL'];
return numericTypes.some(t =>
typeUpper === t ||
typeUpper.startsWith(t + '(') ||
typeUpper === t + '64' ||
typeUpper === t + '32'
);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a6ff7936-ed89-4a81-abd5-cd384717b2a2/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a6ff7936-ed89-4a81-abd5-cd384717b2a2?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a6ff7936-ed89-4a81-abd5-cd384717b2a2?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a6ff7936-ed89-4a81-abd5-cd384717b2a2?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a6ff7936-ed89-4a81-abd5-cd384717b2a2)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:9960cfc4-bd40-447c-9a23-e996b521cc17 -->
[](9960cfc4-bd40-447c-9a23-e996b521cc17)
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx:
##########
@@ -112,8 +112,49 @@ const config: ControlPanelConfig = {
...sharedControls.x_axis_time_format,
default: 'smart_date',
description: `${D3_TIME_FORMAT_DOCS}. ${TIME_SERIES_DESCRIPTION_TEXT}`,
+ visibility: ({ controls }: ControlPanelsContainerProps) => {
+ // check if x axis is a time column
+ const xAxisColumn = controls?.x_axis?.value;
+ const xAxisOptions = controls?.x_axis?.options;
+
+ if (!xAxisColumn || !Array.isArray(xAxisOptions))
+ {
+ return false;
+ }
+
+ const xAxisType = xAxisOptions.find(option => option.column_name === xAxisColumn)?.type;
+
+ return typeof xAxisType === 'string' && xAxisType.toUpperCase().includes('TIME');
+ },
Review Comment:
### Duplicated Column Type Checking Logic <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The visibility logic for x_axis_time_format and x_axis_number_format controls is duplicated with only minor differences in the type checking.
###### Why this matters
Code duplication increases maintenance burden and risk of inconsistencies when changes are needed. If the column type checking logic needs to change, it would need to be updated in multiple places.
###### Suggested change ∙ *Feature Preview*
Extract the common column type checking logic into a reusable function:
```typescript
const getColumnType = (controls: any) => {
const xAxisColumn = controls?.x_axis?.value;
const xAxisOptions = controls?.x_axis?.options;
if (!xAxisColumn || !Array.isArray(xAxisOptions)) {
return null;
}
return xAxisOptions.find(option => option.column_name === xAxisColumn)?.type;
};
// Then use it in visibility checks:
visibility: ({ controls }) => {
const xAxisType = getColumnType(controls);
return typeof xAxisType === 'string' && xAxisType.toUpperCase().includes('TIME');
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5c7d25f4-6f47-4721-80f5-6343526de003/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5c7d25f4-6f47-4721-80f5-6343526de003?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5c7d25f4-6f47-4721-80f5-6343526de003?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5c7d25f4-6f47-4721-80f5-6343526de003?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5c7d25f4-6f47-4721-80f5-6343526de003)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:f18789c9-5753-4a15-88a1-26a3b34d5641 -->
[](f18789c9-5753-4a15-88a1-26a3b34d5641)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Floating Point Formatting for Scatter Point Chart [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #35915:
URL: https://github.com/apache/superset/pull/35915#discussion_r2478956795
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx:
##########
@@ -112,8 +112,49 @@ const config: ControlPanelConfig = {
...sharedControls.x_axis_time_format,
default: 'smart_date',
description: `${D3_TIME_FORMAT_DOCS}. ${TIME_SERIES_DESCRIPTION_TEXT}`,
+ visibility: ({ controls }: ControlPanelsContainerProps) => {
+ // check if x axis is a time column
+ const xAxisColumn = controls?.x_axis?.value;
+ const xAxisOptions = controls?.x_axis?.options;
+
+ if (!xAxisColumn || !Array.isArray(xAxisOptions))
+ {
+ return false;
+ }
+
+ const xAxisType = xAxisOptions.find(option => option.column_name === xAxisColumn)?.type;
+
+ return typeof xAxisType === 'string' && xAxisType.toUpperCase().includes('TIME');
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incorrect time format visibility for DATE columns</b></div>
<div id="fix">
The visibility logic for the x_axis_time_format control only checks if the column type includes 'TIME', which excludes DATE columns. However, DATE is a time-related data type that should also have access to time formatting options. This prevents users from formatting DATE columns properly on the x-axis, impacting chart usability for date-based data. The fix extends the condition to include types containing 'DATE' as well.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
return typeof xAxisType === 'string' && (xAxisType.toUpperCase().includes('TIME') || xAxisType.toUpperCase().includes('DATE'));
````
</div>
</details>
</div>
<small><i>Code Review Run <a href=https://github.com/apache/superset/pull/35915#issuecomment-3469177632>#2e97c7</a></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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Floating Point Formatting for Scatter Point Chart [superset]
Posted by "alex241728 (via GitHub)" <gi...@apache.org>.
alex241728 commented on PR #35915:
URL: https://github.com/apache/superset/pull/35915#issuecomment-3523948006
> Looking at some of the failing CI stuff, this PR _might_ need a rebase, pulling in the latest changes from `master` to resolve a couple of the items (at least I hope it will fix the typescript issue I'm currently seeing).
I have solved the suggestions provided by copilot. However, there is one thing I cannot solve in the pre-commit checks, which is provided in the quote. By the way, the transformProps.test for bar chart contains the same superTheme from the same file. It has the type error too.
> Type checking 3 changed TypeScript files...
Running targeted type check on 3 files...
plugins/plugin-chart-echarts/test/Timeseries/Bar/transformProps.test.ts:20:31 - error TS6305: Output file '/Users/alex241728/superset/superset-frontend/packages/superset-core/lib/ui/index.d.ts' has not been built from source file '/Users/alex241728/superset/superset-frontend/packages/superset-core/src/ui/index.ts'.
20 import { supersetTheme } from '@apache-superset/core/ui';
~~~~~~~~~~~~~~~~~~~~~~~~~~
plugins/plugin-chart-echarts/test/Timeseries/Scatter/transformProps.test.ts:33:31 - error TS6305: Output file '/Users/alex241728/superset/superset-frontend/packages/superset-core/lib/ui/index.d.ts' has not been built from source file '/Users/alex241728/superset/superset-frontend/packages/superset-core/src/ui/index.ts'.
33 import { supersetTheme } from '@apache-superset/core/ui';
~~~~~~~~~~~~~~~~~~~~~~~~~~
Found 2 errors in 2 files.
Errors Files
1 plugins/plugin-chart-echarts/test/Timeseries/Bar/transformProps.test.ts:20
1 plugins/plugin-chart-echarts/test/Timeseries/Scatter/transformProps.test.ts:33
Failed to execute type checking: Type check failed with exit code 2
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Floating Point Formatting for Scatter Point Chart [superset]
Posted by "alex241728 (via GitHub)" <gi...@apache.org>.
alex241728 commented on PR #35915:
URL: https://github.com/apache/superset/pull/35915#issuecomment-3494152036
> Re-running CI 🤞
I have completed the tests and rerun the precommit checks. I think the tests and checks are all passed. Can you check the pull request again. @rusackas
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Floating Point Formatting for Scatter Point Chart [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #35915:
URL: https://github.com/apache/superset/pull/35915#issuecomment-3469393093
@alex241728 thanks for creating the PR. Do you mind adding tests to your new feature?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Floating Point Formatting for Scatter Point Chart [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #35915:
URL: https://github.com/apache/superset/pull/35915#issuecomment-3523422407
Looking at some of the failing CI stuff, this PR *might* need a rebase, pulling in the latest changes from `master` to resolve a couple of the items (at least I hope it will fix the typescript issue I'm currently seeing).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Floating Point Formatting for Scatter Point Chart [superset]
Posted by "alex241728 (via GitHub)" <gi...@apache.org>.
alex241728 commented on PR #35915:
URL: https://github.com/apache/superset/pull/35915#issuecomment-3528286728
> > Looking at some of the failing CI stuff, this PR _might_ need a rebase, pulling in the latest changes from `master` to resolve a couple of the items (at least I hope it will fix the typescript issue I'm currently seeing).
>
> @rusackas I have solved the suggestions provided by copilot. However, there is one thing I cannot solve in the pre-commit checks, which is provided in the quote. By the way, the transformProps.test for bar chart contains the same superTheme from the same file. It has the type error too.
>
> > Type checking 3 changed TypeScript files...
> > Running targeted type check on 3 files...
> > plugins/plugin-chart-echarts/test/Timeseries/Bar/transformProps.test.ts:20:31 - error TS6305: Output file '/Users/alex241728/superset/superset-frontend/packages/superset-core/lib/ui/index.d.ts' has not been built from source file '/Users/alex241728/superset/superset-frontend/packages/superset-core/src/ui/index.ts'.
> > 20 import { supersetTheme } from '@apache-superset/core/ui';
> > ~~~~~~~~~~~~~~~~~~~~~~~~~~
> > plugins/plugin-chart-echarts/test/Timeseries/Scatter/transformProps.test.ts:33:31 - error TS6305: Output file '/Users/alex241728/superset/superset-frontend/packages/superset-core/lib/ui/index.d.ts' has not been built from source file '/Users/alex241728/superset/superset-frontend/packages/superset-core/src/ui/index.ts'.
> > 33 import { supersetTheme } from '@apache-superset/core/ui';
> > ~~~~~~~~~~~~~~~~~~~~~~~~~~
> > Found 2 errors in 2 files.
> > Errors Files
> > 1 plugins/plugin-chart-echarts/test/Timeseries/Bar/transformProps.test.ts:20
> > 1 plugins/plugin-chart-echarts/test/Timeseries/Scatter/transformProps.test.ts:33
> > Failed to execute type checking: Type check failed with exit code 2
The error is fixed by
>cd superset-frontend
>npm run plugins:build
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org