You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "rusackas (via GitHub)" <gi...@apache.org> on 2026/06/11 07:49:40 UTC
[PR] feat(scatter): add chart orientation and dot size metric controls [superset]
rusackas opened a new pull request, #40967:
URL: https://github.com/apache/superset/pull/40967
### SUMMARY
Adds two long-requested capabilities to the ECharts Scatter chart (`echarts_timeseries_scatter`):
**1. Chart orientation (categorical y-axis support)**
A new Vertical/Horizontal orientation control, reusing the shared Timeseries horizontal-orientation plumbing that the Bar chart already uses. Horizontal orientation places the dimension (x-axis column) on the y-axis and the metric on the x-axis — so a categorical (or temporal/numeric) y-axis is one click away, and combined with the existing "Force categorical" control even numeric columns can be treated as categories. Axis controls (formats, label rotation, log axis, bounds, titles) swap between the X Axis and Y Axis panel sections based on orientation, mirroring the Bar chart's control panel behavior.
**2. Dot size metric with area-based scaling**
An optional "Dot size metric" control in the Query section, plus "Minimum dot size" / "Maximum dot size" sliders (defaults 5/30) that appear when the metric is set. Design notes:
- The size metric flows into the query through the existing `size → metrics` query-field alias, so no `buildQuery` change was needed; it is deduped when it matches a value metric (in which case each point's own value doubles as its size value).
- The size metric's series are excluded from rendering and the legend; their values feed per-point `symbolSize` callbacks, matched to value series by dimension key for grouped charts.
- Scaling is **area-based** (`getAreaScaledSymbolSize` in `utils/series.ts`): marker *area* scales linearly with the metric between the configured bounds, i.e. radius ∝ √value. This avoids the perceptual exaggeration of diameter-linear scaling (where a 2× value reads as a 4× area) — notably the existing Bubble chart scales linearly on radius.
- Points lacking a size value (e.g. nulls, time-shift series) fall back to the fixed marker size.
Both features compose (horizontal orientation + sized dots) and are covered by unit tests.
Possible follow-ups (intentionally out of scope): showing the size value in the tooltip, and adopting area-based scaling in the Bubble chart.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before: the Scatter chart only offered a fixed marker-size slider and a vertical layout.
After: orientation radio control under "Chart Orientation", "Dot size metric" in the Query section, and min/max dot size sliders under Chart Options.
### TESTING INSTRUCTIONS
1. Create a Scatter Plot chart with a categorical X-axis column and a metric.
2. In **Customize → Chart Orientation** (Data tab → "Chart Orientation" section), switch to **Horizontal** — the categories move to the y-axis and the metric to the x-axis; axis format/title controls follow their axes.
3. Back in the Query section, set a **Dot size metric** (e.g. `COUNT(*)`); dots resize so their areas are proportional to the metric, scaled between the **Minimum/Maximum dot size** sliders that appear under Chart Options.
4. Add a "Dimensions" (group by) column and confirm each series' dots size independently against a shared global scale.
5. Unit tests: `npx jest plugins/plugin-chart-echarts/test/Timeseries plugins/plugin-chart-echarts/test/utils/series.test.ts`
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [x] 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
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40967:
URL: https://github.com/apache/superset/pull/40967#discussion_r3400608057
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/transformProps.test.ts:
##########
@@ -175,3 +175,283 @@ describe('Scatter Chart X-axis Number Formatting', () => {
},
);
});
+
+describe('Scatter Chart Orientation and Dot Size Metric', () => {
+ const baseFormData: EchartsTimeseriesFormData = {
+ ...DEFAULT_FORM_DATA,
+ colorScheme: 'supersetColors',
+ datasource: '1__table',
+ metrics: ['sum_val'],
+ x_axis: 'category_col',
+ groupby: [],
+ viz_type: 'echarts_timeseries_scatter',
+ seriesType: EchartsTimeseriesSeriesType.Scatter,
+ };
+
+ const categoricalData = [
+ {
+ data: [
+ { category_col: 'A', sum_val: 1, size_metric: 10 },
+ { category_col: 'B', sum_val: 2, size_metric: 25 },
+ { category_col: 'C', sum_val: 3, size_metric: 40 },
+ ],
+ colnames: ['category_col', 'sum_val', 'size_metric'],
+ coltypes: [
+ GenericDataType.String,
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ ],
+ label_map: {
+ category_col: ['category_col'],
+ sum_val: ['sum_val'],
+ size_metric: ['size_metric'],
+ },
+ },
+ ];
+
+ const baseChartPropsConfig = {
+ width: 800,
+ height: 600,
+ theme: supersetTheme,
+ };
+
+ const getScatterSeries = (props: ReturnType<typeof transformProps>) =>
+ (props.echartOptions.series as any[]).filter(s => s.type === 'scatter');
+
+ const singleMetricData = [
+ {
+ ...categoricalData[0],
+ data: categoricalData[0].data.map(row => ({
+ category_col: row.category_col,
+ sum_val: row.sum_val,
+ })),
+ colnames: ['category_col', 'sum_val'],
+ coltypes: [GenericDataType.String, GenericDataType.Numeric],
+ },
+ ];
+
+ test('horizontal orientation swaps the dimension axis onto the y-axis', () => {
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: singleMetricData,
+ formData: { ...baseFormData, orientation: 'horizontal' },
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const { xAxis, yAxis } = transformedProps.echartOptions as any;
+ expect(yAxis.type).toBe('category');
+ expect(xAxis.type).toBe('value');
+
+ const series = getScatterSeries(transformedProps);
+ expect(series).toHaveLength(1);
+ // data points are flipped to [metric, dimension]
+ expect(series[0].data[0]).toEqual([1, 'A']);
+ });
+
+ test('vertical orientation keeps the dimension axis on the x-axis', () => {
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: singleMetricData,
+ formData: baseFormData,
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const { xAxis, yAxis } = transformedProps.echartOptions as any;
Review Comment:
**Suggestion:** Remove the second `any` cast for axis destructuring by typing the chart options object with an explicit ECharts options interface. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is another direct `any` cast in TypeScript test code. The custom rule requires avoiding `any`, so the violation is real.
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c3606d0397594e199df03b4d20e79bb9&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=c3606d0397594e199df03b4d20e79bb9&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/transformProps.test.ts
**Line:** 263:263
**Comment:**
*Custom Rule: Remove the second `any` cast for axis destructuring by typing the chart options object with an explicit ECharts options interface.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=c01420d9b1bb9952dd1d6edca3144fd7f0455bd78aab0d54914fa7e8a3fe5a9d&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=c01420d9b1bb9952dd1d6edca3144fd7f0455bd78aab0d54914fa7e8a3fe5a9d&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/controlPanel.test.ts:
##########
@@ -148,3 +148,59 @@ test('x_axis_number_format control should be hidden for non-numeric data types',
expect(isNumberVisible(null, null)).toBe(false);
expect(isNumberVisible('time_column', GenericDataType.Temporal)).toBe(false);
});
+
+// tests for orientation and dot size controls
+const orientationControl: any = getControl('orientation');
+const sizeControl: any = getControl('size');
+const minMarkerSizeControl: any = getControl('minMarkerSize');
+const maxMarkerSizeControl: any = getControl('maxMarkerSize');
+
+test('scatter chart control panel should include an orientation control defaulting to vertical', () => {
+ expect(orientationControl).toBeDefined();
+ expect(orientationControl.config.default).toBe('vertical');
+ expect(orientationControl.config.options).toEqual([
+ ['vertical', expect.anything()],
+ ['horizontal', expect.anything()],
+ ]);
+});
+
+test('scatter chart control panel should include an optional dot size metric control', () => {
+ expect(sizeControl).toBeDefined();
+ expect(sizeControl.config.validators).toEqual([]);
+ expect(sizeControl.config.default).toBeNull();
+});
+
+const mockSizeControls = (
+ sizeValue: string | null,
+): ControlPanelsContainerProps =>
+ ({
+ controls: {
+ size: { value: sizeValue },
+ markerEnabled: { value: true },
+ },
+ }) as unknown as ControlPanelsContainerProps;
+
+test('dot size range controls should only be visible when a size metric is set', () => {
+ expect(minMarkerSizeControl.config.visibility(mockSizeControls(null))).toBe(
+ false,
+ );
+ expect(maxMarkerSizeControl.config.visibility(mockSizeControls(null))).toBe(
+ false,
+ );
+ expect(
+ minMarkerSizeControl.config.visibility(mockSizeControls('size_metric')),
+ ).toBe(true);
+ expect(
+ maxMarkerSizeControl.config.visibility(mockSizeControls('size_metric')),
+ ).toBe(true);
+});
+
+test('fixed marker size control should hide when a size metric is set', () => {
+ const markerSizeControl: any = getControl('markerSize');
Review Comment:
**Suggestion:** Change this `any` usage to the same concrete control type used elsewhere in the test file to avoid bypassing type checks in visibility assertions. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a direct `any` annotation in a modified TypeScript test file, so it violates the custom rule against using `any` in new or changed TS/TSX code.
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=287f0bf684ba49938d07a3b0808b58cc&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=287f0bf684ba49938d07a3b0808b58cc&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/controlPanel.test.ts
**Line:** 199:199
**Comment:**
*Custom Rule: Change this `any` usage to the same concrete control type used elsewhere in the test file to avoid bypassing type checks in visibility assertions.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=58291e70b650a86c9c76b46093943267b291a40189c0926dba161863cad1f061&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=58291e70b650a86c9c76b46093943267b291a40189c0926dba161863cad1f061&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/transformProps.test.ts:
##########
@@ -175,3 +175,283 @@ describe('Scatter Chart X-axis Number Formatting', () => {
},
);
});
+
+describe('Scatter Chart Orientation and Dot Size Metric', () => {
+ const baseFormData: EchartsTimeseriesFormData = {
+ ...DEFAULT_FORM_DATA,
+ colorScheme: 'supersetColors',
+ datasource: '1__table',
+ metrics: ['sum_val'],
+ x_axis: 'category_col',
+ groupby: [],
+ viz_type: 'echarts_timeseries_scatter',
+ seriesType: EchartsTimeseriesSeriesType.Scatter,
+ };
+
+ const categoricalData = [
+ {
+ data: [
+ { category_col: 'A', sum_val: 1, size_metric: 10 },
+ { category_col: 'B', sum_val: 2, size_metric: 25 },
+ { category_col: 'C', sum_val: 3, size_metric: 40 },
+ ],
+ colnames: ['category_col', 'sum_val', 'size_metric'],
+ coltypes: [
+ GenericDataType.String,
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ ],
+ label_map: {
+ category_col: ['category_col'],
+ sum_val: ['sum_val'],
+ size_metric: ['size_metric'],
+ },
+ },
+ ];
+
+ const baseChartPropsConfig = {
+ width: 800,
+ height: 600,
+ theme: supersetTheme,
+ };
+
+ const getScatterSeries = (props: ReturnType<typeof transformProps>) =>
+ (props.echartOptions.series as any[]).filter(s => s.type === 'scatter');
Review Comment:
**Suggestion:** Replace the `any[]` cast on the series collection with a concrete ECharts series type so scatter filtering is type-safe. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is TypeScript test code and it uses `any[]` directly in a cast. The custom rule explicitly flags any new or modified TypeScript/TSX code that uses `any`, so this is a real violation.
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ae7feb05f8fa440e92527820d58dd0c8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=ae7feb05f8fa440e92527820d58dd0c8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/transformProps.test.ts
**Line:** 218:219
**Comment:**
*Custom Rule: Replace the `any[]` cast on the series collection with a concrete ECharts series type so scatter filtering is type-safe.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=8be97440ad089f23634ec6575640b5352aa1727fe444ffa4350ddcca405c8710&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=8be97440ad089f23634ec6575640b5352aa1727fe444ffa4350ddcca405c8710&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/controlPanel.test.ts:
##########
@@ -148,3 +148,59 @@ test('x_axis_number_format control should be hidden for non-numeric data types',
expect(isNumberVisible(null, null)).toBe(false);
expect(isNumberVisible('time_column', GenericDataType.Temporal)).toBe(false);
});
+
+// tests for orientation and dot size controls
+const orientationControl: any = getControl('orientation');
+const sizeControl: any = getControl('size');
+const minMarkerSizeControl: any = getControl('minMarkerSize');
+const maxMarkerSizeControl: any = getControl('maxMarkerSize');
Review Comment:
**Suggestion:** Replace these `any` annotations with a concrete control type (or a shared reusable control interface from the chart controls types) so the assertions on `config` and `visibility` are type-safe. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The file is a TypeScript test file, and these newly added declarations use the banned `any` type directly. This matches the custom rule requiring a concrete, reusable, or narrower type instead of `any`.
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1357b71bf0644e18b35999d4fdd14711&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=1357b71bf0644e18b35999d4fdd14711&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/controlPanel.test.ts
**Line:** 152:156
**Comment:**
*Custom Rule: Replace these `any` annotations with a concrete control type (or a shared reusable control interface from the chart controls types) so the assertions on `config` and `visibility` are type-safe.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=bca55730d1805d4943eccfddec623a0d84e58155ed273c896ed4bd3da08c2898&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=bca55730d1805d4943eccfddec623a0d84e58155ed273c896ed4bd3da08c2898&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/transformProps.test.ts:
##########
@@ -175,3 +175,283 @@ describe('Scatter Chart X-axis Number Formatting', () => {
},
);
});
+
+describe('Scatter Chart Orientation and Dot Size Metric', () => {
+ const baseFormData: EchartsTimeseriesFormData = {
+ ...DEFAULT_FORM_DATA,
+ colorScheme: 'supersetColors',
+ datasource: '1__table',
+ metrics: ['sum_val'],
+ x_axis: 'category_col',
+ groupby: [],
+ viz_type: 'echarts_timeseries_scatter',
+ seriesType: EchartsTimeseriesSeriesType.Scatter,
+ };
+
+ const categoricalData = [
+ {
+ data: [
+ { category_col: 'A', sum_val: 1, size_metric: 10 },
+ { category_col: 'B', sum_val: 2, size_metric: 25 },
+ { category_col: 'C', sum_val: 3, size_metric: 40 },
+ ],
+ colnames: ['category_col', 'sum_val', 'size_metric'],
+ coltypes: [
+ GenericDataType.String,
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ ],
+ label_map: {
+ category_col: ['category_col'],
+ sum_val: ['sum_val'],
+ size_metric: ['size_metric'],
+ },
+ },
+ ];
+
+ const baseChartPropsConfig = {
+ width: 800,
+ height: 600,
+ theme: supersetTheme,
+ };
+
+ const getScatterSeries = (props: ReturnType<typeof transformProps>) =>
+ (props.echartOptions.series as any[]).filter(s => s.type === 'scatter');
+
+ const singleMetricData = [
+ {
+ ...categoricalData[0],
+ data: categoricalData[0].data.map(row => ({
+ category_col: row.category_col,
+ sum_val: row.sum_val,
+ })),
+ colnames: ['category_col', 'sum_val'],
+ coltypes: [GenericDataType.String, GenericDataType.Numeric],
+ },
+ ];
+
+ test('horizontal orientation swaps the dimension axis onto the y-axis', () => {
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: singleMetricData,
+ formData: { ...baseFormData, orientation: 'horizontal' },
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const { xAxis, yAxis } = transformedProps.echartOptions as any;
Review Comment:
**Suggestion:** Avoid casting axis config to `any`; use the proper ECharts axis option type (or a narrowed union) for the destructured axis objects. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The code casts the chart options object to `any`, which is explicitly disallowed by the rule for TypeScript/TSX changes. This is a direct and present violation.
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=fd5a87534e5f4d5aaa37fad3a3cb57ad&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=fd5a87534e5f4d5aaa37fad3a3cb57ad&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/transformProps.test.ts
**Line:** 243:243
**Comment:**
*Custom Rule: Avoid casting axis config to `any`; use the proper ECharts axis option type (or a narrowed union) for the destructured axis objects.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=23ec214352420cb43514444aaa1b7dc7f05a03b2f4741d77ffab9e38464069e0&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=23ec214352420cb43514444aaa1b7dc7f05a03b2f4741d77ffab9e38464069e0&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/transformProps.test.ts:
##########
@@ -175,3 +175,283 @@ describe('Scatter Chart X-axis Number Formatting', () => {
},
);
});
+
+describe('Scatter Chart Orientation and Dot Size Metric', () => {
+ const baseFormData: EchartsTimeseriesFormData = {
+ ...DEFAULT_FORM_DATA,
+ colorScheme: 'supersetColors',
+ datasource: '1__table',
+ metrics: ['sum_val'],
+ x_axis: 'category_col',
+ groupby: [],
+ viz_type: 'echarts_timeseries_scatter',
+ seriesType: EchartsTimeseriesSeriesType.Scatter,
+ };
+
+ const categoricalData = [
+ {
+ data: [
+ { category_col: 'A', sum_val: 1, size_metric: 10 },
+ { category_col: 'B', sum_val: 2, size_metric: 25 },
+ { category_col: 'C', sum_val: 3, size_metric: 40 },
+ ],
+ colnames: ['category_col', 'sum_val', 'size_metric'],
+ coltypes: [
+ GenericDataType.String,
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ ],
+ label_map: {
+ category_col: ['category_col'],
+ sum_val: ['sum_val'],
+ size_metric: ['size_metric'],
+ },
+ },
+ ];
+
+ const baseChartPropsConfig = {
+ width: 800,
+ height: 600,
+ theme: supersetTheme,
+ };
+
+ const getScatterSeries = (props: ReturnType<typeof transformProps>) =>
+ (props.echartOptions.series as any[]).filter(s => s.type === 'scatter');
+
+ const singleMetricData = [
+ {
+ ...categoricalData[0],
+ data: categoricalData[0].data.map(row => ({
+ category_col: row.category_col,
+ sum_val: row.sum_val,
+ })),
+ colnames: ['category_col', 'sum_val'],
+ coltypes: [GenericDataType.String, GenericDataType.Numeric],
+ },
+ ];
+
+ test('horizontal orientation swaps the dimension axis onto the y-axis', () => {
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: singleMetricData,
+ formData: { ...baseFormData, orientation: 'horizontal' },
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const { xAxis, yAxis } = transformedProps.echartOptions as any;
+ expect(yAxis.type).toBe('category');
+ expect(xAxis.type).toBe('value');
+
+ const series = getScatterSeries(transformedProps);
+ expect(series).toHaveLength(1);
+ // data points are flipped to [metric, dimension]
+ expect(series[0].data[0]).toEqual([1, 'A']);
+ });
+
+ test('vertical orientation keeps the dimension axis on the x-axis', () => {
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: singleMetricData,
+ formData: baseFormData,
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const { xAxis, yAxis } = transformedProps.echartOptions as any;
+ expect(xAxis.type).toBe('category');
+ expect(yAxis.type).toBe('value');
+
+ const series = getScatterSeries(transformedProps);
+ expect(series[0].data[0]).toEqual(['A', 1]);
+ });
+
+ test('size metric series is not rendered or shown in the legend', () => {
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: categoricalData,
+ formData: { ...baseFormData, size: 'size_metric' },
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const series = getScatterSeries(transformedProps);
+ expect(series).toHaveLength(1);
+ expect(series[0].name).toBe('sum_val');
+ expect(transformedProps.legendData).toEqual(['sum_val']);
+ });
+
+ test('size metric scales marker areas between min and max dot size', () => {
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: categoricalData,
+ formData: {
+ ...baseFormData,
+ size: 'size_metric',
+ minMarkerSize: 5,
+ maxMarkerSize: 30,
+ },
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const [series] = getScatterSeries(transformedProps);
+ expect(typeof series.symbolSize).toBe('function');
+ // smallest size value -> min dot size, largest -> max dot size
+ expect(series.symbolSize(['A', 1])).toBe(5);
+ expect(series.symbolSize(['C', 3])).toBe(30);
+ // midpoint value -> midpoint *area*, not midpoint diameter
+ expect(series.symbolSize(['B', 2]) ** 2).toBeCloseTo(
+ (5 ** 2 + 30 ** 2) / 2,
+ );
+ });
+
+ test('size metric lookups follow the dimension key when grouped', () => {
+ const groupedData = [
+ {
+ data: [
+ {
+ category_col: 'A',
+ 'sum_val, g1': 1,
+ 'size_metric, g1': 10,
+ 'sum_val, g2': 2,
+ 'size_metric, g2': 40,
+ },
+ ],
+ colnames: [
+ 'category_col',
+ 'sum_val, g1',
+ 'size_metric, g1',
+ 'sum_val, g2',
+ 'size_metric, g2',
+ ],
+ coltypes: [
+ GenericDataType.String,
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ ],
+ label_map: {
+ category_col: ['category_col'],
+ 'sum_val, g1': ['sum_val', 'g1'],
+ 'size_metric, g1': ['size_metric', 'g1'],
+ 'sum_val, g2': ['sum_val', 'g2'],
+ 'size_metric, g2': ['size_metric', 'g2'],
+ },
+ },
+ ];
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: groupedData,
+ formData: {
+ ...baseFormData,
+ groupby: ['group_col'],
+ size: 'size_metric',
+ minMarkerSize: 5,
+ maxMarkerSize: 30,
+ },
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const series = getScatterSeries(transformedProps);
+ expect(series.map((s: any) => s.name).sort()).toEqual([
+ 'sum_val, g1',
+ 'sum_val, g2',
+ ]);
+ const g1 = series.find((s: any) => s.name === 'sum_val, g1');
+ const g2 = series.find((s: any) => s.name === 'sum_val, g2');
Review Comment:
**Suggestion:** Type the `find` callback parameter with the same series type used elsewhere rather than `any` to enforce safe property access. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
Both callback parameters are typed as `any` in TypeScript test code. That directly violates the rule requiring concrete or narrower types instead of `any`.
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=78950423cd8446fd8b890fa01a58066e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=78950423cd8446fd8b890fa01a58066e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/transformProps.test.ts
**Line:** 368:369
**Comment:**
*Custom Rule: Type the `find` callback parameter with the same series type used elsewhere rather than `any` to enforce safe property access.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=01d159edd6e7a94ec0a5c2854c7ccdf6cea56b5fc538aad2102493d1aa992393&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=01d159edd6e7a94ec0a5c2854c7ccdf6cea56b5fc538aad2102493d1aa992393&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/transformProps.test.ts:
##########
@@ -175,3 +175,283 @@ describe('Scatter Chart X-axis Number Formatting', () => {
},
);
});
+
+describe('Scatter Chart Orientation and Dot Size Metric', () => {
+ const baseFormData: EchartsTimeseriesFormData = {
+ ...DEFAULT_FORM_DATA,
+ colorScheme: 'supersetColors',
+ datasource: '1__table',
+ metrics: ['sum_val'],
+ x_axis: 'category_col',
+ groupby: [],
+ viz_type: 'echarts_timeseries_scatter',
+ seriesType: EchartsTimeseriesSeriesType.Scatter,
+ };
+
+ const categoricalData = [
+ {
+ data: [
+ { category_col: 'A', sum_val: 1, size_metric: 10 },
+ { category_col: 'B', sum_val: 2, size_metric: 25 },
+ { category_col: 'C', sum_val: 3, size_metric: 40 },
+ ],
+ colnames: ['category_col', 'sum_val', 'size_metric'],
+ coltypes: [
+ GenericDataType.String,
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ ],
+ label_map: {
+ category_col: ['category_col'],
+ sum_val: ['sum_val'],
+ size_metric: ['size_metric'],
+ },
+ },
+ ];
+
+ const baseChartPropsConfig = {
+ width: 800,
+ height: 600,
+ theme: supersetTheme,
+ };
+
+ const getScatterSeries = (props: ReturnType<typeof transformProps>) =>
+ (props.echartOptions.series as any[]).filter(s => s.type === 'scatter');
+
+ const singleMetricData = [
+ {
+ ...categoricalData[0],
+ data: categoricalData[0].data.map(row => ({
+ category_col: row.category_col,
+ sum_val: row.sum_val,
+ })),
+ colnames: ['category_col', 'sum_val'],
+ coltypes: [GenericDataType.String, GenericDataType.Numeric],
+ },
+ ];
+
+ test('horizontal orientation swaps the dimension axis onto the y-axis', () => {
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: singleMetricData,
+ formData: { ...baseFormData, orientation: 'horizontal' },
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const { xAxis, yAxis } = transformedProps.echartOptions as any;
+ expect(yAxis.type).toBe('category');
+ expect(xAxis.type).toBe('value');
+
+ const series = getScatterSeries(transformedProps);
+ expect(series).toHaveLength(1);
+ // data points are flipped to [metric, dimension]
+ expect(series[0].data[0]).toEqual([1, 'A']);
+ });
+
+ test('vertical orientation keeps the dimension axis on the x-axis', () => {
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: singleMetricData,
+ formData: baseFormData,
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const { xAxis, yAxis } = transformedProps.echartOptions as any;
+ expect(xAxis.type).toBe('category');
+ expect(yAxis.type).toBe('value');
+
+ const series = getScatterSeries(transformedProps);
+ expect(series[0].data[0]).toEqual(['A', 1]);
+ });
+
+ test('size metric series is not rendered or shown in the legend', () => {
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: categoricalData,
+ formData: { ...baseFormData, size: 'size_metric' },
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const series = getScatterSeries(transformedProps);
+ expect(series).toHaveLength(1);
+ expect(series[0].name).toBe('sum_val');
+ expect(transformedProps.legendData).toEqual(['sum_val']);
+ });
+
+ test('size metric scales marker areas between min and max dot size', () => {
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: categoricalData,
+ formData: {
+ ...baseFormData,
+ size: 'size_metric',
+ minMarkerSize: 5,
+ maxMarkerSize: 30,
+ },
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const [series] = getScatterSeries(transformedProps);
+ expect(typeof series.symbolSize).toBe('function');
+ // smallest size value -> min dot size, largest -> max dot size
+ expect(series.symbolSize(['A', 1])).toBe(5);
+ expect(series.symbolSize(['C', 3])).toBe(30);
+ // midpoint value -> midpoint *area*, not midpoint diameter
+ expect(series.symbolSize(['B', 2]) ** 2).toBeCloseTo(
+ (5 ** 2 + 30 ** 2) / 2,
+ );
+ });
+
+ test('size metric lookups follow the dimension key when grouped', () => {
+ const groupedData = [
+ {
+ data: [
+ {
+ category_col: 'A',
+ 'sum_val, g1': 1,
+ 'size_metric, g1': 10,
+ 'sum_val, g2': 2,
+ 'size_metric, g2': 40,
+ },
+ ],
+ colnames: [
+ 'category_col',
+ 'sum_val, g1',
+ 'size_metric, g1',
+ 'sum_val, g2',
+ 'size_metric, g2',
+ ],
+ coltypes: [
+ GenericDataType.String,
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ ],
+ label_map: {
+ category_col: ['category_col'],
+ 'sum_val, g1': ['sum_val', 'g1'],
+ 'size_metric, g1': ['size_metric', 'g1'],
+ 'sum_val, g2': ['sum_val', 'g2'],
+ 'size_metric, g2': ['size_metric', 'g2'],
+ },
+ },
+ ];
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: groupedData,
+ formData: {
+ ...baseFormData,
+ groupby: ['group_col'],
+ size: 'size_metric',
+ minMarkerSize: 5,
+ maxMarkerSize: 30,
+ },
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const series = getScatterSeries(transformedProps);
+ expect(series.map((s: any) => s.name).sort()).toEqual([
Review Comment:
**Suggestion:** Replace the callback parameter type `any` with a typed scatter series object so `.name` access is validated by TypeScript. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The callback parameter is explicitly typed as `any`, which falls under the custom rule banning `any` in TypeScript/TSX code.
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=86d471c2415e47db9c39ace481e8ba29&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=86d471c2415e47db9c39ace481e8ba29&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/transformProps.test.ts
**Line:** 364:364
**Comment:**
*Custom Rule: Replace the callback parameter type `any` with a typed scatter series object so `.name` access is validated by TypeScript.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=8ddf532e349c77cc1700f3f39bc45bd4fda1f6d6de29c8d1ee7d45ada0d0e45a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=8ddf532e349c77cc1700f3f39bc45bd4fda1f6d6de29c8d1ee7d45ada0d0e45a&reaction=dislike'>👎</a>
--
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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #40967:
URL: https://github.com/apache/superset/pull/40967#discussion_r3400935737
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/transformProps.test.ts:
##########
@@ -175,3 +175,283 @@ describe('Scatter Chart X-axis Number Formatting', () => {
},
);
});
+
+describe('Scatter Chart Orientation and Dot Size Metric', () => {
+ const baseFormData: EchartsTimeseriesFormData = {
+ ...DEFAULT_FORM_DATA,
+ colorScheme: 'supersetColors',
+ datasource: '1__table',
+ metrics: ['sum_val'],
+ x_axis: 'category_col',
+ groupby: [],
+ viz_type: 'echarts_timeseries_scatter',
+ seriesType: EchartsTimeseriesSeriesType.Scatter,
+ };
+
+ const categoricalData = [
+ {
+ data: [
+ { category_col: 'A', sum_val: 1, size_metric: 10 },
+ { category_col: 'B', sum_val: 2, size_metric: 25 },
+ { category_col: 'C', sum_val: 3, size_metric: 40 },
+ ],
+ colnames: ['category_col', 'sum_val', 'size_metric'],
+ coltypes: [
+ GenericDataType.String,
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ ],
+ label_map: {
+ category_col: ['category_col'],
+ sum_val: ['sum_val'],
+ size_metric: ['size_metric'],
+ },
+ },
+ ];
+
+ const baseChartPropsConfig = {
+ width: 800,
+ height: 600,
+ theme: supersetTheme,
+ };
+
+ const getScatterSeries = (props: ReturnType<typeof transformProps>) =>
+ (props.echartOptions.series as any[]).filter(s => s.type === 'scatter');
+
+ const singleMetricData = [
+ {
+ ...categoricalData[0],
+ data: categoricalData[0].data.map(row => ({
+ category_col: row.category_col,
+ sum_val: row.sum_val,
+ })),
+ colnames: ['category_col', 'sum_val'],
+ coltypes: [GenericDataType.String, GenericDataType.Numeric],
+ },
+ ];
+
+ test('horizontal orientation swaps the dimension axis onto the y-axis', () => {
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: singleMetricData,
+ formData: { ...baseFormData, orientation: 'horizontal' },
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const { xAxis, yAxis } = transformedProps.echartOptions as any;
+ expect(yAxis.type).toBe('category');
+ expect(xAxis.type).toBe('value');
+
+ const series = getScatterSeries(transformedProps);
+ expect(series).toHaveLength(1);
+ // data points are flipped to [metric, dimension]
+ expect(series[0].data[0]).toEqual([1, 'A']);
+ });
+
+ test('vertical orientation keeps the dimension axis on the x-axis', () => {
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: singleMetricData,
+ formData: baseFormData,
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const { xAxis, yAxis } = transformedProps.echartOptions as any;
Review Comment:
Same `TestAxis` type used here too.
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/transformProps.test.ts:
##########
@@ -175,3 +175,283 @@ describe('Scatter Chart X-axis Number Formatting', () => {
},
);
});
+
+describe('Scatter Chart Orientation and Dot Size Metric', () => {
+ const baseFormData: EchartsTimeseriesFormData = {
+ ...DEFAULT_FORM_DATA,
+ colorScheme: 'supersetColors',
+ datasource: '1__table',
+ metrics: ['sum_val'],
+ x_axis: 'category_col',
+ groupby: [],
+ viz_type: 'echarts_timeseries_scatter',
+ seriesType: EchartsTimeseriesSeriesType.Scatter,
+ };
+
+ const categoricalData = [
+ {
+ data: [
+ { category_col: 'A', sum_val: 1, size_metric: 10 },
+ { category_col: 'B', sum_val: 2, size_metric: 25 },
+ { category_col: 'C', sum_val: 3, size_metric: 40 },
+ ],
+ colnames: ['category_col', 'sum_val', 'size_metric'],
+ coltypes: [
+ GenericDataType.String,
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ ],
+ label_map: {
+ category_col: ['category_col'],
+ sum_val: ['sum_val'],
+ size_metric: ['size_metric'],
+ },
+ },
+ ];
+
+ const baseChartPropsConfig = {
+ width: 800,
+ height: 600,
+ theme: supersetTheme,
+ };
+
+ const getScatterSeries = (props: ReturnType<typeof transformProps>) =>
+ (props.echartOptions.series as any[]).filter(s => s.type === 'scatter');
+
+ const singleMetricData = [
+ {
+ ...categoricalData[0],
+ data: categoricalData[0].data.map(row => ({
+ category_col: row.category_col,
+ sum_val: row.sum_val,
+ })),
+ colnames: ['category_col', 'sum_val'],
+ coltypes: [GenericDataType.String, GenericDataType.Numeric],
+ },
+ ];
+
+ test('horizontal orientation swaps the dimension axis onto the y-axis', () => {
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: singleMetricData,
+ formData: { ...baseFormData, orientation: 'horizontal' },
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const { xAxis, yAxis } = transformedProps.echartOptions as any;
+ expect(yAxis.type).toBe('category');
+ expect(xAxis.type).toBe('value');
+
+ const series = getScatterSeries(transformedProps);
+ expect(series).toHaveLength(1);
+ // data points are flipped to [metric, dimension]
+ expect(series[0].data[0]).toEqual([1, 'A']);
+ });
+
+ test('vertical orientation keeps the dimension axis on the x-axis', () => {
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: singleMetricData,
+ formData: baseFormData,
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const { xAxis, yAxis } = transformedProps.echartOptions as any;
+ expect(xAxis.type).toBe('category');
+ expect(yAxis.type).toBe('value');
+
+ const series = getScatterSeries(transformedProps);
+ expect(series[0].data[0]).toEqual(['A', 1]);
+ });
+
+ test('size metric series is not rendered or shown in the legend', () => {
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: categoricalData,
+ formData: { ...baseFormData, size: 'size_metric' },
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const series = getScatterSeries(transformedProps);
+ expect(series).toHaveLength(1);
+ expect(series[0].name).toBe('sum_val');
+ expect(transformedProps.legendData).toEqual(['sum_val']);
+ });
+
+ test('size metric scales marker areas between min and max dot size', () => {
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: categoricalData,
+ formData: {
+ ...baseFormData,
+ size: 'size_metric',
+ minMarkerSize: 5,
+ maxMarkerSize: 30,
+ },
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const [series] = getScatterSeries(transformedProps);
+ expect(typeof series.symbolSize).toBe('function');
+ // smallest size value -> min dot size, largest -> max dot size
+ expect(series.symbolSize(['A', 1])).toBe(5);
+ expect(series.symbolSize(['C', 3])).toBe(30);
+ // midpoint value -> midpoint *area*, not midpoint diameter
+ expect(series.symbolSize(['B', 2]) ** 2).toBeCloseTo(
+ (5 ** 2 + 30 ** 2) / 2,
+ );
+ });
+
+ test('size metric lookups follow the dimension key when grouped', () => {
+ const groupedData = [
+ {
+ data: [
+ {
+ category_col: 'A',
+ 'sum_val, g1': 1,
+ 'size_metric, g1': 10,
+ 'sum_val, g2': 2,
+ 'size_metric, g2': 40,
+ },
+ ],
+ colnames: [
+ 'category_col',
+ 'sum_val, g1',
+ 'size_metric, g1',
+ 'sum_val, g2',
+ 'size_metric, g2',
+ ],
+ coltypes: [
+ GenericDataType.String,
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ ],
+ label_map: {
+ category_col: ['category_col'],
+ 'sum_val, g1': ['sum_val', 'g1'],
+ 'size_metric, g1': ['size_metric', 'g1'],
+ 'sum_val, g2': ['sum_val', 'g2'],
+ 'size_metric, g2': ['size_metric', 'g2'],
+ },
+ },
+ ];
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: groupedData,
+ formData: {
+ ...baseFormData,
+ groupby: ['group_col'],
+ size: 'size_metric',
+ minMarkerSize: 5,
+ maxMarkerSize: 30,
+ },
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const series = getScatterSeries(transformedProps);
+ expect(series.map((s: any) => s.name).sort()).toEqual([
Review Comment:
This one picks up the series type from the typed array now, no annotation needed.
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/transformProps.test.ts:
##########
@@ -175,3 +175,283 @@ describe('Scatter Chart X-axis Number Formatting', () => {
},
);
});
+
+describe('Scatter Chart Orientation and Dot Size Metric', () => {
+ const baseFormData: EchartsTimeseriesFormData = {
+ ...DEFAULT_FORM_DATA,
+ colorScheme: 'supersetColors',
+ datasource: '1__table',
+ metrics: ['sum_val'],
+ x_axis: 'category_col',
+ groupby: [],
+ viz_type: 'echarts_timeseries_scatter',
+ seriesType: EchartsTimeseriesSeriesType.Scatter,
+ };
+
+ const categoricalData = [
+ {
+ data: [
+ { category_col: 'A', sum_val: 1, size_metric: 10 },
+ { category_col: 'B', sum_val: 2, size_metric: 25 },
+ { category_col: 'C', sum_val: 3, size_metric: 40 },
+ ],
+ colnames: ['category_col', 'sum_val', 'size_metric'],
+ coltypes: [
+ GenericDataType.String,
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ ],
+ label_map: {
+ category_col: ['category_col'],
+ sum_val: ['sum_val'],
+ size_metric: ['size_metric'],
+ },
+ },
+ ];
+
+ const baseChartPropsConfig = {
+ width: 800,
+ height: 600,
+ theme: supersetTheme,
+ };
+
+ const getScatterSeries = (props: ReturnType<typeof transformProps>) =>
+ (props.echartOptions.series as any[]).filter(s => s.type === 'scatter');
+
+ const singleMetricData = [
+ {
+ ...categoricalData[0],
+ data: categoricalData[0].data.map(row => ({
+ category_col: row.category_col,
+ sum_val: row.sum_val,
+ })),
+ colnames: ['category_col', 'sum_val'],
+ coltypes: [GenericDataType.String, GenericDataType.Numeric],
+ },
+ ];
+
+ test('horizontal orientation swaps the dimension axis onto the y-axis', () => {
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: singleMetricData,
+ formData: { ...baseFormData, orientation: 'horizontal' },
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const { xAxis, yAxis } = transformedProps.echartOptions as any;
+ expect(yAxis.type).toBe('category');
+ expect(xAxis.type).toBe('value');
+
+ const series = getScatterSeries(transformedProps);
+ expect(series).toHaveLength(1);
+ // data points are flipped to [metric, dimension]
+ expect(series[0].data[0]).toEqual([1, 'A']);
+ });
+
+ test('vertical orientation keeps the dimension axis on the x-axis', () => {
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: singleMetricData,
+ formData: baseFormData,
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const { xAxis, yAxis } = transformedProps.echartOptions as any;
+ expect(xAxis.type).toBe('category');
+ expect(yAxis.type).toBe('value');
+
+ const series = getScatterSeries(transformedProps);
+ expect(series[0].data[0]).toEqual(['A', 1]);
+ });
+
+ test('size metric series is not rendered or shown in the legend', () => {
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: categoricalData,
+ formData: { ...baseFormData, size: 'size_metric' },
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const series = getScatterSeries(transformedProps);
+ expect(series).toHaveLength(1);
+ expect(series[0].name).toBe('sum_val');
+ expect(transformedProps.legendData).toEqual(['sum_val']);
+ });
+
+ test('size metric scales marker areas between min and max dot size', () => {
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: categoricalData,
+ formData: {
+ ...baseFormData,
+ size: 'size_metric',
+ minMarkerSize: 5,
+ maxMarkerSize: 30,
+ },
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const [series] = getScatterSeries(transformedProps);
+ expect(typeof series.symbolSize).toBe('function');
+ // smallest size value -> min dot size, largest -> max dot size
+ expect(series.symbolSize(['A', 1])).toBe(5);
+ expect(series.symbolSize(['C', 3])).toBe(30);
+ // midpoint value -> midpoint *area*, not midpoint diameter
+ expect(series.symbolSize(['B', 2]) ** 2).toBeCloseTo(
+ (5 ** 2 + 30 ** 2) / 2,
+ );
+ });
+
+ test('size metric lookups follow the dimension key when grouped', () => {
+ const groupedData = [
+ {
+ data: [
+ {
+ category_col: 'A',
+ 'sum_val, g1': 1,
+ 'size_metric, g1': 10,
+ 'sum_val, g2': 2,
+ 'size_metric, g2': 40,
+ },
+ ],
+ colnames: [
+ 'category_col',
+ 'sum_val, g1',
+ 'size_metric, g1',
+ 'sum_val, g2',
+ 'size_metric, g2',
+ ],
+ coltypes: [
+ GenericDataType.String,
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ ],
+ label_map: {
+ category_col: ['category_col'],
+ 'sum_val, g1': ['sum_val', 'g1'],
+ 'size_metric, g1': ['size_metric', 'g1'],
+ 'sum_val, g2': ['sum_val', 'g2'],
+ 'size_metric, g2': ['size_metric', 'g2'],
+ },
+ },
+ ];
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: groupedData,
+ formData: {
+ ...baseFormData,
+ groupby: ['group_col'],
+ size: 'size_metric',
+ minMarkerSize: 5,
+ maxMarkerSize: 30,
+ },
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const series = getScatterSeries(transformedProps);
+ expect(series.map((s: any) => s.name).sort()).toEqual([
+ 'sum_val, g1',
+ 'sum_val, g2',
+ ]);
+ const g1 = series.find((s: any) => s.name === 'sum_val, g1');
+ const g2 = series.find((s: any) => s.name === 'sum_val, g2');
Review Comment:
Same here... the callbacks infer from `TestScatterSeries[]` now.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -337,6 +342,89 @@ export default function transformProps(
xAxisType,
},
);
+
+ // Dot size by metric (scatter): the size metric's series are excluded from
+ // rendering and instead provide per-point values that scale each marker's
+ // area between minMarkerSize and maxMarkerSize.
+ const sizeMetricLabel =
+ seriesType === EchartsTimeseriesSeriesType.Scatter && size
+ ? getMetricLabel(size)
+ : undefined;
+ const sizeSeriesLabel = isDefined(sizeMetricLabel)
+ ? (verboseMap[sizeMetricLabel!] ?? sizeMetricLabel)
+ : undefined;
+ const valueMetricLabels = ensureIsArray(metrics)
+ .map(getMetricLabel)
+ .map(label => verboseMap[label] ?? label);
+ // When the size metric is also a value metric, the query dedupes them into a
+ // single column, so each point's own value doubles as its size value.
+ const sizeIsValueMetric = isDefined(sizeSeriesLabel)
+ ? valueMetricLabels.includes(sizeSeriesLabel!)
+ : false;
+ const isSizeSeries = (name: string) =>
+ isDefined(sizeSeriesLabel) &&
+ !sizeIsValueMetric &&
+ (name === sizeSeriesLabel || name.startsWith(`${sizeSeriesLabel}, `));
+ const rawSeries = sizeSeriesLabel
+ ? allRawSeries.filter(entry => !isSizeSeries(String(entry.name ?? '')))
+ : allRawSeries;
+ // Maps each value series' dimension key to a lookup from primary-axis value
+ // to size value.
+ let sizeLookups: Map<string, Map<DataRecordValue, number>> | undefined;
+ let sizeExtent: [number, number] | undefined;
+ if (sizeSeriesLabel) {
+ let sizeMin = Infinity;
+ let sizeMax = -Infinity;
+ if (sizeIsValueMetric) {
+ rawSeries.forEach(entry => {
+ (entry.data as DataRecordValue[][]).forEach(datum => {
+ const sizeValue = isHorizontal ? datum[0] : datum[1];
+ if (typeof sizeValue === 'number' && Number.isFinite(sizeValue)) {
+ sizeMin = Math.min(sizeMin, sizeValue);
+ sizeMax = Math.max(sizeMax, sizeValue);
+ }
+ });
+ });
+ } else {
+ sizeLookups = new Map();
+ allRawSeries
+ .filter(entry => isSizeSeries(String(entry.name ?? '')))
+ .forEach(entry => {
+ const name = String(entry.name ?? '');
+ const dimsKey =
+ name === sizeSeriesLabel
+ ? ''
+ : name.slice(sizeSeriesLabel.length + 2);
+ const lookup = new Map<DataRecordValue, number>();
+ (entry.data as DataRecordValue[][]).forEach(datum => {
+ const axisValue = isHorizontal ? datum[1] : datum[0];
+ const sizeValue = isHorizontal ? datum[0] : datum[1];
+ if (typeof sizeValue === 'number' && Number.isFinite(sizeValue)) {
+ lookup.set(axisValue, sizeValue);
+ sizeMin = Math.min(sizeMin, sizeValue);
+ sizeMax = Math.max(sizeMax, sizeValue);
+ }
+ });
+ sizeLookups!.set(dimsKey, lookup);
+ });
+ }
+ if (sizeMin <= sizeMax) {
+ sizeExtent = [sizeMin, sizeMax];
+ }
+ }
+ // Strips the metric label off a series name, leaving the dimension key used
+ // to match a value series with its size series.
+ const getSeriesDimsKey = (name: string): string => {
+ const matchedLabel = valueMetricLabels.find(
+ label => name === label || name.startsWith(`${label}, `),
+ );
+ if (matchedLabel === undefined) {
+ return name;
+ }
+ return name === matchedLabel ? '' : name.slice(matchedLabel.length + 2);
+ };
+ const markerSizeRange: [number, number] = [minMarkerSize, maxMarkerSize];
Review Comment:
Good catch. The range gets normalized in `transformProps` now, and there is a test covering the inverted case.
--
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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #40967:
URL: https://github.com/apache/superset/pull/40967#discussion_r3394642080
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -337,6 +342,89 @@ export default function transformProps(
xAxisType,
},
);
+
+ // Dot size by metric (scatter): the size metric's series are excluded from
+ // rendering and instead provide per-point values that scale each marker's
+ // area between minMarkerSize and maxMarkerSize.
+ const sizeMetricLabel =
+ seriesType === EchartsTimeseriesSeriesType.Scatter && size
+ ? getMetricLabel(size)
+ : undefined;
+ const sizeSeriesLabel = isDefined(sizeMetricLabel)
+ ? (verboseMap[sizeMetricLabel!] ?? sizeMetricLabel)
+ : undefined;
+ const valueMetricLabels = ensureIsArray(metrics)
+ .map(getMetricLabel)
+ .map(label => verboseMap[label] ?? label);
+ // When the size metric is also a value metric, the query dedupes them into a
+ // single column, so each point's own value doubles as its size value.
+ const sizeIsValueMetric = isDefined(sizeSeriesLabel)
+ ? valueMetricLabels.includes(sizeSeriesLabel!)
+ : false;
+ const isSizeSeries = (name: string) =>
+ isDefined(sizeSeriesLabel) &&
+ !sizeIsValueMetric &&
+ (name === sizeSeriesLabel || name.startsWith(`${sizeSeriesLabel}, `));
+ const rawSeries = sizeSeriesLabel
+ ? allRawSeries.filter(entry => !isSizeSeries(String(entry.name ?? '')))
+ : allRawSeries;
Review Comment:
Good catch on the edge case. Size-driven dots plus time comparison is a combination I didn't intend to support in this PR — the offset-named series would need their own handling. I'd rather scope that out than half-wire it here; can revisit if anyone actually wants both at once.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -337,6 +342,89 @@ export default function transformProps(
xAxisType,
},
);
+
+ // Dot size by metric (scatter): the size metric's series are excluded from
+ // rendering and instead provide per-point values that scale each marker's
+ // area between minMarkerSize and maxMarkerSize.
+ const sizeMetricLabel =
+ seriesType === EchartsTimeseriesSeriesType.Scatter && size
+ ? getMetricLabel(size)
+ : undefined;
+ const sizeSeriesLabel = isDefined(sizeMetricLabel)
+ ? (verboseMap[sizeMetricLabel!] ?? sizeMetricLabel)
+ : undefined;
+ const valueMetricLabels = ensureIsArray(metrics)
+ .map(getMetricLabel)
+ .map(label => verboseMap[label] ?? label);
+ // When the size metric is also a value metric, the query dedupes them into a
+ // single column, so each point's own value doubles as its size value.
+ const sizeIsValueMetric = isDefined(sizeSeriesLabel)
+ ? valueMetricLabels.includes(sizeSeriesLabel!)
+ : false;
+ const isSizeSeries = (name: string) =>
+ isDefined(sizeSeriesLabel) &&
+ !sizeIsValueMetric &&
+ (name === sizeSeriesLabel || name.startsWith(`${sizeSeriesLabel}, `));
Review Comment:
Same root point as the architect thread above — the `__<offset>` series only show up when you mix a size metric with time comparison, which this PR doesn't aim to support. Leaving offset handling out deliberately rather than partially.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -337,6 +342,89 @@ export default function transformProps(
xAxisType,
},
);
+
+ // Dot size by metric (scatter): the size metric's series are excluded from
+ // rendering and instead provide per-point values that scale each marker's
+ // area between minMarkerSize and maxMarkerSize.
+ const sizeMetricLabel =
+ seriesType === EchartsTimeseriesSeriesType.Scatter && size
+ ? getMetricLabel(size)
+ : undefined;
+ const sizeSeriesLabel = isDefined(sizeMetricLabel)
+ ? (verboseMap[sizeMetricLabel!] ?? sizeMetricLabel)
+ : undefined;
+ const valueMetricLabels = ensureIsArray(metrics)
+ .map(getMetricLabel)
+ .map(label => verboseMap[label] ?? label);
+ // When the size metric is also a value metric, the query dedupes them into a
+ // single column, so each point's own value doubles as its size value.
+ const sizeIsValueMetric = isDefined(sizeSeriesLabel)
+ ? valueMetricLabels.includes(sizeSeriesLabel!)
+ : false;
+ const isSizeSeries = (name: string) =>
+ isDefined(sizeSeriesLabel) &&
+ !sizeIsValueMetric &&
+ (name === sizeSeriesLabel || name.startsWith(`${sizeSeriesLabel}, `));
+ const rawSeries = sizeSeriesLabel
+ ? allRawSeries.filter(entry => !isSizeSeries(String(entry.name ?? '')))
+ : allRawSeries;
+ // Maps each value series' dimension key to a lookup from primary-axis value
+ // to size value.
+ let sizeLookups: Map<string, Map<DataRecordValue, number>> | undefined;
+ let sizeExtent: [number, number] | undefined;
+ if (sizeSeriesLabel) {
+ let sizeMin = Infinity;
+ let sizeMax = -Infinity;
+ if (sizeIsValueMetric) {
+ rawSeries.forEach(entry => {
+ (entry.data as DataRecordValue[][]).forEach(datum => {
+ const sizeValue = isHorizontal ? datum[0] : datum[1];
+ if (typeof sizeValue === 'number' && Number.isFinite(sizeValue)) {
+ sizeMin = Math.min(sizeMin, sizeValue);
+ sizeMax = Math.max(sizeMax, sizeValue);
+ }
+ });
+ });
+ } else {
+ sizeLookups = new Map();
+ allRawSeries
+ .filter(entry => isSizeSeries(String(entry.name ?? '')))
+ .forEach(entry => {
+ const name = String(entry.name ?? '');
+ const dimsKey =
+ name === sizeSeriesLabel
+ ? ''
+ : name.slice(sizeSeriesLabel.length + 2);
+ const lookup = new Map<DataRecordValue, number>();
+ (entry.data as DataRecordValue[][]).forEach(datum => {
+ const axisValue = isHorizontal ? datum[1] : datum[0];
+ const sizeValue = isHorizontal ? datum[0] : datum[1];
+ if (typeof sizeValue === 'number' && Number.isFinite(sizeValue)) {
+ lookup.set(axisValue, sizeValue);
+ sizeMin = Math.min(sizeMin, sizeValue);
+ sizeMax = Math.max(sizeMax, sizeValue);
+ }
+ });
+ sizeLookups!.set(dimsKey, lookup);
+ });
+ }
+ if (sizeMin <= sizeMax) {
+ sizeExtent = [sizeMin, sizeMax];
+ }
+ }
+ // Strips the metric label off a series name, leaving the dimension key used
+ // to match a value series with its size series.
+ const getSeriesDimsKey = (name: string): string => {
+ const matchedLabel = valueMetricLabels.find(
+ label => name === label || name.startsWith(`${label}, `),
+ );
+ if (matchedLabel === undefined) {
+ return name;
+ }
+ return name === matchedLabel ? '' : name.slice(matchedLabel.length + 2);
+ };
Review Comment:
Same time-comparison combination as the sibling threads — the dims-key extraction only diverges for `__<offset>` series, which need the size metric and time comparison both on. Out of scope here on purpose.
--
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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #40967:
URL: https://github.com/apache/superset/pull/40967#discussion_r3394643797
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx:
##########
@@ -99,115 +428,56 @@ const config: ControlPanelConfig = {
'Size of marker. Also applies to forecast observations.',
),
visibility: ({ controls }: ControlPanelsContainerProps) =>
- Boolean(controls?.markerEnabled?.value),
+ Boolean(controls?.markerEnabled?.value) &&
+ !controls?.size?.value,
Review Comment:
That's intentional — once a size metric is driving the dots, the fixed marker slider doesn't apply, so hiding it keeps the panel honest. Points with no size value fall to the min, which is the sensible floor here rather than a separate tunable.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx:
##########
@@ -99,115 +428,56 @@ const config: ControlPanelConfig = {
'Size of marker. Also applies to forecast observations.',
),
visibility: ({ controls }: ControlPanelsContainerProps) =>
- Boolean(controls?.markerEnabled?.value),
+ Boolean(controls?.markerEnabled?.value) &&
+ !controls?.size?.value,
},
},
],
- ['zoomable'],
- [minorTicks],
- ...legendSection,
- [<ControlSubSectionHeader>{t('X Axis')}</ControlSubSectionHeader>],
-
[
{
- name: 'x_axis_time_format',
- config: {
- ...sharedControls.x_axis_time_format,
- default: 'smart_date',
- description: `${D3_TIME_FORMAT_DOCS}. ${TIME_SERIES_DESCRIPTION_TEXT}`,
- visibility: ({ controls }: ControlPanelsContainerProps) =>
- checkColumnType(
- getColumnLabel(controls?.x_axis?.value as QueryFormColumn),
- controls?.datasource?.datasource,
- [GenericDataType.Temporal],
- ),
- disableStash: true,
- resetOnHide: false,
- },
- },
- {
- name: 'x_axis_number_format',
+ name: 'minMarkerSize',
config: {
- ...sharedControls.x_axis_number_format,
- default: '~g',
- mapStateToProps: undefined,
- visibility: ({ controls }: ControlPanelsContainerProps) =>
- checkColumnType(
- getColumnLabel(controls?.x_axis?.value as QueryFormColumn),
- controls?.datasource?.datasource,
- [GenericDataType.Numeric],
- ),
- },
- },
- ],
- [xAxisLabelRotation],
- [xAxisLabelInterval],
- [forceMaxInterval],
- // eslint-disable-next-line react/jsx-key
- ...richTooltipSection,
- // eslint-disable-next-line react/jsx-key
- [<ControlSubSectionHeader>{t('Y Axis')}</ControlSubSectionHeader>],
- ['y_axis_format'],
- ['currency_format'],
- [
- {
- name: 'logAxis',
- config: {
- type: 'CheckboxControl',
- label: t('Logarithmic y-axis'),
- renderTrigger: true,
- default: logAxis,
- description: t('Logarithmic y-axis'),
- },
- },
- ],
- [
- {
- name: 'minorSplitLine',
- config: {
- type: 'CheckboxControl',
- label: t('Minor Split Line'),
- renderTrigger: true,
- default: minorSplitLine,
- description: t('Draw split lines for minor y-axis ticks'),
- },
- },
- ],
- [truncateXAxis],
- [xAxisBounds],
- [
- {
- name: 'truncateYAxis',
- config: {
- type: 'CheckboxControl',
- label: t('Truncate Y Axis'),
- default: truncateYAxis,
+ type: 'SliderControl',
+ label: t('Minimum dot size'),
renderTrigger: true,
+ min: 1,
+ max: 100,
+ default: minMarkerSize,
description: t(
- 'Truncate Y Axis. Can be overridden by specifying a min or max bound.',
+ 'Size of the dot representing the smallest value of the dot size metric.',
),
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ Boolean(controls?.size?.value),
},
},
- ],
- [
{
- name: 'y_axis_bounds',
+ name: 'maxMarkerSize',
config: {
- type: 'BoundsControl',
- label: t('Y Axis Bounds'),
+ type: 'SliderControl',
+ label: t('Maximum dot size'),
renderTrigger: true,
- default: yAxisBounds,
+ min: 1,
+ max: 100,
+ default: maxMarkerSize,
description: t(
- 'Bounds for the Y-axis. When left empty, the bounds are ' +
- 'dynamically defined based on the min/max of the data. Note that ' +
- "this feature will only expand the axis range. It won't " +
- "narrow the data's extent.",
+ 'Size of the dot representing the largest value of the dot size metric.',
),
visibility: ({ controls }: ControlPanelsContainerProps) =>
- Boolean(controls?.truncateYAxis?.value),
+ Boolean(controls?.size?.value),
Review Comment:
Fair point, though both sliders clamp to 1–100 and inverting them just flips the scale rather than breaking anything. I'd sooner leave it than wire cross-field validation into the slider controls for an unusual input.
--
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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40967:
URL: https://github.com/apache/superset/pull/40967#issuecomment-4784918941
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40967:
URL: https://github.com/apache/superset/pull/40967#issuecomment-4678362656
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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #40967:
URL: https://github.com/apache/superset/pull/40967#issuecomment-5036236240
@kgabryje good catch on the time-comparison leak. The offset series keep the raw metric label with the `__<offset>` suffix while the classification only knew the verbose base label, so the size metric's offset copies slipped through. Fixed by matching both forms and keying the size lookups by offset, so each comparison series is sized from its own offset's values. The new regression test reproduces the leak on the old code and passes now.
On the advisory notes, I'd rather tackle the Bar/Scatter control-factory dedup as a follow-up so this one stays reviewable, and I'll fold the test-helper casting cleanup into that pass. CI's running 🤞
--
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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40967:
URL: https://github.com/apache/superset/pull/40967#discussion_r3394243404
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -337,6 +342,89 @@ export default function transformProps(
xAxisType,
},
);
+
+ // Dot size by metric (scatter): the size metric's series are excluded from
+ // rendering and instead provide per-point values that scale each marker's
+ // area between minMarkerSize and maxMarkerSize.
+ const sizeMetricLabel =
+ seriesType === EchartsTimeseriesSeriesType.Scatter && size
+ ? getMetricLabel(size)
+ : undefined;
+ const sizeSeriesLabel = isDefined(sizeMetricLabel)
+ ? (verboseMap[sizeMetricLabel!] ?? sizeMetricLabel)
+ : undefined;
+ const valueMetricLabels = ensureIsArray(metrics)
+ .map(getMetricLabel)
+ .map(label => verboseMap[label] ?? label);
+ // When the size metric is also a value metric, the query dedupes them into a
+ // single column, so each point's own value doubles as its size value.
+ const sizeIsValueMetric = isDefined(sizeSeriesLabel)
+ ? valueMetricLabels.includes(sizeSeriesLabel!)
+ : false;
+ const isSizeSeries = (name: string) =>
+ isDefined(sizeSeriesLabel) &&
+ !sizeIsValueMetric &&
+ (name === sizeSeriesLabel || name.startsWith(`${sizeSeriesLabel}, `));
+ const rawSeries = sizeSeriesLabel
+ ? allRawSeries.filter(entry => !isSizeSeries(String(entry.name ?? '')))
+ : allRawSeries;
+ // Maps each value series' dimension key to a lookup from primary-axis value
+ // to size value.
+ let sizeLookups: Map<string, Map<DataRecordValue, number>> | undefined;
+ let sizeExtent: [number, number] | undefined;
+ if (sizeSeriesLabel) {
+ let sizeMin = Infinity;
+ let sizeMax = -Infinity;
+ if (sizeIsValueMetric) {
+ rawSeries.forEach(entry => {
+ (entry.data as DataRecordValue[][]).forEach(datum => {
+ const sizeValue = isHorizontal ? datum[0] : datum[1];
+ if (typeof sizeValue === 'number' && Number.isFinite(sizeValue)) {
+ sizeMin = Math.min(sizeMin, sizeValue);
+ sizeMax = Math.max(sizeMax, sizeValue);
+ }
+ });
+ });
+ } else {
+ sizeLookups = new Map();
+ allRawSeries
+ .filter(entry => isSizeSeries(String(entry.name ?? '')))
+ .forEach(entry => {
+ const name = String(entry.name ?? '');
+ const dimsKey =
+ name === sizeSeriesLabel
+ ? ''
+ : name.slice(sizeSeriesLabel.length + 2);
+ const lookup = new Map<DataRecordValue, number>();
+ (entry.data as DataRecordValue[][]).forEach(datum => {
+ const axisValue = isHorizontal ? datum[1] : datum[0];
+ const sizeValue = isHorizontal ? datum[0] : datum[1];
+ if (typeof sizeValue === 'number' && Number.isFinite(sizeValue)) {
+ lookup.set(axisValue, sizeValue);
+ sizeMin = Math.min(sizeMin, sizeValue);
+ sizeMax = Math.max(sizeMax, sizeValue);
+ }
+ });
+ sizeLookups!.set(dimsKey, lookup);
+ });
+ }
+ if (sizeMin <= sizeMax) {
+ sizeExtent = [sizeMin, sizeMax];
+ }
+ }
+ // Strips the metric label off a series name, leaving the dimension key used
+ // to match a value series with its size series.
+ const getSeriesDimsKey = (name: string): string => {
+ const matchedLabel = valueMetricLabels.find(
+ label => name === label || name.startsWith(`${label}, `),
+ );
+ if (matchedLabel === undefined) {
+ return name;
+ }
+ return name === matchedLabel ? '' : name.slice(matchedLabel.length + 2);
+ };
Review Comment:
**Suggestion:** Dimension-key extraction for matching value series to size lookups only handles exact metric names and `<metric>, ...` names, but misses derived names like `<metric>__<offset>...`. In time-comparison charts this prevents matching value points to their size values, so symbol sizing silently falls back to fixed marker size for derived series. Normalize/remap derived metric names (including `__<offset>` forms) before extracting the dimension key. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Time-comparison scatter dots ignore size metric on shifted series.
- ⚠️ Dot sizes inconsistent between base and time-shifted series.
- ⚠️ Misleads users interpreting size as a comparable measure.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Use the same ECharts Timeseries scatter configuration as in suggestion 1: `seriesType =
EchartsTimeseriesSeriesType.Scatter`, a distinct `size` metric, and at least one value
metric, all defined via `EchartsTimeseriesFormData` (`src/Timeseries/types.ts:54-82`), and
enable time comparison with one or more offsets so the backend emits derived series such
as `"metric__1 day ago"` as documented in the comment at `transformProps.ts:485-488`.
2. When `transformProps` executes, it computes `sizeSeriesLabel`, `valueMetricLabels`, and
`sizeLookups` (the latter built from size-series in the block at
`transformProps.ts:116-151`) with dimension keys extracted for size-series as `dimsKey =
name === sizeSeriesLabel ? '' : name.slice(sizeSeriesLabel.length + 2)`
(`transformProps.ts:134-138`), so size lookups are keyed only by the dimension portion
(e.g. `"US"` or `"US, segment"`), not by the full metric+offset string.
3. Later, in the `rawSeries.forEach(entry => { ... })` loop (`transformProps.ts:223-235`),
the code tries to retrieve the matching size lookup for each value series using `const
sizeLookup = sizeLookups?.get(getSeriesDimsKey(entryName));` inside the symbol-size block
(`transformProps.ts:567-569`). The helper `getSeriesDimsKey` at
`transformProps.ts:417-425` only strips a value metric label when `name === label` or
`name.startsWith(\`${label}, \`)`; for time-comparison series named like `"revenue__1 day
ago, US"` or `"revenue__1 day ago"` (per the comment at `transformProps.ts:485-488`), no
`valueMetricLabels` element matches, so `matchedLabel` is `undefined` and
`getSeriesDimsKey` returns the entire derived name (e.g. `"revenue__1 day ago, US"`).
4. Because `sizeLookups` were keyed by dimension-only strings (from size-series names)
while `getSeriesDimsKey` returns full `"metric__offset, dims"` names for derived
value-series, `sizeLookups.get(getSeriesDimsKey(entryName))` yields `undefined` for those
derived series when `sizeIsValueMetric` is `false`. The condition `if (sizeIsValueMetric
|| sizeLookup)` at `transformProps.ts:569-575` then fails, leaving `symbolSizeFn`
undefined for derived series; `transformSeries` ultimately uses `symbolSize: symbolSizeFn
?? markerSize` (`transformers.ts:120`), so time-shifted series fall back to fixed
`markerSize` and ignore the configured dot-size metric while base (non-derived) series can
still be sized correctly.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=60f2aa530dea4d8c900f895b55dd4432&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=60f2aa530dea4d8c900f895b55dd4432&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
**Line:** 417:425
**Comment:**
*Logic Error: Dimension-key extraction for matching value series to size lookups only handles exact metric names and `<metric>, ...` names, but misses derived names like `<metric>__<offset>...`. In time-comparison charts this prevents matching value points to their size values, so symbol sizing silently falls back to fixed marker size for derived series. Normalize/remap derived metric names (including `__<offset>` forms) before extracting the dimension key.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=816041b9c5616a646fee8d7d67d0a10c454aaef912e5bd9a185d80ef21e2d645&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=816041b9c5616a646fee8d7d67d0a10c454aaef912e5bd9a185d80ef21e2d645&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -337,6 +342,89 @@ export default function transformProps(
xAxisType,
},
);
+
+ // Dot size by metric (scatter): the size metric's series are excluded from
+ // rendering and instead provide per-point values that scale each marker's
+ // area between minMarkerSize and maxMarkerSize.
+ const sizeMetricLabel =
+ seriesType === EchartsTimeseriesSeriesType.Scatter && size
+ ? getMetricLabel(size)
+ : undefined;
+ const sizeSeriesLabel = isDefined(sizeMetricLabel)
+ ? (verboseMap[sizeMetricLabel!] ?? sizeMetricLabel)
+ : undefined;
+ const valueMetricLabels = ensureIsArray(metrics)
+ .map(getMetricLabel)
+ .map(label => verboseMap[label] ?? label);
+ // When the size metric is also a value metric, the query dedupes them into a
+ // single column, so each point's own value doubles as its size value.
+ const sizeIsValueMetric = isDefined(sizeSeriesLabel)
+ ? valueMetricLabels.includes(sizeSeriesLabel!)
+ : false;
+ const isSizeSeries = (name: string) =>
+ isDefined(sizeSeriesLabel) &&
+ !sizeIsValueMetric &&
+ (name === sizeSeriesLabel || name.startsWith(`${sizeSeriesLabel}, `));
Review Comment:
**Suggestion:** The size-series detection only matches exact metric labels or the `<metric>, <group>` prefix, but time-comparison series are also emitted as patterns like `<metric>__<offset>` (and related offset forms). Those derived size series will not be filtered out, so the size metric can incorrectly render as visible chart/legend series instead of being used only for dot sizing. Extend the matcher to recognize time-offset naming patterns before filtering. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Time-comparison scatter charts show extra unintended size-series.
- ⚠️ Legends include confusing size-metric entries for each offset.
- ⚠️ Dot-size feature behaves inconsistently for shifted metrics.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure an ECharts Timeseries chart (the plugin uses `TimeseriesTransformProps`
exported from `superset-frontend/plugins/plugin-chart-echarts/src/index.ts:55`) with
`seriesType` set to `EchartsTimeseriesSeriesType.Scatter` and a distinct `size` metric set
in the form data (`EchartsTimeseriesFormData` fields `seriesType` and `size` are defined
in `src/Timeseries/types.ts:44-79`).
2. Enable time comparison on the same chart by setting `timeCompare` / `time_compare` in
the form data so that the backend returns time-shifted series; `transformProps` receives
this in `timeCompare` and `timeShiftColor` (see destructuring in
`transformProps.ts:220-255`), and the comment at `transformProps.ts:485-488` describes the
resulting time-comparison series naming patterns like `"metric__1 day ago"` and `"1 day
ago, groupby"`.
3. When `transformProps` runs, it builds `[allRawSeries]` via `extractSeries`
(`transformProps.ts:70-85`), then computes `sizeSeriesLabel` and `sizeIsValueMetric`
(`transformProps.ts:90-104`). The helper `isSizeSeries` at `transformProps.ts:364-367`
only treats series as size-series when `name === sizeSeriesLabel` or
`name.startsWith(\`${sizeSeriesLabel}, \`)`, so time-shifted size-series named according
to the documented patterns (e.g. `"sizeMetric__1 day ago"` or `"sizeMetric__1 day ago,
US"`) are not recognized as size-series.
4. Because `rawSeries` is built as `sizeSeriesLabel ? allRawSeries.filter(entry =>
!isSizeSeries(String(entry.name ?? ''))) : allRawSeries` (`transformProps.ts:105-111`),
those unrecognized time-shifted size-metric series remain in `rawSeries` and are then
iterated in the `rawSeries.forEach(entry => { ... const transformedSeries =
transformSeries(...); })` loop (`transformProps.ts:223-235`), causing the size metric's
time-comparison series to be rendered as normal scatter/legend series instead of being
used solely to drive marker sizes.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a96a8b044b604b6db83f913bd13088c0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a96a8b044b604b6db83f913bd13088c0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
**Line:** 364:367
**Comment:**
*Logic Error: The size-series detection only matches exact metric labels or the `<metric>, <group>` prefix, but time-comparison series are also emitted as patterns like `<metric>__<offset>` (and related offset forms). Those derived size series will not be filtered out, so the size metric can incorrectly render as visible chart/legend series instead of being used only for dot sizing. Extend the matcher to recognize time-offset naming patterns before filtering.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=0e6bdd5e5bcd23ebdd413ea88fc6d4442e2e7bf7880b69b7579db12fbb577983&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=0e6bdd5e5bcd23ebdd413ea88fc6d4442e2e7bf7880b69b7579db12fbb577983&reaction=dislike'>👎</a>
--
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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #40967:
URL: https://github.com/apache/superset/pull/40967#issuecomment-4678465709
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/40967?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `85.59322%` with `17 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 64.31%. Comparing base ([`046b1b6`](https://app.codecov.io/gh/apache/superset/commit/046b1b61b3855d6f195601d95d2b67724788070f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`1fbde07`](https://app.codecov.io/gh/apache/superset/commit/1fbde07e300e35cae4ef822905e334e38f6b2744?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 3 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/40967?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...ts/src/Timeseries/Regular/Scatter/controlPanel.tsx](https://app.codecov.io/gh/apache/superset/pull/40967?src=pr&el=tree&filepath=superset-frontend%2Fplugins%2Fplugin-chart-echarts%2Fsrc%2FTimeseries%2FRegular%2FScatter%2FcontrolPanel.tsx&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvVGltZXNlcmllcy9SZWd1bGFyL1NjYXR0ZXIvY29udHJvbFBhbmVsLnRzeA==) | 65.85% | [14 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/40967?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...gin-chart-echarts/src/Timeseries/transformProps.ts](https://app.codecov.io/gh/apache/superset/pull/40967?src=pr&el=tree&filepath=superset-frontend%2Fplugins%2Fplugin-chart-echarts%2Fsrc%2FTimeseries%2FtransformProps.ts&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvVGltZXNlcmllcy90cmFuc2Zvcm1Qcm9wcy50cw==) | 95.65% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/40967?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
:x: Your project check has failed because the head coverage (99.95%) is below the target coverage (100.00%). You can increase the head coverage or adjust the [target](https://docs.codecov.com/docs/commit-status#target) coverage.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #40967 +/- ##
==========================================
+ Coverage 64.29% 64.31% +0.01%
==========================================
Files 2657 2657
Lines 144027 144140 +113
Branches 33207 33263 +56
==========================================
+ Hits 92604 92701 +97
- Misses 49803 49819 +16
Partials 1620 1620
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/40967/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [javascript](https://app.codecov.io/gh/apache/superset/pull/40967/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `68.25% <85.59%> (+0.02%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/40967?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #40967:
URL: https://github.com/apache/superset/pull/40967#discussion_r3394639721
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/buildQuery.test.ts:
##########
@@ -46,6 +46,24 @@ describe('Timeseries buildQuery', () => {
expect(query.orderby).toEqual([['bar', false]]);
});
+ test('should include the scatter dot size metric in query metrics', () => {
+ const queryContext = buildQuery({
+ ...formData,
+ size: 'qux',
+ });
+ const [query] = queryContext.queries;
+ expect(query.metrics).toEqual(['bar', 'baz', 'qux']);
+ });
Review Comment:
`size` does get into the query metrics, just not via `buildQuery` itself — `extractQueryFields` in core aliases `size: 'metrics'`, so the value flows in through `buildQueryObject`. The test passes in CI for that reason.
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/buildQuery.test.ts:
##########
@@ -46,6 +46,24 @@ describe('Timeseries buildQuery', () => {
expect(query.orderby).toEqual([['bar', false]]);
});
+ test('should include the scatter dot size metric in query metrics', () => {
+ const queryContext = buildQuery({
+ ...formData,
+ size: 'qux',
+ });
+ const [query] = queryContext.queries;
+ expect(query.metrics).toEqual(['bar', 'baz', 'qux']);
+ });
+
+ test('should dedupe the dot size metric when it is also a value metric', () => {
+ const queryContext = buildQuery({
+ ...formData,
+ size: 'bar',
+ });
+ const [query] = queryContext.queries;
+ expect(query.metrics).toEqual(['bar', 'baz']);
+ });
Review Comment:
Same as the sibling thread — `extractQueryFields` aliases `size` into `metrics` and `removeDuplicates` handles the dedupe, so the value-metric collision case works without anything in `buildQuery.ts`. Green in 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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40967:
URL: https://github.com/apache/superset/pull/40967#issuecomment-5037990463
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40967:
URL: https://github.com/apache/superset/pull/40967#issuecomment-4678410185
<!-- Bito Reply -->
The architect review correctly identifies that time-comparison size series (e.g., `<metric>__<offset>`) are not currently recognized as size-only series, causing them to leak into `rawSeries` and render incorrectly in the legend. To resolve this, you should update the `isSizeSeries` detection logic to include these time-comparison patterns.
### Proposed Fix
In `superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts`, update the `isSizeSeries` helper (or equivalent logic) to account for the `__` separator used in time comparisons:
```typescript
// Example of updated detection logic
const isSizeSeries = (seriesName: string, sizeMetric: string) => {
const basePattern = `^${sizeMetric}(, .*)?$`;
const timeComparisonPattern = `^${sizeMetric}__.*(, .*)?$`;
return new RegExp(`${basePattern}|${timeComparisonPattern}`).test(seriesName);
};
```
This change ensures that both standard size metrics and their time-comparison variants are correctly identified and excluded from `rawSeries` and `sizeLookups`.
**superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts**
```
const isSizeSeries = (seriesName: string, sizeMetric: string) => {
const basePattern = `^${sizeMetric}(, .*)?$`;
const timeComparisonPattern = `^${sizeMetric}__.*(, .*)?$`;
return new RegExp(`${basePattern}|${timeComparisonPattern}`).test(seriesName);
};
```
--
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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #40967:
URL: https://github.com/apache/superset/pull/40967#discussion_r3400934911
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/controlPanel.test.ts:
##########
@@ -148,3 +148,59 @@ test('x_axis_number_format control should be hidden for non-numeric data types',
expect(isNumberVisible(null, null)).toBe(false);
expect(isNumberVisible('time_column', GenericDataType.Temporal)).toBe(false);
});
+
+// tests for orientation and dot size controls
+const orientationControl: any = getControl('orientation');
+const sizeControl: any = getControl('size');
+const minMarkerSizeControl: any = getControl('minMarkerSize');
+const maxMarkerSizeControl: any = getControl('maxMarkerSize');
Review Comment:
Good call. Added a narrow `TestControl` interface for these lookups, so the `any`s are gone.
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/controlPanel.test.ts:
##########
@@ -148,3 +148,59 @@ test('x_axis_number_format control should be hidden for non-numeric data types',
expect(isNumberVisible(null, null)).toBe(false);
expect(isNumberVisible('time_column', GenericDataType.Temporal)).toBe(false);
});
+
+// tests for orientation and dot size controls
+const orientationControl: any = getControl('orientation');
+const sizeControl: any = getControl('size');
+const minMarkerSizeControl: any = getControl('minMarkerSize');
+const maxMarkerSizeControl: any = getControl('maxMarkerSize');
+
+test('scatter chart control panel should include an orientation control defaulting to vertical', () => {
+ expect(orientationControl).toBeDefined();
+ expect(orientationControl.config.default).toBe('vertical');
+ expect(orientationControl.config.options).toEqual([
+ ['vertical', expect.anything()],
+ ['horizontal', expect.anything()],
+ ]);
+});
+
+test('scatter chart control panel should include an optional dot size metric control', () => {
+ expect(sizeControl).toBeDefined();
+ expect(sizeControl.config.validators).toEqual([]);
+ expect(sizeControl.config.default).toBeNull();
+});
+
+const mockSizeControls = (
+ sizeValue: string | null,
+): ControlPanelsContainerProps =>
+ ({
+ controls: {
+ size: { value: sizeValue },
+ markerEnabled: { value: true },
+ },
+ }) as unknown as ControlPanelsContainerProps;
+
+test('dot size range controls should only be visible when a size metric is set', () => {
+ expect(minMarkerSizeControl.config.visibility(mockSizeControls(null))).toBe(
+ false,
+ );
+ expect(maxMarkerSizeControl.config.visibility(mockSizeControls(null))).toBe(
+ false,
+ );
+ expect(
+ minMarkerSizeControl.config.visibility(mockSizeControls('size_metric')),
+ ).toBe(true);
+ expect(
+ maxMarkerSizeControl.config.visibility(mockSizeControls('size_metric')),
+ ).toBe(true);
+});
+
+test('fixed marker size control should hide when a size metric is set', () => {
+ const markerSizeControl: any = getControl('markerSize');
Review Comment:
Same fix as the sibling thread... these all go through the typed `TestControl` now.
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/transformProps.test.ts:
##########
@@ -175,3 +175,283 @@ describe('Scatter Chart X-axis Number Formatting', () => {
},
);
});
+
+describe('Scatter Chart Orientation and Dot Size Metric', () => {
+ const baseFormData: EchartsTimeseriesFormData = {
+ ...DEFAULT_FORM_DATA,
+ colorScheme: 'supersetColors',
+ datasource: '1__table',
+ metrics: ['sum_val'],
+ x_axis: 'category_col',
+ groupby: [],
+ viz_type: 'echarts_timeseries_scatter',
+ seriesType: EchartsTimeseriesSeriesType.Scatter,
+ };
+
+ const categoricalData = [
+ {
+ data: [
+ { category_col: 'A', sum_val: 1, size_metric: 10 },
+ { category_col: 'B', sum_val: 2, size_metric: 25 },
+ { category_col: 'C', sum_val: 3, size_metric: 40 },
+ ],
+ colnames: ['category_col', 'sum_val', 'size_metric'],
+ coltypes: [
+ GenericDataType.String,
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ ],
+ label_map: {
+ category_col: ['category_col'],
+ sum_val: ['sum_val'],
+ size_metric: ['size_metric'],
+ },
+ },
+ ];
+
+ const baseChartPropsConfig = {
+ width: 800,
+ height: 600,
+ theme: supersetTheme,
+ };
+
+ const getScatterSeries = (props: ReturnType<typeof transformProps>) =>
+ (props.echartOptions.series as any[]).filter(s => s.type === 'scatter');
Review Comment:
Swapped this for a `TestScatterSeries[]` cast.
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/transformProps.test.ts:
##########
@@ -175,3 +175,283 @@ describe('Scatter Chart X-axis Number Formatting', () => {
},
);
});
+
+describe('Scatter Chart Orientation and Dot Size Metric', () => {
+ const baseFormData: EchartsTimeseriesFormData = {
+ ...DEFAULT_FORM_DATA,
+ colorScheme: 'supersetColors',
+ datasource: '1__table',
+ metrics: ['sum_val'],
+ x_axis: 'category_col',
+ groupby: [],
+ viz_type: 'echarts_timeseries_scatter',
+ seriesType: EchartsTimeseriesSeriesType.Scatter,
+ };
+
+ const categoricalData = [
+ {
+ data: [
+ { category_col: 'A', sum_val: 1, size_metric: 10 },
+ { category_col: 'B', sum_val: 2, size_metric: 25 },
+ { category_col: 'C', sum_val: 3, size_metric: 40 },
+ ],
+ colnames: ['category_col', 'sum_val', 'size_metric'],
+ coltypes: [
+ GenericDataType.String,
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ ],
+ label_map: {
+ category_col: ['category_col'],
+ sum_val: ['sum_val'],
+ size_metric: ['size_metric'],
+ },
+ },
+ ];
+
+ const baseChartPropsConfig = {
+ width: 800,
+ height: 600,
+ theme: supersetTheme,
+ };
+
+ const getScatterSeries = (props: ReturnType<typeof transformProps>) =>
+ (props.echartOptions.series as any[]).filter(s => s.type === 'scatter');
+
+ const singleMetricData = [
+ {
+ ...categoricalData[0],
+ data: categoricalData[0].data.map(row => ({
+ category_col: row.category_col,
+ sum_val: row.sum_val,
+ })),
+ colnames: ['category_col', 'sum_val'],
+ coltypes: [GenericDataType.String, GenericDataType.Numeric],
+ },
+ ];
+
+ test('horizontal orientation swaps the dimension axis onto the y-axis', () => {
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: singleMetricData,
+ formData: { ...baseFormData, orientation: 'horizontal' },
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const { xAxis, yAxis } = transformedProps.echartOptions as any;
Review Comment:
Casting to a small `TestAxis` type instead now.
--
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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #40967:
URL: https://github.com/apache/superset/pull/40967#discussion_r3402419510
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts:
##########
@@ -125,6 +125,35 @@ describe('transformSeries', () => {
// OpacityEnum.NonTransparent = 1 (not dimmed)
expect((result as any).itemStyle.opacity).toBe(1);
});
+
+ test('should use symbolSizeFn for symbolSize when provided', () => {
+ const symbolSizeFn = jest.fn(
+ (value: (number | string | null)[]) => Number(value[1]) * 2,
+ );
+ const opts = {
+ seriesType: EchartsTimeseriesSeriesType.Scatter,
+ markerSize: 7,
+ symbolSizeFn,
+ timeShiftColor: false,
+ };
+
+ const result = transformSeries(series, mockColorScale, 'test-key', opts);
+
+ expect((result as any).symbolSize).toBe(symbolSizeFn);
+ expect((result as any).symbolSize(['A', 4])).toBe(8);
Review Comment:
Fair point... swapped the `any` casts for `ScatterSeriesOption` so `symbolSize` is checked through the real type.
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts:
##########
@@ -125,6 +125,35 @@ describe('transformSeries', () => {
// OpacityEnum.NonTransparent = 1 (not dimmed)
expect((result as any).itemStyle.opacity).toBe(1);
});
+
+ test('should use symbolSizeFn for symbolSize when provided', () => {
+ const symbolSizeFn = jest.fn(
+ (value: (number | string | null)[]) => Number(value[1]) * 2,
+ );
+ const opts = {
+ seriesType: EchartsTimeseriesSeriesType.Scatter,
+ markerSize: 7,
+ symbolSizeFn,
+ timeShiftColor: false,
+ };
+
+ const result = transformSeries(series, mockColorScale, 'test-key', opts);
+
+ expect((result as any).symbolSize).toBe(symbolSizeFn);
+ expect((result as any).symbolSize(['A', 4])).toBe(8);
+ });
+
+ test('should fall back to markerSize for symbolSize when symbolSizeFn is not provided', () => {
+ const opts = {
+ seriesType: EchartsTimeseriesSeriesType.Scatter,
+ markerSize: 7,
+ timeShiftColor: false,
+ };
+
+ const result = transformSeries(series, mockColorScale, 'test-key', opts);
+
+ expect((result as any).symbolSize).toBe(7);
Review Comment:
Done! This one now goes through `ScatterSeriesOption` too.
--
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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas closed pull request #40967: feat(scatter): add chart orientation and dot size metric controls
URL: https://github.com/apache/superset/pull/40967
--
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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40967:
URL: https://github.com/apache/superset/pull/40967#discussion_r3400936778
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/controlPanel.test.ts:
##########
@@ -148,3 +148,59 @@ test('x_axis_number_format control should be hidden for non-numeric data types',
expect(isNumberVisible(null, null)).toBe(false);
expect(isNumberVisible('time_column', GenericDataType.Temporal)).toBe(false);
});
+
+// tests for orientation and dot size controls
+const orientationControl: any = getControl('orientation');
+const sizeControl: any = getControl('size');
+const minMarkerSizeControl: any = getControl('minMarkerSize');
+const maxMarkerSizeControl: any = getControl('maxMarkerSize');
+
+test('scatter chart control panel should include an orientation control defaulting to vertical', () => {
+ expect(orientationControl).toBeDefined();
+ expect(orientationControl.config.default).toBe('vertical');
+ expect(orientationControl.config.options).toEqual([
+ ['vertical', expect.anything()],
+ ['horizontal', expect.anything()],
+ ]);
+});
+
+test('scatter chart control panel should include an optional dot size metric control', () => {
+ expect(sizeControl).toBeDefined();
+ expect(sizeControl.config.validators).toEqual([]);
+ expect(sizeControl.config.default).toBeNull();
+});
+
+const mockSizeControls = (
+ sizeValue: string | null,
+): ControlPanelsContainerProps =>
+ ({
+ controls: {
+ size: { value: sizeValue },
+ markerEnabled: { value: true },
+ },
+ }) as unknown as ControlPanelsContainerProps;
+
+test('dot size range controls should only be visible when a size metric is set', () => {
+ expect(minMarkerSizeControl.config.visibility(mockSizeControls(null))).toBe(
+ false,
+ );
+ expect(maxMarkerSizeControl.config.visibility(mockSizeControls(null))).toBe(
+ false,
+ );
+ expect(
+ minMarkerSizeControl.config.visibility(mockSizeControls('size_metric')),
+ ).toBe(true);
+ expect(
+ maxMarkerSizeControl.config.visibility(mockSizeControls('size_metric')),
+ ).toBe(true);
+});
+
+test('fixed marker size control should hide when a size metric is set', () => {
+ const markerSizeControl: any = getControl('markerSize');
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> In this test file, use the typed `TestControl` for visibility assertions instead of `any`.
**Applied to:**
- `superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/controlPanel.test.ts`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40967:
URL: https://github.com/apache/superset/pull/40967#discussion_r3401945896
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/controlPanel.test.ts:
##########
@@ -33,7 +45,7 @@ const getControl = (controlName: string) => {
'name' in control &&
control.name === controlName
) {
- return control;
+ return control as unknown as TestControl;
Review Comment:
<!-- Bito Reply -->
The update to the `getControl` function correctly implements the suggested consolidation by casting the control to `TestControl`. This change ensures type safety while maintaining compatibility with the shared test utility.
**superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/controlPanel.test.ts**
```
control.name === controlName
) {
- return control;
+ return control as unknown as TestControl;
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts:
##########
@@ -205,6 +205,7 @@ export function transformSeries(
seriesContexts?: { [key: string]: ForecastSeriesEnum[] };
markerEnabled?: boolean;
markerSize?: number;
+ symbolSizeFn?: (value: (number | string | null)[]) => number;
Review Comment:
<!-- Bito Reply -->
The addition of unit tests for `symbolSizeFn` in `transformSeries` is a positive step that addresses the reviewer's concern regarding test coverage for the new parameter. These tests ensure that the fallback logic (`symbolSizeFn ?? markerSize`) functions correctly and that the function receives the expected data values, which is essential for maintaining the reliability of the scatter chart's dot-size 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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40967:
URL: https://github.com/apache/superset/pull/40967#discussion_r3394647045
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx:
##########
@@ -99,115 +428,56 @@ const config: ControlPanelConfig = {
'Size of marker. Also applies to forecast observations.',
),
visibility: ({ controls }: ControlPanelsContainerProps) =>
- Boolean(controls?.markerEnabled?.value),
+ Boolean(controls?.markerEnabled?.value) &&
+ !controls?.size?.value,
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not flag hiding the fixed marker size control when a size metric is selected in scatter chart control panels; using the minimum as the fallback for points without a size value is intentional.
**Applied to:**
- `superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/**`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40967:
URL: https://github.com/apache/superset/pull/40967#discussion_r3394255806
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx:
##########
@@ -99,115 +428,56 @@ const config: ControlPanelConfig = {
'Size of marker. Also applies to forecast observations.',
),
visibility: ({ controls }: ControlPanelsContainerProps) =>
- Boolean(controls?.markerEnabled?.value),
+ Boolean(controls?.markerEnabled?.value) &&
+ !controls?.size?.value,
Review Comment:
**Suggestion:** The fixed marker size control is hidden whenever a dot-size metric is selected, but the renderer still uses `markerSize` as the fallback for points without a size value. This makes fallback sizing effectively unconfigurable in the intended size-metric mode and leaves null/shifted points stuck at an implicit default. Keep a configurable fallback-size control visible when `size` is set (or add a dedicated fallback size control). [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Scatter charts with size metrics cannot tune fallback marker size.
- ⚠️ Null or time-shifted points render at unintended default size.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create or open an ECharts Timeseries Scatter chart (`viz_type:
'echarts_timeseries_scatter'`, see
`superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/transformProps.test.ts:43`)
which uses the control panel defined in
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx`.
2. In the Query section, set a "Dot size metric" via the `size` control defined at
`controlPanel.tsx:323-336`; this populates `formData.size` and makes `controls.size.value`
truthy in the control panel state.
3. Observe that in the Chart Options section, the "Marker Size" slider (`markerSize`
control at `controlPanel.tsx:419-427`) becomes hidden because its `visibility` function at
`controlPanel.tsx:430-432` requires both `markerEnabled` and `!controls?.size?.value`,
which is false once a size metric is selected.
4. At render time, the chart uses `markerSize` as a runtime fallback for points without a
numeric size value:
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:237-247`
defines `symbolSizeFn`, which returns `markerSize` whenever `sizeValue` is not a finite
number. With the `markerSize` control hidden in size-metric mode, users cannot adjust this
fallback size while configuring dot-size metrics, so null/derived points remain stuck at
the implicit default or a previously-set value that is no longer visible.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2c757ad8e9e34c93b1dbd0829ffc40a5&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=2c757ad8e9e34c93b1dbd0829ffc40a5&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx
**Line:** 430:432
**Comment:**
*Incomplete Implementation: The fixed marker size control is hidden whenever a dot-size metric is selected, but the renderer still uses `markerSize` as the fallback for points without a size value. This makes fallback sizing effectively unconfigurable in the intended size-metric mode and leaves null/shifted points stuck at an implicit default. Keep a configurable fallback-size control visible when `size` is set (or add a dedicated fallback size control).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=2360b0e4c11b0485f5336260956bc6d20bdc6d91bc0c1409d430c05c802d8d70&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=2360b0e4c11b0485f5336260956bc6d20bdc6d91bc0c1409d430c05c802d8d70&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx:
##########
@@ -99,115 +428,56 @@ const config: ControlPanelConfig = {
'Size of marker. Also applies to forecast observations.',
),
visibility: ({ controls }: ControlPanelsContainerProps) =>
- Boolean(controls?.markerEnabled?.value),
+ Boolean(controls?.markerEnabled?.value) &&
+ !controls?.size?.value,
},
},
],
- ['zoomable'],
- [minorTicks],
- ...legendSection,
- [<ControlSubSectionHeader>{t('X Axis')}</ControlSubSectionHeader>],
-
[
{
- name: 'x_axis_time_format',
- config: {
- ...sharedControls.x_axis_time_format,
- default: 'smart_date',
- description: `${D3_TIME_FORMAT_DOCS}. ${TIME_SERIES_DESCRIPTION_TEXT}`,
- visibility: ({ controls }: ControlPanelsContainerProps) =>
- checkColumnType(
- getColumnLabel(controls?.x_axis?.value as QueryFormColumn),
- controls?.datasource?.datasource,
- [GenericDataType.Temporal],
- ),
- disableStash: true,
- resetOnHide: false,
- },
- },
- {
- name: 'x_axis_number_format',
+ name: 'minMarkerSize',
config: {
- ...sharedControls.x_axis_number_format,
- default: '~g',
- mapStateToProps: undefined,
- visibility: ({ controls }: ControlPanelsContainerProps) =>
- checkColumnType(
- getColumnLabel(controls?.x_axis?.value as QueryFormColumn),
- controls?.datasource?.datasource,
- [GenericDataType.Numeric],
- ),
- },
- },
- ],
- [xAxisLabelRotation],
- [xAxisLabelInterval],
- [forceMaxInterval],
- // eslint-disable-next-line react/jsx-key
- ...richTooltipSection,
- // eslint-disable-next-line react/jsx-key
- [<ControlSubSectionHeader>{t('Y Axis')}</ControlSubSectionHeader>],
- ['y_axis_format'],
- ['currency_format'],
- [
- {
- name: 'logAxis',
- config: {
- type: 'CheckboxControl',
- label: t('Logarithmic y-axis'),
- renderTrigger: true,
- default: logAxis,
- description: t('Logarithmic y-axis'),
- },
- },
- ],
- [
- {
- name: 'minorSplitLine',
- config: {
- type: 'CheckboxControl',
- label: t('Minor Split Line'),
- renderTrigger: true,
- default: minorSplitLine,
- description: t('Draw split lines for minor y-axis ticks'),
- },
- },
- ],
- [truncateXAxis],
- [xAxisBounds],
- [
- {
- name: 'truncateYAxis',
- config: {
- type: 'CheckboxControl',
- label: t('Truncate Y Axis'),
- default: truncateYAxis,
+ type: 'SliderControl',
+ label: t('Minimum dot size'),
renderTrigger: true,
+ min: 1,
+ max: 100,
+ default: minMarkerSize,
description: t(
- 'Truncate Y Axis. Can be overridden by specifying a min or max bound.',
+ 'Size of the dot representing the smallest value of the dot size metric.',
),
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ Boolean(controls?.size?.value),
},
},
- ],
- [
{
- name: 'y_axis_bounds',
+ name: 'maxMarkerSize',
config: {
- type: 'BoundsControl',
- label: t('Y Axis Bounds'),
+ type: 'SliderControl',
+ label: t('Maximum dot size'),
renderTrigger: true,
- default: yAxisBounds,
+ min: 1,
+ max: 100,
+ default: maxMarkerSize,
description: t(
- 'Bounds for the Y-axis. When left empty, the bounds are ' +
- 'dynamically defined based on the min/max of the data. Note that ' +
- "this feature will only expand the axis range. It won't " +
- "narrow the data's extent.",
+ 'Size of the dot representing the largest value of the dot size metric.',
),
visibility: ({ controls }: ControlPanelsContainerProps) =>
- Boolean(controls?.truncateYAxis?.value),
+ Boolean(controls?.size?.value),
Review Comment:
**Suggestion:** `minMarkerSize` and `maxMarkerSize` are independent sliders with no cross-field validation, so users can save `min > max`, which inverts the size mapping and contradicts the "minimum/maximum" semantics. Add validation (or normalization) to enforce `minMarkerSize <= maxMarkerSize`. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Dot-size scaling reverses when user sets min > max.
- ⚠️ "Minimum/maximum dot size" labels no longer match rendering.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open an ECharts Timeseries Scatter chart (`viz_type: 'echarts_timeseries_scatter'`,
tested in `test/Timeseries/Scatter/transformProps.test.ts`) using the control panel
defined in
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx`.
2. Select a "Dot size metric" so that the `size` control at `controlPanel.tsx:323-336` is
populated; this makes the `minMarkerSize` and `maxMarkerSize` sliders visible due to the
`visibility` conditions at `controlPanel.tsx:449-450` and `controlPanel.tsx:465-466` which
only check `Boolean(controls?.size?.value)`.
3. In the Chart Options section, drag "Minimum dot size" above "Maximum dot size" (e.g.,
minMarkerSize = 40, maxMarkerSize = 10). There are no `validators` or cross-field checks
on these controls at `controlPanel.tsx:438-452` and `454-467`, so `formData` can be saved
with `minMarkerSize > maxMarkerSize`.
4. When the chart renders,
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:97-99`
constructs `markerSizeRange: [number, number] = [minMarkerSize, maxMarkerSize]` and passes
it to `getAreaScaledSymbolSize` via `symbolSizeFn` at `transformProps.ts:237-247`. With an
inverted range ([40, 10]), `getAreaScaledSymbolSize` (`utils/series.ts:15-31`) still
returns values between 10 and 40, but the mapping is reversed (smallest metric gets the
largest diameter, largest metric the smallest), contradicting the "Minimum/Maximum dot
size" semantics and producing visually incorrect scaling.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f8790d723ec243579ee83872ad79a808&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f8790d723ec243579ee83872ad79a808&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx
**Line:** 438:466
**Comment:**
*Incorrect Condition Logic: `minMarkerSize` and `maxMarkerSize` are independent sliders with no cross-field validation, so users can save `min > max`, which inverts the size mapping and contradicts the "minimum/maximum" semantics. Add validation (or normalization) to enforce `minMarkerSize <= maxMarkerSize`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=8a90534a2e5aa4c1a1a38a33253038b526cea7cdc1cc7a584dd4432ca5557d7f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=8a90534a2e5aa4c1a1a38a33253038b526cea7cdc1cc7a584dd4432ca5557d7f&reaction=dislike'>👎</a>
--
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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40967:
URL: https://github.com/apache/superset/pull/40967#discussion_r3400625645
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -337,6 +342,89 @@ export default function transformProps(
xAxisType,
},
);
+
+ // Dot size by metric (scatter): the size metric's series are excluded from
+ // rendering and instead provide per-point values that scale each marker's
+ // area between minMarkerSize and maxMarkerSize.
+ const sizeMetricLabel =
+ seriesType === EchartsTimeseriesSeriesType.Scatter && size
+ ? getMetricLabel(size)
+ : undefined;
+ const sizeSeriesLabel = isDefined(sizeMetricLabel)
+ ? (verboseMap[sizeMetricLabel!] ?? sizeMetricLabel)
+ : undefined;
+ const valueMetricLabels = ensureIsArray(metrics)
+ .map(getMetricLabel)
+ .map(label => verboseMap[label] ?? label);
+ // When the size metric is also a value metric, the query dedupes them into a
+ // single column, so each point's own value doubles as its size value.
+ const sizeIsValueMetric = isDefined(sizeSeriesLabel)
+ ? valueMetricLabels.includes(sizeSeriesLabel!)
+ : false;
+ const isSizeSeries = (name: string) =>
+ isDefined(sizeSeriesLabel) &&
+ !sizeIsValueMetric &&
+ (name === sizeSeriesLabel || name.startsWith(`${sizeSeriesLabel}, `));
+ const rawSeries = sizeSeriesLabel
+ ? allRawSeries.filter(entry => !isSizeSeries(String(entry.name ?? '')))
+ : allRawSeries;
+ // Maps each value series' dimension key to a lookup from primary-axis value
+ // to size value.
+ let sizeLookups: Map<string, Map<DataRecordValue, number>> | undefined;
+ let sizeExtent: [number, number] | undefined;
+ if (sizeSeriesLabel) {
+ let sizeMin = Infinity;
+ let sizeMax = -Infinity;
+ if (sizeIsValueMetric) {
+ rawSeries.forEach(entry => {
+ (entry.data as DataRecordValue[][]).forEach(datum => {
+ const sizeValue = isHorizontal ? datum[0] : datum[1];
+ if (typeof sizeValue === 'number' && Number.isFinite(sizeValue)) {
+ sizeMin = Math.min(sizeMin, sizeValue);
+ sizeMax = Math.max(sizeMax, sizeValue);
+ }
+ });
+ });
+ } else {
+ sizeLookups = new Map();
+ allRawSeries
+ .filter(entry => isSizeSeries(String(entry.name ?? '')))
+ .forEach(entry => {
+ const name = String(entry.name ?? '');
+ const dimsKey =
+ name === sizeSeriesLabel
+ ? ''
+ : name.slice(sizeSeriesLabel.length + 2);
+ const lookup = new Map<DataRecordValue, number>();
+ (entry.data as DataRecordValue[][]).forEach(datum => {
+ const axisValue = isHorizontal ? datum[1] : datum[0];
+ const sizeValue = isHorizontal ? datum[0] : datum[1];
+ if (typeof sizeValue === 'number' && Number.isFinite(sizeValue)) {
+ lookup.set(axisValue, sizeValue);
+ sizeMin = Math.min(sizeMin, sizeValue);
+ sizeMax = Math.max(sizeMax, sizeValue);
+ }
+ });
+ sizeLookups!.set(dimsKey, lookup);
+ });
+ }
+ if (sizeMin <= sizeMax) {
+ sizeExtent = [sizeMin, sizeMax];
+ }
+ }
+ // Strips the metric label off a series name, leaving the dimension key used
+ // to match a value series with its size series.
+ const getSeriesDimsKey = (name: string): string => {
+ const matchedLabel = valueMetricLabels.find(
+ label => name === label || name.startsWith(`${label}, `),
+ );
+ if (matchedLabel === undefined) {
+ return name;
+ }
+ return name === matchedLabel ? '' : name.slice(matchedLabel.length + 2);
+ };
+ const markerSizeRange: [number, number] = [minMarkerSize, maxMarkerSize];
Review Comment:
**Suggestion:** The code passes `minMarkerSize` and `maxMarkerSize` through without normalizing or validating their order, so if users set minimum larger than maximum the scaling inverts (larger metric values render smaller dots). Normalize the range before use (or enforce min <= max in controls) to preserve the documented semantics. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Higher size metrics can render with smaller dots.
- ⚠️ Dot-size controls allow contradictory min/max configuration.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In the "Generic Chart" Scatter variant, set a "Dot size metric" and then use the
"Minimum dot size" and "Maximum dot size" sliders configured in
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx:59-89`.
The UI allows choosing a minimum larger than the maximum because the two sliders are
independent and have no mutual validation.
2. Save or run the chart so that form data flows into `transformProps` in
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:20-82`,
where `minMarkerSize` and `maxMarkerSize` are destructured from
`EchartsTimeseriesFormData` without adjustment (defaults at `constants.ts:64-68` but
overridden by the user's selections).
3. `transformProps.ts:419-427` constructs `markerSizeRange: [number, number] =
[minMarkerSize, maxMarkerSize];` (diff line 426) and later builds `symbolSizeFn` at
`transformProps.ts:145-159`, which calls `getAreaScaledSymbolSize(sizeValue, extent,
markerSizeRange)` for each data point when `sizeExtent` is defined.
4. `getAreaScaledSymbolSize` in
`superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:6-23` interprets
`sizeRange` as `[minSize, maxSize]` and computes `sqrt(minSize ** 2 + ratio * (maxSize **
2 - minSize ** 2))`. When `minMarkerSize > maxMarkerSize`, `maxSize ** 2 - minSize ** 2`
is negative, so as `ratio` (and the metric value) increases, the returned size decreases,
causing larger metric values to render with smaller dots, inverting the intended sizing
semantics.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e356e02b3a274dcf935e7c20a2f6a766&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=e356e02b3a274dcf935e7c20a2f6a766&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
**Line:** 426:426
**Comment:**
*Incorrect Condition Logic: The code passes `minMarkerSize` and `maxMarkerSize` through without normalizing or validating their order, so if users set minimum larger than maximum the scaling inverts (larger metric values render smaller dots). Normalize the range before use (or enforce min <= max in controls) to preserve the documented semantics.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=4917579fa5e0ffbb675b4e7a2ebadcf6df92232bfb8dda9471109588dae57134&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=4917579fa5e0ffbb675b4e7a2ebadcf6df92232bfb8dda9471109588dae57134&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -472,6 +560,24 @@ export default function transformProps(
}
}
+ let symbolSizeFn:
+ | ((value: (number | string | null)[]) => number)
+ | undefined;
+ if (sizeExtent) {
+ const extent = sizeExtent;
+ const sizeLookup = sizeLookups?.get(getSeriesDimsKey(entryName));
+ if (sizeIsValueMetric || sizeLookup) {
+ symbolSizeFn = value => {
+ const sizeValue = sizeIsValueMetric
+ ? value[isHorizontal ? 0 : 1]
+ : sizeLookup!.get(value[isHorizontal ? 1 : 0]);
+ return typeof sizeValue === 'number'
+ ? getAreaScaledSymbolSize(sizeValue, extent, markerSizeRange)
+ : markerSize;
Review Comment:
**Suggestion:** Points without a valid size metric currently fall back to `markerSize`, but that control is hidden when a dot-size metric is selected. This makes fallback sizing depend on a stale/hidden value instead of the configured dot-size range and produces inconsistent marker sizes for null/missing size values. Use `minMarkerSize` (or the lower bound of the configured range) as the fallback so missing values follow the visible dot-size controls. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Scatter chart sized dots render inconsistent for null sizes.
- ⚠️ Hidden markerSize overrides visible dot-size configuration.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create or edit a "Generic Chart" using the ECharts timeseries plugin, select the
Scatter series type, and configure a "Dot size metric" in the control panel defined in
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx:24-37`
(size metric row) and `:55-89` (minMarkerSize/maxMarkerSize sliders).
2. Ensure your query returns some points where the dot-size metric is null or missing
(e.g., some dimension rows lack that metric). These values flow into `extractSeries()` and
then into the size-mapping logic in
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:147-215`,
where `sizeLookups` and `sizeExtent` are computed only for finite numeric size values.
3. When `rawSeries.forEach` runs in `transformProps.ts:64-161`, the code constructs
`symbolSizeFn` at `transformProps.ts:145-159`. For a point whose size metric is
null/undefined or absent from `sizeLookup`, `sizeValue` is not a number, so the ternary at
`transformProps.ts:156-159` returns `markerSize` instead of a value derived from
`[minMarkerSize, maxMarkerSize]`.
4. `transformSeries()` in
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts:197-201,239-243,159`
receives both `markerSize` and `symbolSizeFn`; it assigns `symbolSize: symbolSizeFn ??
markerSize`, so such points are rendered using `markerSize`. However, when a dot-size
metric is set, the `markerSize` slider is hidden (Scatter control panel visibility logic
at `Regular/Scatter/controlPanel.tsx:39-54`), while only `minMarkerSize`/`maxMarkerSize`
are visible (`:59-89`), causing points with missing size values to use a hidden/stale
`markerSize` that can fall outside the configured dot-size range.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3f4459092f1b449ca6e8aceaff338daa&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=3f4459092f1b449ca6e8aceaff338daa&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
**Line:** 574:576
**Comment:**
*Logic Error: Points without a valid size metric currently fall back to `markerSize`, but that control is hidden when a dot-size metric is selected. This makes fallback sizing depend on a stale/hidden value instead of the configured dot-size range and produces inconsistent marker sizes for null/missing size values. Use `minMarkerSize` (or the lower bound of the configured range) as the fallback so missing values follow the visible dot-size controls.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=1419211f6171cf5b113e61c44f741e6d7e16ca3be3781bd4f8e66c974fa18042&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=1419211f6171cf5b113e61c44f741e6d7e16ca3be3781bd4f8e66c974fa18042&reaction=dislike'>👎</a>
--
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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #40967:
URL: https://github.com/apache/superset/pull/40967#discussion_r3400936782
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -472,6 +560,24 @@ export default function transformProps(
}
}
+ let symbolSizeFn:
+ | ((value: (number | string | null)[]) => number)
+ | undefined;
+ if (sizeExtent) {
+ const extent = sizeExtent;
+ const sizeLookup = sizeLookups?.get(getSeriesDimsKey(entryName));
+ if (sizeIsValueMetric || sizeLookup) {
+ symbolSizeFn = value => {
+ const sizeValue = sizeIsValueMetric
+ ? value[isHorizontal ? 0 : 1]
+ : sizeLookup!.get(value[isHorizontal ? 1 : 0]);
+ return typeof sizeValue === 'number'
+ ? getAreaScaledSymbolSize(sizeValue, extent, markerSizeRange)
+ : markerSize;
Review Comment:
Yeah, that fallback was pointing at a hidden control. Missing size values fall back to `minMarkerSize` now.
--
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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #40967:
URL: https://github.com/apache/superset/pull/40967#discussion_r3401944381
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts:
##########
@@ -205,6 +205,7 @@ export function transformSeries(
seriesContexts?: { [key: string]: ForecastSeriesEnum[] };
markerEnabled?: boolean;
markerSize?: number;
+ symbolSizeFn?: (value: (number | string | null)[]) => number;
Review Comment:
Good call... added transformSeries unit tests covering the symbolSizeFn passthrough and the markerSize fallback.
--
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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40967:
URL: https://github.com/apache/superset/pull/40967#issuecomment-4691023430
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #40967:
URL: https://github.com/apache/superset/pull/40967#discussion_r3625656422
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx:
##########
@@ -99,114 +428,56 @@ const config: ControlPanelConfig = {
'Size of marker. Also applies to forecast observations.',
),
visibility: ({ controls }: ControlPanelsContainerProps) =>
- Boolean(controls?.markerEnabled?.value),
+ Boolean(controls?.markerEnabled?.value) &&
+ !controls?.size?.value,
},
},
],
- ['zoomable'],
- [minorTicks],
- ...legendSection,
- [<ControlSubSectionHeader>{t('X Axis')}</ControlSubSectionHeader>],
-
[
{
- name: 'x_axis_time_format',
- config: {
- ...sharedControls.x_axis_time_format,
- description: `${D3_TIME_FORMAT_DOCS}. ${TIME_SERIES_DESCRIPTION_TEXT}`,
- visibility: ({ controls }: ControlPanelsContainerProps) =>
- checkColumnType(
- getColumnLabel(controls?.x_axis?.value as QueryFormColumn),
- controls?.datasource?.datasource,
- [GenericDataType.Temporal],
- ),
- disableStash: true,
- resetOnHide: false,
- },
- },
- {
- name: 'x_axis_number_format',
+ name: 'minMarkerSize',
config: {
- ...sharedControls.x_axis_number_format,
- default: '~g',
- mapStateToProps: undefined,
- visibility: ({ controls }: ControlPanelsContainerProps) =>
- checkColumnType(
- getColumnLabel(controls?.x_axis?.value as QueryFormColumn),
- controls?.datasource?.datasource,
- [GenericDataType.Numeric],
- ),
- },
- },
- ],
- [xAxisLabelRotation],
- [xAxisLabelInterval],
- [forceMaxInterval],
- // eslint-disable-next-line react/jsx-key
- ...richTooltipSection,
- // eslint-disable-next-line react/jsx-key
- [<ControlSubSectionHeader>{t('Y Axis')}</ControlSubSectionHeader>],
- ['y_axis_format'],
- ['currency_format'],
- [
- {
- name: 'logAxis',
- config: {
- type: 'CheckboxControl',
- label: t('Logarithmic y-axis'),
- renderTrigger: true,
- default: logAxis,
- description: t('Logarithmic y-axis'),
- },
- },
- ],
- [
- {
- name: 'minorSplitLine',
- config: {
- type: 'CheckboxControl',
- label: t('Minor Split Line'),
- renderTrigger: true,
- default: minorSplitLine,
- description: t('Draw split lines for minor y-axis ticks'),
- },
- },
- ],
- [truncateXAxis],
- [xAxisBounds],
- [
- {
- name: 'truncateYAxis',
- config: {
- type: 'CheckboxControl',
- label: t('Truncate Y Axis'),
- default: truncateYAxis,
+ type: 'SliderControl',
+ label: t('Minimum dot size'),
renderTrigger: true,
+ min: 1,
+ max: 100,
+ default: minMarkerSize,
description: t(
- 'Truncate Y Axis. Can be overridden by specifying a min or max bound.',
+ 'Size of the dot representing the smallest value of the dot size metric.',
),
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ Boolean(controls?.size?.value),
},
},
- ],
- [
{
- name: 'y_axis_bounds',
+ name: 'maxMarkerSize',
config: {
- type: 'BoundsControl',
- label: t('Y Axis Bounds'),
+ type: 'SliderControl',
+ label: t('Maximum dot size'),
renderTrigger: true,
- default: yAxisBounds,
+ min: 1,
+ max: 100,
+ default: maxMarkerSize,
description: t(
- 'Bounds for the Y-axis. When left empty, the bounds are ' +
- 'dynamically defined based on the min/max of the data. Note that ' +
- "this feature will only expand the axis range. It won't " +
- "narrow the data's extent.",
+ 'Size of the dot representing the largest value of the dot size metric.',
),
visibility: ({ controls }: ControlPanelsContainerProps) =>
- Boolean(controls?.truncateYAxis?.value),
+ Boolean(controls?.size?.value),
},
},
],
+ ['zoomable'],
+ [minorTicks],
+ ...legendSection,
+ [<ControlSubSectionHeader>{t('X Axis')}</ControlSubSectionHeader>],
+ ...createAxisControl('x'),
+ [truncateXAxis],
+ [xAxisBounds],
+ [forceMaxInterval],
Review Comment:
Those are keyed off `truncateXAxis`/`xAxisBounds` specifically, they are about the underlying dimension column regardless of which physical axis it renders on, not the visual X-position. Bar's control panel pins them under the X Axis header the same way (Timeseries/Regular/Bar/controlPanel.tsx), so this matches the existing convention rather than a scatter-specific bug.
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/SmoothLine/controlPanel.test.ts:
##########
@@ -16,58 +16,14 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { ControlPanelsContainerProps } from '@superset-ui/chart-controls/types';
import { GenericDataType } from '@apache-superset/core/common';
import controlPanel from '../../../src/Timeseries/Regular/SmoothLine/controlPanel';
+import { getControl, mockControls } from '../helpers';
Review Comment:
`mockControls` is used in this file, e.g. `visibilityFn(mockControls('year', GenericDataType.Numeric))` a few lines down. Not an unused import.
--
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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40967:
URL: https://github.com/apache/superset/pull/40967#discussion_r3394274764
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/buildQuery.test.ts:
##########
@@ -46,6 +46,24 @@ describe('Timeseries buildQuery', () => {
expect(query.orderby).toEqual([['bar', false]]);
});
+ test('should include the scatter dot size metric in query metrics', () => {
+ const queryContext = buildQuery({
+ ...formData,
+ size: 'qux',
+ });
+ const [query] = queryContext.queries;
+ expect(query.metrics).toEqual(['bar', 'baz', 'qux']);
+ });
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Test validates non-existent code</b></div>
<div id="fix">
This test (lines 49-56) expects `buildQuery` to include `formData.size` ('qux') in query metrics, but `buildQuery.ts` does not reference or handle the `size` property. The `extractExtraMetrics` helper only processes `timeseries_limit_metric` and `x_axis_sort`. This test will always fail until the size metric extraction logic is implemented in the source file.
</div>
</div>
<small><i>Code Review Run #1871ad</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/test/Timeseries/buildQuery.test.ts:
##########
@@ -46,6 +46,24 @@ describe('Timeseries buildQuery', () => {
expect(query.orderby).toEqual([['bar', false]]);
});
+ test('should include the scatter dot size metric in query metrics', () => {
+ const queryContext = buildQuery({
+ ...formData,
+ size: 'qux',
+ });
+ const [query] = queryContext.queries;
+ expect(query.metrics).toEqual(['bar', 'baz', 'qux']);
+ });
+
+ test('should dedupe the dot size metric when it is also a value metric', () => {
+ const queryContext = buildQuery({
+ ...formData,
+ size: 'bar',
+ });
+ const [query] = queryContext.queries;
+ expect(query.metrics).toEqual(['bar', 'baz']);
+ });
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Test validates non-existent dedup logic</b></div>
<div id="fix">
This test (lines 58-65) expects `buildQuery` to deduplicate the size metric when it matches an existing value metric. However, `buildQuery.ts` does not extract or handle `formData.size` at all, so deduplication logic cannot exist. The test cannot pass until size metric handling is implemented in the source.
</div>
</div>
<small><i>Code Review Run #1871ad</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/test/Timeseries/Scatter/controlPanel.test.ts:
##########
@@ -24,7 +24,7 @@ const config = controlPanel;
const getControl = (controlName: string) => {
for (const section of config.controlPanelSections) {
- if (section && section.controlSetRows) {
+ if (section?.controlSetRows) {
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicate test control setup across test files</b></div>
<div id="fix">
Detected ~43 lines of duplicate test setup code across controlPanel.test.ts files (Scatter:27‑69, Area:27‑69, SmoothLine:27‑69, Step:27‑69, Line:27‑69). Consider moving common code to a shared helper to improve maintainability.
</div>
</div>
<small><i>Code Review Run #1871ad</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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40967:
URL: https://github.com/apache/superset/pull/40967#discussion_r3401685897
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts:
##########
@@ -205,6 +205,7 @@ export function transformSeries(
seriesContexts?: { [key: string]: ForecastSeriesEnum[] };
markerEnabled?: boolean;
markerSize?: number;
+ symbolSizeFn?: (value: (number | string | null)[]) => number;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing unit tests for symbolSizeFn</b></div>
<div id="fix">
The new `symbolSizeFn` parameter lacks unit test coverage in `transformers.test.ts`. Per BITO.md rule [6262], tests should verify actual business logic. Add tests to validate the fallback behavior (`symbolSizeFn ?? markerSize`) and confirm the function receives correct data values.
</div>
</div>
<small><i>Code Review Run #b98d61</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/test/Timeseries/Scatter/controlPanel.test.ts:
##########
@@ -33,7 +45,7 @@ const getControl = (controlName: string) => {
'name' in control &&
control.name === controlName
) {
- return control;
+ return control as unknown as TestControl;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Consolidate duplicate mockControls utility across chart test files</b></div>
<div id="fix">
The mockControls helper function and timeFormatControl definition are duplicated across 5 test files (Scatter, Line, Area, SmoothLine, Step). Consider extracting these into a shared test utility to reduce duplication and improve maintainability. Line ranges: Scatter (48-81), Line/Area/SmoothLine/Step (36-69).
</div>
</div>
<small><i>Code Review Run #b98d61</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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #40967:
URL: https://github.com/apache/superset/pull/40967#discussion_r3401944594
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/controlPanel.test.ts:
##########
@@ -33,7 +45,7 @@ const getControl = (controlName: string) => {
'name' in control &&
control.name === controlName
) {
- return control;
+ return control as unknown as TestControl;
Review Comment:
Done! Pulled `getControl`/`mockControls` into the shared Timeseries test helpers and pointed all five files at it.
--
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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40967:
URL: https://github.com/apache/superset/pull/40967#discussion_r3623731856
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx:
##########
@@ -99,114 +428,56 @@ const config: ControlPanelConfig = {
'Size of marker. Also applies to forecast observations.',
),
visibility: ({ controls }: ControlPanelsContainerProps) =>
- Boolean(controls?.markerEnabled?.value),
+ Boolean(controls?.markerEnabled?.value) &&
+ !controls?.size?.value,
},
},
],
- ['zoomable'],
- [minorTicks],
- ...legendSection,
- [<ControlSubSectionHeader>{t('X Axis')}</ControlSubSectionHeader>],
-
[
{
- name: 'x_axis_time_format',
- config: {
- ...sharedControls.x_axis_time_format,
- description: `${D3_TIME_FORMAT_DOCS}. ${TIME_SERIES_DESCRIPTION_TEXT}`,
- visibility: ({ controls }: ControlPanelsContainerProps) =>
- checkColumnType(
- getColumnLabel(controls?.x_axis?.value as QueryFormColumn),
- controls?.datasource?.datasource,
- [GenericDataType.Temporal],
- ),
- disableStash: true,
- resetOnHide: false,
- },
- },
- {
- name: 'x_axis_number_format',
+ name: 'minMarkerSize',
config: {
- ...sharedControls.x_axis_number_format,
- default: '~g',
- mapStateToProps: undefined,
- visibility: ({ controls }: ControlPanelsContainerProps) =>
- checkColumnType(
- getColumnLabel(controls?.x_axis?.value as QueryFormColumn),
- controls?.datasource?.datasource,
- [GenericDataType.Numeric],
- ),
- },
- },
- ],
- [xAxisLabelRotation],
- [xAxisLabelInterval],
- [forceMaxInterval],
- // eslint-disable-next-line react/jsx-key
- ...richTooltipSection,
- // eslint-disable-next-line react/jsx-key
- [<ControlSubSectionHeader>{t('Y Axis')}</ControlSubSectionHeader>],
- ['y_axis_format'],
- ['currency_format'],
- [
- {
- name: 'logAxis',
- config: {
- type: 'CheckboxControl',
- label: t('Logarithmic y-axis'),
- renderTrigger: true,
- default: logAxis,
- description: t('Logarithmic y-axis'),
- },
- },
- ],
- [
- {
- name: 'minorSplitLine',
- config: {
- type: 'CheckboxControl',
- label: t('Minor Split Line'),
- renderTrigger: true,
- default: minorSplitLine,
- description: t('Draw split lines for minor y-axis ticks'),
- },
- },
- ],
- [truncateXAxis],
- [xAxisBounds],
- [
- {
- name: 'truncateYAxis',
- config: {
- type: 'CheckboxControl',
- label: t('Truncate Y Axis'),
- default: truncateYAxis,
+ type: 'SliderControl',
+ label: t('Minimum dot size'),
renderTrigger: true,
+ min: 1,
+ max: 100,
+ default: minMarkerSize,
description: t(
- 'Truncate Y Axis. Can be overridden by specifying a min or max bound.',
+ 'Size of the dot representing the smallest value of the dot size metric.',
),
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ Boolean(controls?.size?.value),
},
},
- ],
- [
{
- name: 'y_axis_bounds',
+ name: 'maxMarkerSize',
config: {
- type: 'BoundsControl',
- label: t('Y Axis Bounds'),
+ type: 'SliderControl',
+ label: t('Maximum dot size'),
renderTrigger: true,
- default: yAxisBounds,
+ min: 1,
+ max: 100,
+ default: maxMarkerSize,
description: t(
- 'Bounds for the Y-axis. When left empty, the bounds are ' +
- 'dynamically defined based on the min/max of the data. Note that ' +
- "this feature will only expand the axis range. It won't " +
- "narrow the data's extent.",
+ 'Size of the dot representing the largest value of the dot size metric.',
),
visibility: ({ controls }: ControlPanelsContainerProps) =>
- Boolean(controls?.truncateYAxis?.value),
+ Boolean(controls?.size?.value),
},
},
],
+ ['zoomable'],
+ [minorTicks],
+ ...legendSection,
+ [<ControlSubSectionHeader>{t('X Axis')}</ControlSubSectionHeader>],
+ ...createAxisControl('x'),
+ [truncateXAxis],
+ [xAxisBounds],
+ [forceMaxInterval],
Review Comment:
**Suggestion:** These controls are always pinned under the X Axis subsection, so in horizontal orientation they no longer follow the dimension axis as intended and the panel swap behavior becomes inconsistent. Gate them by orientation (or move them into the same axis-swapping helper) so they appear under the correct subsection. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Axis truncation and bounds controls fixed under X Axis.
- ⚠️ Horizontal orientation shows dimension controls split across subsections.
- ⚠️ UX diverges from Bar chart axis orientation behavior.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx:361-379`,
the `orientation` radio control is defined, with values `OrientationType.Vertical` and
`OrientationType.Horizontal`. This orientation is later used by
`isHorizontal`/`isVertical` and `createAxisControl` to swap axis-specific controls.
2. At lines 162-320, `createAxisControl(axis: 'x' | 'y')` defines axis controls, using
`showsDimensionAxis` and `showsMetricAxis` so that many controls (time format, number
format, label rotation, logAxis, truncateYAxis, y_axis_bounds, etc.) appear under either
the X Axis or Y Axis subsection depending on `orientation`.
3. At lines 473-481, the Chart Options section composes the subsections: it adds the X
Axis header, then `...createAxisControl('x')`, then the rows `[truncateXAxis],
[xAxisBounds], [forceMaxInterval], ...richTooltipSection`, followed by the Y Axis header
and `...createAxisControl('y')`. The three rows for `truncateXAxis`, `xAxisBounds`, and
`forceMaxInterval` are statically placed under the X Axis subsection.
4. When the control panel is rendered and the user switches to horizontal orientation
(dimension on Y axis, metric on X axis as described in the PR), the dimension/metric axis
controls created inside `createAxisControl` swap between X and Y subsections, but
`truncateXAxis`, `xAxisBounds`, and `forceMaxInterval` remain under X Axis only, causing
these axis-bound controls to be grouped with the wrong subsection relative to the
dimension axis and making the orientation-dependent panel layout inconsistent.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0e2481be2bc34c419a4e0e369e4e3741&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=0e2481be2bc34c419a4e0e369e4e3741&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx
**Line:** 475:477
**Comment:**
*Logic Error: These controls are always pinned under the X Axis subsection, so in horizontal orientation they no longer follow the dimension axis as intended and the panel swap behavior becomes inconsistent. Gate them by orientation (or move them into the same axis-swapping helper) so they appear under the correct subsection.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=69050c4c4b6f3876b2c6a03da4b8964c7b12b1683fd87923f6e577366981d354&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=69050c4c4b6f3876b2c6a03da4b8964c7b12b1683fd87923f6e577366981d354&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx:
##########
@@ -51,18 +56,342 @@ const {
logAxis,
markerEnabled,
markerSize,
+ maxMarkerSize,
+ minMarkerSize,
minorSplitLine,
+ orientation,
rowLimit,
truncateYAxis,
yAxisBounds,
} = DEFAULT_FORM_DATA;
+
+const isHorizontal = (controls: ControlStateMapping) =>
+ controls?.orientation?.value === OrientationType.Horizontal;
+const isVertical = (controls: ControlStateMapping) => !isHorizontal(controls);
+
+function createAxisTitleControl(axis: 'x' | 'y'): ControlSetRow[] {
+ const isXAxis = axis === 'x';
+ return [
+ [
+ {
+ name: 'x_axis_title',
+ config: {
+ type: 'TextControl',
+ label: t('Axis Title'),
+ renderTrigger: true,
+ default: '',
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ isXAxis ? isVertical(controls) : isHorizontal(controls),
+ disableStash: true,
+ resetOnHide: false,
+ },
+ },
+ ],
+ [
+ {
+ name: 'x_axis_title_margin',
+ config: {
+ type: 'SelectControl',
+ freeForm: true,
+ clearable: true,
+ label: t('Axis title margin'),
+ renderTrigger: true,
+ default: sections.TITLE_MARGIN_OPTIONS[3],
+ choices: formatSelectOptions(sections.TITLE_MARGIN_OPTIONS),
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ isXAxis ? isVertical(controls) : isHorizontal(controls),
+ disableStash: true,
+ resetOnHide: false,
+ },
+ },
+ ],
+ [
+ {
+ name: 'y_axis_title',
+ config: {
+ type: 'TextControl',
+ label: t('Axis Title'),
+ renderTrigger: true,
+ default: '',
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ isXAxis ? isHorizontal(controls) : isVertical(controls),
+ disableStash: true,
+ resetOnHide: false,
+ },
+ },
+ ],
+ [
+ {
+ name: 'y_axis_title_margin',
+ config: {
+ type: 'SelectControl',
+ freeForm: true,
+ clearable: true,
+ label: t('Axis title margin'),
+ renderTrigger: true,
+ default: sections.TITLE_MARGIN_OPTIONS[4],
+ choices: formatSelectOptions(sections.TITLE_MARGIN_OPTIONS),
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ isXAxis ? isHorizontal(controls) : isVertical(controls),
+ disableStash: true,
+ resetOnHide: false,
+ },
+ },
+ ],
+ [
+ {
+ name: 'y_axis_title_position',
+ config: {
+ type: 'SelectControl',
+ freeForm: true,
+ clearable: false,
+ label: t('Axis title position'),
+ renderTrigger: true,
+ default: sections.TITLE_POSITION_OPTIONS[0][0],
+ choices: sections.TITLE_POSITION_OPTIONS,
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ isXAxis ? isHorizontal(controls) : isVertical(controls),
+ disableStash: true,
+ resetOnHide: false,
+ },
+ },
+ ],
+ ];
+}
+
+function createAxisControl(axis: 'x' | 'y'): ControlSetRow[] {
+ const isXAxis = axis === 'x';
+ // The dimension (x_axis) controls follow the dimension axis: they show
+ // under "X Axis" when vertical and under "Y Axis" when horizontal. The
+ // metric controls follow the opposite axis.
+ const showsDimensionAxis = (controls: ControlStateMapping) =>
+ isXAxis ? isVertical(controls) : isHorizontal(controls);
+ const showsMetricAxis = (controls: ControlStateMapping) =>
+ isXAxis ? isHorizontal(controls) : isVertical(controls);
+ return [
+ [
+ {
+ name: 'x_axis_time_format',
+ config: {
+ ...sharedControls.x_axis_time_format,
+ default: 'smart_date',
+ description: `${D3_TIME_FORMAT_DOCS}. ${TIME_SERIES_DESCRIPTION_TEXT}`,
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ showsDimensionAxis(controls) &&
+ checkColumnType(
+ getColumnLabel(controls?.x_axis?.value as QueryFormColumn),
+ controls?.datasource?.datasource,
+ [GenericDataType.Temporal],
+ ),
+ disableStash: true,
+ resetOnHide: false,
+ },
+ },
+ ],
+ [
+ {
+ name: 'x_axis_number_format',
+ config: {
+ ...sharedControls.x_axis_number_format,
+ default: '~g',
+ mapStateToProps: undefined,
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ showsDimensionAxis(controls) &&
+ checkColumnType(
+ getColumnLabel(controls?.x_axis?.value as QueryFormColumn),
+ controls?.datasource?.datasource,
+ [GenericDataType.Numeric],
+ ),
+ disableStash: true,
+ resetOnHide: false,
+ },
+ },
+ ],
+ [
+ {
+ name: xAxisLabelRotation.name,
+ config: {
+ ...xAxisLabelRotation.config,
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ showsDimensionAxis(controls),
+ disableStash: true,
+ resetOnHide: false,
+ },
+ },
+ ],
+ [
+ {
+ name: xAxisLabelInterval.name,
+ config: {
+ ...xAxisLabelInterval.config,
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ showsDimensionAxis(controls),
+ disableStash: true,
+ resetOnHide: false,
+ },
+ },
+ ],
+ [
+ {
+ name: 'y_axis_format',
+ config: {
+ ...sharedControls.y_axis_format,
+ label: t('Axis Format'),
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ showsMetricAxis(controls),
+ disableStash: true,
+ resetOnHide: false,
+ },
+ },
+ ],
+ ['currency_format'],
Review Comment:
**Suggestion:** The currency format control is added unconditionally inside a helper that is rendered for both axis sections, so the same control will appear twice and can be shown in the wrong section for a given orientation. Make this control orientation-aware (metric-axis only) or render it only once. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Scatter chart currency format control duplicated per axis section.
- ⚠️ Users may confuse which axis currency applies.
- ⚠️ Inconsistent with metric-axis-only currency format elsewhere.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx:162-320`,
note that `createAxisControl(axis: 'x' | 'y')` returns an array of `ControlSetRow`s used
for both X and Y axis subsections.
2. Inside `createAxisControl` at line 247, the row `['currency_format'],` is included
without any `visibility` logic tied to `showsMetricAxis` or `showsDimensionAxis`, unlike
surrounding metric/dimension axis controls.
3. Later in the same file at lines 473-480, the config section adds both subsections:
`[<ControlSubSectionHeader>{t('X Axis')}</ControlSubSectionHeader>],
...createAxisControl('x'), ...` and `[<ControlSubSectionHeader>{t('Y
Axis')}</ControlSubSectionHeader>], ...createAxisControl('y'),` meaning
`createAxisControl` (and thus the `currency_format` row) is rendered twice.
4. When the scatter chart plugin renders this control panel (via the chart-controls
framework that consumes `config` from this file), the `currency_format` control appears in
both X Axis and Y Axis sections, even though it conceptually belongs to the metric axis
only, leading to duplicated UI and ambiguous placement.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=edf05a10ab4240c68eb7c8fedf48309c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=edf05a10ab4240c68eb7c8fedf48309c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx
**Line:** 247:247
**Comment:**
*Logic Error: The currency format control is added unconditionally inside a helper that is rendered for both axis sections, so the same control will appear twice and can be shown in the wrong section for a given orientation. Make this control orientation-aware (metric-axis only) or render it only once.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=00c051b132c8fe85d6cd9576be71470c17dcfd21f73c3abf5601e55a6568bc97&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=00c051b132c8fe85d6cd9576be71470c17dcfd21f73c3abf5601e55a6568bc97&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Area/controlPanel.test.ts:
##########
@@ -16,58 +16,14 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { ControlPanelsContainerProps } from '@superset-ui/chart-controls/types';
import { GenericDataType } from '@apache-superset/core/common';
import controlPanel from '../../../src/Timeseries/Area/controlPanel';
+import { getControl, mockControls } from '../helpers';
Review Comment:
**Suggestion:** `mockControls` is imported but never used, which introduces dead code and can fail strict lint rules for unused imports. Remove the unused import or use it in assertions. [possible bug]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
- ⚠️ Test file triggers unused-import lint warnings or errors.
- ⚠️ CI pipeline may fail on style violations.
- ⚠️ Minor noise and clutter in test codebase.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open
`superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Area/controlPanel.test.ts`.
At line 21, observe the import `import { getControl, mockControls } from '../helpers';`.
2. At lines 25-27, `timeFormatControl` and `numberFormatControl` are initialized using
`getControl(config, 'x_axis_time_format')` and `getControl(config,
'x_axis_number_format')`, respectively; `mockControls` is not referenced anywhere else in
the file.
3. Search within the same file for `mockControls` usage; no matches are found, confirming
that the imported symbol is unused.
4. Running tooling that flags unused imports (e.g., ESLint or TypeScript with appropriate
rules configured in this repository) will report `mockControls` as an unused import in
this test file, causing lint warnings or failures in the standard dev/CI workflow.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d2624d92d6e84d6bb2795d71df6ceb07&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=d2624d92d6e84d6bb2795d71df6ceb07&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Area/controlPanel.test.ts
**Line:** 21:21
**Comment:**
*Possible Bug: `mockControls` is imported but never used, which introduces dead code and can fail strict lint rules for unused imports. Remove the unused import or use it in assertions.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=ca983054736a9e0c2d899703b655093a4149d5b563fff7e34d11498728c8c9c4&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=ca983054736a9e0c2d899703b655093a4149d5b563fff7e34d11498728c8c9c4&reaction=dislike'>👎</a>
--
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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #40967:
URL: https://github.com/apache/superset/pull/40967
--
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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #40967:
URL: https://github.com/apache/superset/pull/40967#issuecomment-5036260144
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 7f1c3f92b005794fe40cedb710d846e7d329de99 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a5f97e63c545b0008e0a6e5 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-40967--superset-docs-preview.netlify.app](https://deploy-preview-40967--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DctLDoAgDADRu3RtI4mCnxt4jApFSVQIdoPGu8tqZvNeSDmeSWAGaGDNdNm9vmeS9rYkwhlreIs5WDqwIF0OXRS8w8OVOE5HLIuryJD208Cms7rXq1JqZEWGNXw_) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-40967--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40967:
URL: https://github.com/apache/superset/pull/40967#discussion_r3624102442
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/controlPanel.test.ts:
##########
@@ -19,55 +19,12 @@
import { ControlPanelsContainerProps } from '@superset-ui/chart-controls/types';
import { GenericDataType } from '@apache-superset/core/common';
import controlPanel from '../../../src/Timeseries/Regular/Scatter/controlPanel';
+import { getControl, mockControls } from '../helpers';
Review Comment:
**Suggestion:** `mockControls` is imported but never used in this test file, which introduces dead code and may fail CI where unused imports are lint errors. Remove the unused import to keep the test clean and lint-safe. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Test file contains dead, unused import code.
- ⚠️ Potential lint failures depending on CI lint settings.
- ⚠️ Slightly increases maintenance noise in test suite.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open
`superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/controlPanel.test.ts`
and inspect imports at lines `19-22`, where `{ getControl, mockControls }` is imported
from `../helpers`.
2. Search within the same file (`controlPanel.test.ts:1-163`) for usages of
`mockControls`; only `getControl` is referenced for control lookups, while `mockControls`
is never used.
3. Because `mockControls` is unused, TypeScript tooling and common ESLint configurations
(e.g., `no-unused-vars` / `no-unused-imports`) will report this as an unused import when
running the test or lint tasks on this file.
4. In CI environments where such lint warnings are treated as errors, this unused import
can cause the lint step to fail, even though the tests themselves are otherwise correct.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=885b6c441ba049e7ba97ff39c7b8ccf3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=885b6c441ba049e7ba97ff39c7b8ccf3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/controlPanel.test.ts
**Line:** 22:22
**Comment:**
*Possible Bug: `mockControls` is imported but never used in this test file, which introduces dead code and may fail CI where unused imports are lint errors. Remove the unused import to keep the test clean and lint-safe.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=530049f654fae564543fea1b300a134f2786efb17d4852afd7842d084a9112d0&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=530049f654fae564543fea1b300a134f2786efb17d4852afd7842d084a9112d0&reaction=dislike'>👎</a>
--
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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #40967:
URL: https://github.com/apache/superset/pull/40967#issuecomment-5005950559
As noted on the CodeAnt threads above, dot-size-by-metric plus time comparison isn't something I meant to support here, the offset-named series would need their own handling and I scoped that out on purpose. Not blocking IMO.
--
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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40967:
URL: https://github.com/apache/superset/pull/40967#discussion_r3623729184
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/SmoothLine/controlPanel.test.ts:
##########
@@ -16,58 +16,14 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { ControlPanelsContainerProps } from '@superset-ui/chart-controls/types';
import { GenericDataType } from '@apache-superset/core/common';
import controlPanel from '../../../src/Timeseries/Regular/SmoothLine/controlPanel';
+import { getControl, mockControls } from '../helpers';
Review Comment:
**Suggestion:** `mockControls` is imported but never used, which will trip no-unused-vars/no-unused-imports lint checks in strict CI setups. Remove the unused import or add a test that uses it. [possible bug]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
- ⚠️ Lint step may warn about unused mockControls import.
- ⚠️ Minor dead code; no runtime behavior affected.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open
`superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/SmoothLine/controlPanel.test.ts`
and observe the import at line 21: `import { getControl, mockControls } from
'../helpers';`.
2. In the same file, inspect the usages at lines 25–34 where `getControl` is called to
build `timeFormatControl` and `numberFormatControl`; `mockControls` is never referenced.
3. Run the frontend lint task (e.g. `npm run lint` or `yarn lint`), which executes ESLint
over test files including `SmoothLine/controlPanel.test.ts`.
4. ESLint’s `no-unused-vars`/`no-unused-imports` rules (commonly enabled in this
repository) will flag `mockControls` at line 21 as an unused import, causing the lint step
(and CI that depends on it) to fail.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e1e127315a1740ceaa02f380e27d1baf&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=e1e127315a1740ceaa02f380e27d1baf&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/SmoothLine/controlPanel.test.ts
**Line:** 21:21
**Comment:**
*Possible Bug: `mockControls` is imported but never used, which will trip no-unused-vars/no-unused-imports lint checks in strict CI setups. Remove the unused import or add a test that uses it.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=9c9bb7d8e7371782b78305a5bc7c0a7b74e5e407ba76ef37e830ceca998bcfe6&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=9c9bb7d8e7371782b78305a5bc7c0a7b74e5e407ba76ef37e830ceca998bcfe6&reaction=dislike'>👎</a>
--
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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40967:
URL: https://github.com/apache/superset/pull/40967#discussion_r3625659325
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx:
##########
@@ -99,114 +428,56 @@ const config: ControlPanelConfig = {
'Size of marker. Also applies to forecast observations.',
),
visibility: ({ controls }: ControlPanelsContainerProps) =>
- Boolean(controls?.markerEnabled?.value),
+ Boolean(controls?.markerEnabled?.value) &&
+ !controls?.size?.value,
},
},
],
- ['zoomable'],
- [minorTicks],
- ...legendSection,
- [<ControlSubSectionHeader>{t('X Axis')}</ControlSubSectionHeader>],
-
[
{
- name: 'x_axis_time_format',
- config: {
- ...sharedControls.x_axis_time_format,
- description: `${D3_TIME_FORMAT_DOCS}. ${TIME_SERIES_DESCRIPTION_TEXT}`,
- visibility: ({ controls }: ControlPanelsContainerProps) =>
- checkColumnType(
- getColumnLabel(controls?.x_axis?.value as QueryFormColumn),
- controls?.datasource?.datasource,
- [GenericDataType.Temporal],
- ),
- disableStash: true,
- resetOnHide: false,
- },
- },
- {
- name: 'x_axis_number_format',
+ name: 'minMarkerSize',
config: {
- ...sharedControls.x_axis_number_format,
- default: '~g',
- mapStateToProps: undefined,
- visibility: ({ controls }: ControlPanelsContainerProps) =>
- checkColumnType(
- getColumnLabel(controls?.x_axis?.value as QueryFormColumn),
- controls?.datasource?.datasource,
- [GenericDataType.Numeric],
- ),
- },
- },
- ],
- [xAxisLabelRotation],
- [xAxisLabelInterval],
- [forceMaxInterval],
- // eslint-disable-next-line react/jsx-key
- ...richTooltipSection,
- // eslint-disable-next-line react/jsx-key
- [<ControlSubSectionHeader>{t('Y Axis')}</ControlSubSectionHeader>],
- ['y_axis_format'],
- ['currency_format'],
- [
- {
- name: 'logAxis',
- config: {
- type: 'CheckboxControl',
- label: t('Logarithmic y-axis'),
- renderTrigger: true,
- default: logAxis,
- description: t('Logarithmic y-axis'),
- },
- },
- ],
- [
- {
- name: 'minorSplitLine',
- config: {
- type: 'CheckboxControl',
- label: t('Minor Split Line'),
- renderTrigger: true,
- default: minorSplitLine,
- description: t('Draw split lines for minor y-axis ticks'),
- },
- },
- ],
- [truncateXAxis],
- [xAxisBounds],
- [
- {
- name: 'truncateYAxis',
- config: {
- type: 'CheckboxControl',
- label: t('Truncate Y Axis'),
- default: truncateYAxis,
+ type: 'SliderControl',
+ label: t('Minimum dot size'),
renderTrigger: true,
+ min: 1,
+ max: 100,
+ default: minMarkerSize,
description: t(
- 'Truncate Y Axis. Can be overridden by specifying a min or max bound.',
+ 'Size of the dot representing the smallest value of the dot size metric.',
),
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ Boolean(controls?.size?.value),
},
},
- ],
- [
{
- name: 'y_axis_bounds',
+ name: 'maxMarkerSize',
config: {
- type: 'BoundsControl',
- label: t('Y Axis Bounds'),
+ type: 'SliderControl',
+ label: t('Maximum dot size'),
renderTrigger: true,
- default: yAxisBounds,
+ min: 1,
+ max: 100,
+ default: maxMarkerSize,
description: t(
- 'Bounds for the Y-axis. When left empty, the bounds are ' +
- 'dynamically defined based on the min/max of the data. Note that ' +
- "this feature will only expand the axis range. It won't " +
- "narrow the data's extent.",
+ 'Size of the dot representing the largest value of the dot size metric.',
),
visibility: ({ controls }: ControlPanelsContainerProps) =>
- Boolean(controls?.truncateYAxis?.value),
+ Boolean(controls?.size?.value),
},
},
],
+ ['zoomable'],
+ [minorTicks],
+ ...legendSection,
+ [<ControlSubSectionHeader>{t('X Axis')}</ControlSubSectionHeader>],
+ ...createAxisControl('x'),
+ [truncateXAxis],
+ [xAxisBounds],
+ [forceMaxInterval],
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not flag dimension-column-specific controls that are pinned under the X Axis header in scatter control panels; this placement is intentional because they follow the underlying dimension column rather than the physical rendered axis, matching the existing bar chart convention.
**Applied to:**
- `superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/**`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #40967:
URL: https://github.com/apache/superset/pull/40967#discussion_r3625658246
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Area/controlPanel.test.ts:
##########
@@ -16,58 +16,14 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { ControlPanelsContainerProps } from '@superset-ui/chart-controls/types';
import { GenericDataType } from '@apache-superset/core/common';
import controlPanel from '../../../src/Timeseries/Area/controlPanel';
+import { getControl, mockControls } from '../helpers';
Review Comment:
Same as the SmoothLine thread, `mockControls` is used a few lines below in this file. Not an unused import.
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/controlPanel.test.ts:
##########
@@ -19,55 +19,12 @@
import { ControlPanelsContainerProps } from '@superset-ui/chart-controls/types';
import { GenericDataType } from '@apache-superset/core/common';
import controlPanel from '../../../src/Timeseries/Regular/Scatter/controlPanel';
+import { getControl, mockControls } from '../helpers';
Review Comment:
`mockControls` is used here too, e.g. `mockControls(xAxisColumn, xAxisType)`. Not an unused import.
--
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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #40967:
URL: https://github.com/apache/superset/pull/40967#issuecomment-5125004031
@kgabryje all fixed up if you can re-review, please.
--
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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40967:
URL: https://github.com/apache/superset/pull/40967#discussion_r3394640685
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/buildQuery.test.ts:
##########
@@ -46,6 +46,24 @@ describe('Timeseries buildQuery', () => {
expect(query.orderby).toEqual([['bar', false]]);
});
+ test('should include the scatter dot size metric in query metrics', () => {
+ const queryContext = buildQuery({
+ ...formData,
+ size: 'qux',
+ });
+ const [query] = queryContext.queries;
+ expect(query.metrics).toEqual(['bar', 'baz', 'qux']);
+ });
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion is based on the observation that `buildQuery.ts` does not explicitly handle the `size` property. However, if `extractQueryFields` in the core logic correctly aliases `size` to `metrics` before `buildQueryObject` is called, the test will indeed pass as expected. You may consider marking this suggestion as resolved if the data flow is handled by the core utility as you described.
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/buildQuery.test.ts:
##########
@@ -46,6 +46,24 @@ describe('Timeseries buildQuery', () => {
expect(query.orderby).toEqual([['bar', false]]);
});
+ test('should include the scatter dot size metric in query metrics', () => {
+ const queryContext = buildQuery({
+ ...formData,
+ size: 'qux',
+ });
+ const [query] = queryContext.queries;
+ expect(query.metrics).toEqual(['bar', 'baz', 'qux']);
+ });
+
+ test('should dedupe the dot size metric when it is also a value metric', () => {
+ const queryContext = buildQuery({
+ ...formData,
+ size: 'bar',
+ });
+ const [query] = queryContext.queries;
+ expect(query.metrics).toEqual(['bar', 'baz']);
+ });
Review Comment:
<!-- Bito Reply -->
The reviewer's concern regarding the deduplication logic is noted. Since the implementation relies on `extractQueryFields` and `removeDuplicates` to handle the metric mapping and deduplication, the test correctly validates the expected behavior of the `buildQuery` function without requiring additional changes in `buildQuery.ts`.
**superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/buildQuery.test.ts**
```
test('should dedupe the dot size metric when it is also a value metric', () => {
const queryContext = buildQuery({
...formData,
size: 'bar',
});
const [query] = queryContext.queries;
expect(query.metrics).toEqual(['bar', 'baz']);
});
```
--
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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40967:
URL: https://github.com/apache/superset/pull/40967#discussion_r3623725743
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -321,7 +326,7 @@ export default function transformProps(
seriesType,
);
- const [rawSeries, sortedTotalValues, minPositiveValue] = extractSeries(
+ const [allRawSeries, sortedTotalValues, minPositiveValue] = extractSeries(
Review Comment:
**Suggestion:** `sortedTotalValues` and `minPositiveValue` are still computed from `allRawSeries`, which now includes the hidden size-metric series; this can incorrectly inflate axis bounds/log tick calculations using data that is not rendered. Recompute these aggregates from the filtered render series (or exclude size series before extracting totals) so only visible value series affect axis scaling. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Scatter value axis scaled by hidden size metric.
- ⚠️ Log-axis lower tick misaligned with visible data.
- ⚠️ Auto axis bounds distorted for sized scatter charts.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In Superset, create a chart using the ECharts Time-series Scatter plugin, which calls
`transformProps` in
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:220` as
part of the chart’s `transformProps` pipeline.
2. Configure the chart with `seriesType = EchartsTimeseriesSeriesType.Scatter` and set
both a value metric in `metrics` and a distinct dot size metric in `size` (form data
consumed at `transformProps` destructuring around lines 220–247).
3. Ensure the size metric has a different scale than the value metric and enable axis
features that rely on `sortedTotalValues` / `minPositiveValue` (e.g. `logAxis` and
automatic bounds, imported at lines 74–77 via `calculateLowerLogTick` and related helpers
in `../utils/series`).
4. When `transformProps` runs, `extractSeries` at line 329 produces `allRawSeries`,
`sortedTotalValues`, and `minPositiveValue` using **all** series (including the
size-metric-only series); later, size series are filtered out into `rawSeries` at lines
398–400, but `sortedTotalValues` and `minPositiveValue` still reflect hidden size-series
data, causing axis scaling and log tick computation to be driven partly by non-rendered
size values rather than visible value-series points.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c36f044115e5407e8927725e3b8cc3f1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=c36f044115e5407e8927725e3b8cc3f1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
**Line:** 329:329
**Comment:**
*Logic Error: `sortedTotalValues` and `minPositiveValue` are still computed from `allRawSeries`, which now includes the hidden size-metric series; this can incorrectly inflate axis bounds/log tick calculations using data that is not rendered. Recompute these aggregates from the filtered render series (or exclude size series before extracting totals) so only visible value series affect axis scaling.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=3f14b0cc3f1385a77f1e87fef7900b98c0f24469553bfdccfdcd5c1b585b1bd6&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=3f14b0cc3f1385a77f1e87fef7900b98c0f24469553bfdccfdcd5c1b585b1bd6&reaction=dislike'>👎</a>
--
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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #40967:
URL: https://github.com/apache/superset/pull/40967#discussion_r3625655166
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -321,7 +326,7 @@ export default function transformProps(
seriesType,
);
- const [rawSeries, sortedTotalValues, minPositiveValue] = extractSeries(
+ const [allRawSeries, sortedTotalValues, minPositiveValue] = extractSeries(
Review Comment:
Confirmed: `minPositiveValue` was still computed from `allRawSeries` before the size-metric series get filtered out, so log-axis bounds could be pulled from a hidden series with a different value range. Recomputed it from the rendered `rawSeries` instead. `sortedTotalValues` is fine as-is, it only feeds `shouldCalculateDataBounds`, which is Bar-only.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx:
##########
@@ -51,18 +56,342 @@ const {
logAxis,
markerEnabled,
markerSize,
+ maxMarkerSize,
+ minMarkerSize,
minorSplitLine,
+ orientation,
rowLimit,
truncateYAxis,
yAxisBounds,
} = DEFAULT_FORM_DATA;
+
+const isHorizontal = (controls: ControlStateMapping) =>
+ controls?.orientation?.value === OrientationType.Horizontal;
+const isVertical = (controls: ControlStateMapping) => !isHorizontal(controls);
+
+function createAxisTitleControl(axis: 'x' | 'y'): ControlSetRow[] {
+ const isXAxis = axis === 'x';
+ return [
+ [
+ {
+ name: 'x_axis_title',
+ config: {
+ type: 'TextControl',
+ label: t('Axis Title'),
+ renderTrigger: true,
+ default: '',
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ isXAxis ? isVertical(controls) : isHorizontal(controls),
+ disableStash: true,
+ resetOnHide: false,
+ },
+ },
+ ],
+ [
+ {
+ name: 'x_axis_title_margin',
+ config: {
+ type: 'SelectControl',
+ freeForm: true,
+ clearable: true,
+ label: t('Axis title margin'),
+ renderTrigger: true,
+ default: sections.TITLE_MARGIN_OPTIONS[3],
+ choices: formatSelectOptions(sections.TITLE_MARGIN_OPTIONS),
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ isXAxis ? isVertical(controls) : isHorizontal(controls),
+ disableStash: true,
+ resetOnHide: false,
+ },
+ },
+ ],
+ [
+ {
+ name: 'y_axis_title',
+ config: {
+ type: 'TextControl',
+ label: t('Axis Title'),
+ renderTrigger: true,
+ default: '',
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ isXAxis ? isHorizontal(controls) : isVertical(controls),
+ disableStash: true,
+ resetOnHide: false,
+ },
+ },
+ ],
+ [
+ {
+ name: 'y_axis_title_margin',
+ config: {
+ type: 'SelectControl',
+ freeForm: true,
+ clearable: true,
+ label: t('Axis title margin'),
+ renderTrigger: true,
+ default: sections.TITLE_MARGIN_OPTIONS[4],
+ choices: formatSelectOptions(sections.TITLE_MARGIN_OPTIONS),
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ isXAxis ? isHorizontal(controls) : isVertical(controls),
+ disableStash: true,
+ resetOnHide: false,
+ },
+ },
+ ],
+ [
+ {
+ name: 'y_axis_title_position',
+ config: {
+ type: 'SelectControl',
+ freeForm: true,
+ clearable: false,
+ label: t('Axis title position'),
+ renderTrigger: true,
+ default: sections.TITLE_POSITION_OPTIONS[0][0],
+ choices: sections.TITLE_POSITION_OPTIONS,
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ isXAxis ? isHorizontal(controls) : isVertical(controls),
+ disableStash: true,
+ resetOnHide: false,
+ },
+ },
+ ],
+ ];
+}
+
+function createAxisControl(axis: 'x' | 'y'): ControlSetRow[] {
+ const isXAxis = axis === 'x';
+ // The dimension (x_axis) controls follow the dimension axis: they show
+ // under "X Axis" when vertical and under "Y Axis" when horizontal. The
+ // metric controls follow the opposite axis.
+ const showsDimensionAxis = (controls: ControlStateMapping) =>
+ isXAxis ? isVertical(controls) : isHorizontal(controls);
+ const showsMetricAxis = (controls: ControlStateMapping) =>
+ isXAxis ? isHorizontal(controls) : isVertical(controls);
+ return [
+ [
+ {
+ name: 'x_axis_time_format',
+ config: {
+ ...sharedControls.x_axis_time_format,
+ default: 'smart_date',
+ description: `${D3_TIME_FORMAT_DOCS}. ${TIME_SERIES_DESCRIPTION_TEXT}`,
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ showsDimensionAxis(controls) &&
+ checkColumnType(
+ getColumnLabel(controls?.x_axis?.value as QueryFormColumn),
+ controls?.datasource?.datasource,
+ [GenericDataType.Temporal],
+ ),
+ disableStash: true,
+ resetOnHide: false,
+ },
+ },
+ ],
+ [
+ {
+ name: 'x_axis_number_format',
+ config: {
+ ...sharedControls.x_axis_number_format,
+ default: '~g',
+ mapStateToProps: undefined,
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ showsDimensionAxis(controls) &&
+ checkColumnType(
+ getColumnLabel(controls?.x_axis?.value as QueryFormColumn),
+ controls?.datasource?.datasource,
+ [GenericDataType.Numeric],
+ ),
+ disableStash: true,
+ resetOnHide: false,
+ },
+ },
+ ],
+ [
+ {
+ name: xAxisLabelRotation.name,
+ config: {
+ ...xAxisLabelRotation.config,
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ showsDimensionAxis(controls),
+ disableStash: true,
+ resetOnHide: false,
+ },
+ },
+ ],
+ [
+ {
+ name: xAxisLabelInterval.name,
+ config: {
+ ...xAxisLabelInterval.config,
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ showsDimensionAxis(controls),
+ disableStash: true,
+ resetOnHide: false,
+ },
+ },
+ ],
+ [
+ {
+ name: 'y_axis_format',
+ config: {
+ ...sharedControls.y_axis_format,
+ label: t('Axis Format'),
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ showsMetricAxis(controls),
+ disableStash: true,
+ resetOnHide: false,
+ },
+ },
+ ],
+ ['currency_format'],
Review Comment:
Good catch, fixed. `currency_format` now has the same `showsMetricAxis` visibility gate as `y_axis_format`/`logAxis` instead of rendering unconditionally under both sections.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx:
##########
@@ -99,114 +428,56 @@ const config: ControlPanelConfig = {
'Size of marker. Also applies to forecast observations.',
),
visibility: ({ controls }: ControlPanelsContainerProps) =>
- Boolean(controls?.markerEnabled?.value),
+ Boolean(controls?.markerEnabled?.value) &&
+ !controls?.size?.value,
},
},
],
- ['zoomable'],
- [minorTicks],
- ...legendSection,
- [<ControlSubSectionHeader>{t('X Axis')}</ControlSubSectionHeader>],
-
[
{
- name: 'x_axis_time_format',
- config: {
- ...sharedControls.x_axis_time_format,
- description: `${D3_TIME_FORMAT_DOCS}. ${TIME_SERIES_DESCRIPTION_TEXT}`,
- visibility: ({ controls }: ControlPanelsContainerProps) =>
- checkColumnType(
- getColumnLabel(controls?.x_axis?.value as QueryFormColumn),
- controls?.datasource?.datasource,
- [GenericDataType.Temporal],
- ),
- disableStash: true,
- resetOnHide: false,
- },
- },
- {
- name: 'x_axis_number_format',
+ name: 'minMarkerSize',
config: {
- ...sharedControls.x_axis_number_format,
- default: '~g',
- mapStateToProps: undefined,
- visibility: ({ controls }: ControlPanelsContainerProps) =>
- checkColumnType(
- getColumnLabel(controls?.x_axis?.value as QueryFormColumn),
- controls?.datasource?.datasource,
- [GenericDataType.Numeric],
- ),
- },
- },
- ],
- [xAxisLabelRotation],
- [xAxisLabelInterval],
- [forceMaxInterval],
- // eslint-disable-next-line react/jsx-key
- ...richTooltipSection,
- // eslint-disable-next-line react/jsx-key
- [<ControlSubSectionHeader>{t('Y Axis')}</ControlSubSectionHeader>],
- ['y_axis_format'],
- ['currency_format'],
- [
- {
- name: 'logAxis',
- config: {
- type: 'CheckboxControl',
- label: t('Logarithmic y-axis'),
- renderTrigger: true,
- default: logAxis,
- description: t('Logarithmic y-axis'),
- },
- },
- ],
- [
- {
- name: 'minorSplitLine',
- config: {
- type: 'CheckboxControl',
- label: t('Minor Split Line'),
- renderTrigger: true,
- default: minorSplitLine,
- description: t('Draw split lines for minor y-axis ticks'),
- },
- },
- ],
- [truncateXAxis],
- [xAxisBounds],
- [
- {
- name: 'truncateYAxis',
- config: {
- type: 'CheckboxControl',
- label: t('Truncate Y Axis'),
- default: truncateYAxis,
+ type: 'SliderControl',
+ label: t('Minimum dot size'),
renderTrigger: true,
+ min: 1,
+ max: 100,
+ default: minMarkerSize,
description: t(
- 'Truncate Y Axis. Can be overridden by specifying a min or max bound.',
+ 'Size of the dot representing the smallest value of the dot size metric.',
),
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ Boolean(controls?.size?.value),
},
},
- ],
- [
{
- name: 'y_axis_bounds',
+ name: 'maxMarkerSize',
config: {
- type: 'BoundsControl',
- label: t('Y Axis Bounds'),
+ type: 'SliderControl',
+ label: t('Maximum dot size'),
renderTrigger: true,
- default: yAxisBounds,
+ min: 1,
+ max: 100,
+ default: maxMarkerSize,
description: t(
- 'Bounds for the Y-axis. When left empty, the bounds are ' +
- 'dynamically defined based on the min/max of the data. Note that ' +
- "this feature will only expand the axis range. It won't " +
- "narrow the data's extent.",
+ 'Size of the dot representing the largest value of the dot size metric.',
),
visibility: ({ controls }: ControlPanelsContainerProps) =>
- Boolean(controls?.truncateYAxis?.value),
+ Boolean(controls?.size?.value),
},
},
],
+ ['zoomable'],
+ [minorTicks],
+ ...legendSection,
+ [<ControlSubSectionHeader>{t('X Axis')}</ControlSubSectionHeader>],
+ ...createAxisControl('x'),
+ [truncateXAxis],
+ [xAxisBounds],
+ [forceMaxInterval],
Review Comment:
Those are keyed off / bounds specifically, they're about the underlying dimension column regardless of which physical axis it renders on, not the visual X-position. Bar's control panel pins them under the X Axis header the same way (Timeseries/Regular/Bar/controlPanel.tsx), so this matches the existing convention rather than a scatter-specific bug.
--
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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40967:
URL: https://github.com/apache/superset/pull/40967#issuecomment-5040289835
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40967:
URL: https://github.com/apache/superset/pull/40967#discussion_r3394221279
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -337,6 +342,89 @@ export default function transformProps(
xAxisType,
},
);
+
+ // Dot size by metric (scatter): the size metric's series are excluded from
+ // rendering and instead provide per-point values that scale each marker's
+ // area between minMarkerSize and maxMarkerSize.
+ const sizeMetricLabel =
+ seriesType === EchartsTimeseriesSeriesType.Scatter && size
+ ? getMetricLabel(size)
+ : undefined;
+ const sizeSeriesLabel = isDefined(sizeMetricLabel)
+ ? (verboseMap[sizeMetricLabel!] ?? sizeMetricLabel)
+ : undefined;
+ const valueMetricLabels = ensureIsArray(metrics)
+ .map(getMetricLabel)
+ .map(label => verboseMap[label] ?? label);
+ // When the size metric is also a value metric, the query dedupes them into a
+ // single column, so each point's own value doubles as its size value.
+ const sizeIsValueMetric = isDefined(sizeSeriesLabel)
+ ? valueMetricLabels.includes(sizeSeriesLabel!)
+ : false;
+ const isSizeSeries = (name: string) =>
+ isDefined(sizeSeriesLabel) &&
+ !sizeIsValueMetric &&
+ (name === sizeSeriesLabel || name.startsWith(`${sizeSeriesLabel}, `));
+ const rawSeries = sizeSeriesLabel
+ ? allRawSeries.filter(entry => !isSizeSeries(String(entry.name ?? '')))
+ : allRawSeries;
Review Comment:
**🟠 Architect Review — HIGH**
Size-series detection only matches `<metric>` or `<metric>, <group>` names, so time-comparison size series like `<metric>__1 year ago` and `<metric>__1 year ago, <group>` are not treated as size-only series and leak into `rawSeries`, causing size metrics for time offsets to render as normal scatter series and appear in the legend instead of being hidden.
**Suggestion:** Extend `isSizeSeries` (and the related size-series handling) to recognize the same time-comparison naming patterns used elsewhere (e.g. `<metric>__<offset>`, `<metric>__<offset>, <group>`, and `<offset>, <group>`), and use that expanded detection both when filtering non-rendered size series out of `rawSeries` and when building `sizeLookups` so all size-only series—including time-comparison variants—are excluded from rendering.
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6766ee0cd5dd43738ddabc3805379f49&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=6766ee0cd5dd43738ddabc3805379f49&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
**Line:** 364:370
**Comment:**
*HIGH: Size-series detection only matches `<metric>` or `<metric>, <group>` names, so time-comparison size series like `<metric>__1 year ago` and `<metric>__1 year ago, <group>` are not treated as size-only series and leak into `rawSeries`, causing size metrics for time offsets to render as normal scatter series and appear in the legend instead of being hidden.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40967:
URL: https://github.com/apache/superset/pull/40967#issuecomment-4688420819
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40967:
URL: https://github.com/apache/superset/pull/40967#discussion_r3401956526
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts:
##########
@@ -125,6 +125,35 @@ describe('transformSeries', () => {
// OpacityEnum.NonTransparent = 1 (not dimmed)
expect((result as any).itemStyle.opacity).toBe(1);
});
+
+ test('should use symbolSizeFn for symbolSize when provided', () => {
+ const symbolSizeFn = jest.fn(
+ (value: (number | string | null)[]) => Number(value[1]) * 2,
+ );
+ const opts = {
+ seriesType: EchartsTimeseriesSeriesType.Scatter,
+ markerSize: 7,
+ symbolSizeFn,
+ timeShiftColor: false,
+ };
+
+ const result = transformSeries(series, mockColorScale, 'test-key', opts);
+
+ expect((result as any).symbolSize).toBe(symbolSizeFn);
+ expect((result as any).symbolSize(['A', 4])).toBe(8);
+ });
+
+ test('should fall back to markerSize for symbolSize when symbolSizeFn is not provided', () => {
+ const opts = {
+ seriesType: EchartsTimeseriesSeriesType.Scatter,
+ markerSize: 7,
+ timeShiftColor: false,
+ };
+
+ const result = transformSeries(series, mockColorScale, 'test-key', opts);
+
+ expect((result as any).symbolSize).toBe(7);
Review Comment:
**Suggestion:** Remove this `any` cast and assert through a concrete type or narrowed generic for the series object so the fallback `symbolSize` check remains type-safe. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This line uses an `any` cast in newly added TypeScript test code, which is directly covered by the custom rule against `any` in modified TS/TSX code.
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ed8ed9fcf9924e25b1921263ae1526ff&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=ed8ed9fcf9924e25b1921263ae1526ff&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts
**Line:** 155:155
**Comment:**
*Custom Rule: Remove this `any` cast and assert through a concrete type or narrowed generic for the series object so the fallback `symbolSize` check remains type-safe.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=038d1b379d8f3d05369fdb295d1c6ef52deb3d9102b1404ed8b07ef159fef4e7&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=038d1b379d8f3d05369fdb295d1c6ef52deb3d9102b1404ed8b07ef159fef4e7&reaction=dislike'>👎</a>
--
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(scatter): add chart orientation and dot size metric controls [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40967:
URL: https://github.com/apache/superset/pull/40967#discussion_r3401956519
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts:
##########
@@ -125,6 +125,35 @@ describe('transformSeries', () => {
// OpacityEnum.NonTransparent = 1 (not dimmed)
expect((result as any).itemStyle.opacity).toBe(1);
});
+
+ test('should use symbolSizeFn for symbolSize when provided', () => {
+ const symbolSizeFn = jest.fn(
+ (value: (number | string | null)[]) => Number(value[1]) * 2,
+ );
+ const opts = {
+ seriesType: EchartsTimeseriesSeriesType.Scatter,
+ markerSize: 7,
+ symbolSizeFn,
+ timeShiftColor: false,
+ };
+
+ const result = transformSeries(series, mockColorScale, 'test-key', opts);
+
+ expect((result as any).symbolSize).toBe(symbolSizeFn);
+ expect((result as any).symbolSize(['A', 4])).toBe(8);
Review Comment:
**Suggestion:** Replace the `any` cast in these assertions with a concrete/narrower type for the `transformSeries` result (or a typed helper) so `symbolSize` is validated without untyped access. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is TypeScript test code, and the new assertions explicitly cast `result` to `any` to access `symbolSize`. That matches the custom rule forbidding `any` in new or modified TypeScript/TSX code.
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d5bf790f7623408c91187b6d1d9d2982&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=d5bf790f7623408c91187b6d1d9d2982&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts
**Line:** 142:143
**Comment:**
*Custom Rule: Replace the `any` cast in these assertions with a concrete/narrower type for the `transformSeries` result (or a typed helper) so `symbolSize` is validated without untyped access.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=5facd16852d8f79a705d565c2e9104343f41f90c9a26e50789b6fed584bd2b93&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=5facd16852d8f79a705d565c2e9104343f41f90c9a26e50789b6fed584bd2b93&reaction=dislike'>👎</a>
--
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