You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "YousufFFFF (via GitHub)" <gi...@apache.org> on 2026/02/05 20:21:42 UTC
[PR] Fix Bar Chart X-Axis Sort By behavior when Dimensions are set [superset]
YousufFFFF opened a new pull request, #37712:
URL: https://github.com/apache/superset/pull/37712
…it tests. The sorting options now consistently include axis and metric choices regardless of dimension presence, and multi-series sort options are conditionally added. New tests ensure expected behavior for various control states.
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
## Summary
This PR fixes inconsistent behavior in the Bar chart **X-Axis Sort By** control when **Dimensions (groupby)** are set.
Currently, when a dimension is added, the available sort options become restricted and prevent users from sorting the categorical axis by another numeric column (e.g. sorting `DayName` by `Day_Number`). This differs from the behavior when no dimension is set and leads to confusing, inconsistent UX.
This change aligns the sort behavior so users can explicitly control axis ordering in a predictable way, without altering existing defaults.
---
## Problem
When **Dimensions** are configured on a Bar chart:
- The **X-Axis Sort By** dropdown is limited to a small fixed set of options.
- Users cannot sort a categorical axis using a different numeric column.
- This is surprising in common BI use cases (e.g. days of week, ranked categories).
The behavior differs depending on whether a dimension is set, even though the underlying use case remains valid.
Tracked in: #34352
---
## Solution
- Updated the Bar chart control logic to make **X-Axis Sort By** behave consistently when Dimensions are present.
- Ensured valid numeric sort options remain available without breaking existing defaults.
- No changes were made to query semantics unless the user explicitly selects a different sort option.
---
## Scope
- Applies to **ECharts Bar Chart**
- Frontend-only change
- No breaking changes
- No behavior change unless the user opts into a different sort option
---
## Tests
- Added unit tests covering **X-Axis Sort By behavior when Dimensions are set**
- Tests verify that valid numeric sort options remain available and selectable
- Existing behavior without Dimensions is unchanged
---
## After Video:
https://github.com/user-attachments/assets/a9c8c28d-293e-46fc-a760-97e1a8e767b7
--
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] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "YousufFFFF (via GitHub)" <gi...@apache.org>.
YousufFFFF commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3936260501
@sfirke - Thank you for the detailed review, this clearly highlights the inconsistency in how the X-Axis sorting behaves when a dimension is added.
I’ll investigate why it’s currently falling back to metric-based sorting instead of respecting the value specified in X-Axis Sort By, and update the implementation accordingly.
I’ll push the changes once I’ve verified the fix in the dimension + stacked scenario.
--
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] fix(bar chart): X-Axis Sort By behavior when Dimensions are set [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3873287485
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/21835904525) for [c534b79](https://github.com/apache/superset/commit/c534b79)
• **Environment:** http://18.236.196.108:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-5649372676
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 an incremental AI Review.
- `/review full` - 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] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3935282429
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/22228256649) for [240c741](https://github.com/apache/superset/commit/240c741)
--
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] Fix Bar Chart X-Axis Sort By behavior when Dimensions are set [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3856006250
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | d91ef366f32b9416fe303283c36dd0ae1e721135 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6984fbd68d6af6000877e03a |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-37712--superset-docs-preview.netlify.app](https://deploy-preview-37712--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> |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-37712--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] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "YousufFFFF (via GitHub)" <gi...@apache.org>.
YousufFFFF commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3899437761
Hi @sfirke! The X-Axis Sort By functionality has been implemented and is functional. The implementation includes:
1. Updated `xAxisSortControl` in `customControls.tsx` to handle dimension-based sorting
2. Modified bar chart control panel to support the new sorting options
3. Added comprehensive unit tests for the new functionality
The CI failures are related to pre-existing build issues in the repository (missing `@react-icons/all-files` dependencies) that also occur on the master branch - these are not caused by this PR. All pre-commit checks pass locally.
Please let me know if there are specific aspects of the implementation you'd like me to adjust!
--
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] fix(bar chart): X-Axis Sort By behavior when Dimensions are set [superset]
Posted by "YousufFFFF (via GitHub)" <gi...@apache.org>.
YousufFFFF commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3873755649
Sounds good! I’ll extend this PR to also map metric-based X-axis sorting correctly in the ECharts multi-series path so the sorting actually takes effect when dimensions are present.
I’ll update the implementation and tests accordingly and follow up here once it’s ready for review. Thanks!
--
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] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3904747276
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/22038580723) for [240c741](https://github.com/apache/superset/commit/240c741)
• **Environment:** http://54.200.40.113:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(bar chart): X-Axis Sort By behavior when Dimensions are set [superset]
Posted by "YousufFFFF (via GitHub)" <gi...@apache.org>.
YousufFFFF commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3873631047
You’re absolutely right @sfirke! Thanks for validating this.
I tested the same steps as you asked me to below is the video:
https://github.com/user-attachments/assets/6955d174-c01f-4dcb-91ce-8ae691bec0e5
This PR currently fixes the control-panel behavior only (the X-Axis Sort By dropdown no longer gets restricted when a dimension is added).
The actual sorting behavior in multi-series mode is not fixed yet, which is why the chart still sorts alphabetically after adding a dimension.
--
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] fix(bar chart): X-Axis Sort By behavior when Dimensions are set [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3873185983
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/21835904525) for [c534b79](https://github.com/apache/superset/commit/c534b79)
--
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] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3882836941
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] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "YousufFFFF (via GitHub)" <gi...@apache.org>.
YousufFFFF commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3882724585
@sfirke I've pushed the requested changes.
I’ve also updated the PR description to clarify the functional sorting fix and added a demo video showing the corrected behavior when Dimensions are set.
I am adding tests now will push it shortly!
Please let me know if any further adjustments are needed.
--
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] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3901669730
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] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #37712:
URL: https://github.com/apache/superset/pull/37712#discussion_r2805326523
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformPropsWithDimensions.test.ts:
##########
@@ -0,0 +1,510 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * Tests for X-Axis Sort By with Dimensions in Bar Charts
+ *
+ * These tests verify the fix for issue #34352, ensuring:
+ * 1. X-Axis Sort By actually works in multi-series scenarios
+ * 2. Categories are ordered correctly based on sort configuration
+ * 3. The mapXAxisSortToSeriesType function properly maps sort values
+ */
+
+import { ChartProps, SqlaFormData } from '@superset-ui/core';
+import { supersetTheme } from '@apache-superset/core/ui';
+import { EchartsTimeseriesChartProps } from '../../src/types';
+import transformProps from '../../src/Timeseries/transformProps';
+import { DEFAULT_FORM_DATA } from '../../src/Timeseries/constants';
+import { EchartsTimeseriesSeriesType } from '../../src/Timeseries/types';
+
+describe('Bar Chart X-Axis Sort By with Dimensions', () => {
+ const baseFormData: SqlaFormData = {
+ ...DEFAULT_FORM_DATA,
+ colorScheme: 'bnbColors',
+ datasource: '3__table',
+ granularity_sqla: '__timestamp',
+ viz_type: 'echarts_timeseries_bar',
+ seriesType: EchartsTimeseriesSeriesType.Bar,
+ orientation: 'vertical',
+ };
+
+ describe('Multi-series sorting (with groupby)', () => {
+ const createMultiSeriesData = () => [
+ {
+ data: [
+ {
+ genre: 'Action',
+ platform: 'Console',
+ na_sales: 150,
+ __timestamp: 1609459200000,
+ },
+ {
+ genre: 'Sports',
+ platform: 'Console',
+ na_sales: 200,
+ __timestamp: 1609459200000,
+ },
+ {
+ genre: 'RPG',
+ platform: 'Console',
+ na_sales: 80,
+ __timestamp: 1609459200000,
+ },
+ {
+ genre: 'Action',
+ platform: 'PC',
+ na_sales: 120,
+ __timestamp: 1609459200000,
+ },
+ {
+ genre: 'Sports',
+ platform: 'PC',
+ na_sales: 180,
+ __timestamp: 1609459200000,
+ },
+ {
+ genre: 'RPG',
+ platform: 'PC',
+ na_sales: 90,
+ __timestamp: 1609459200000,
+ },
+ ],
+ colnames: ['genre', 'platform', 'na_sales', '__timestamp'],
+ coltypes: ['STRING', 'STRING', 'BIGINT', 'TIMESTAMP'],
+ },
+ ];
+
+ it('should handle xAxisSort with dimensions present', () => {
+ const formData = {
+ ...baseFormData,
+ groupby: ['platform'], // This creates multi-series
+ metrics: ['na_sales'],
+ x_axis: 'genre',
+ x_axis_sort: 'sum',
+ x_axis_sort_asc: false,
+ };
+
+ const chartProps = new ChartProps({
+ width: 800,
+ height: 600,
+ formData,
+ queriesData: createMultiSeriesData(),
+ theme: supersetTheme,
+ }) as unknown as EchartsTimeseriesChartProps;
+
+ const transformedProps = transformProps(chartProps);
+
+ // Verify chart renders successfully
+ expect(transformedProps.echartOptions).toBeDefined();
+ expect(transformedProps.echartOptions.series).toBeDefined();
+
+ const series = transformedProps.echartOptions.series as any[];
+ expect(series.length).toBeGreaterThan(0);
+
+ // Each series should have data
+ series.forEach(s => {
+ expect(s.data).toBeDefined();
+ expect(Array.isArray(s.data)).toBe(true);
+ });
+ });
+
+ it('should sort categories by sum when xAxisSort=sum with dimension', () => {
+ const formData = {
+ ...baseFormData,
+ groupby: ['platform'],
+ metrics: ['na_sales'],
+ x_axis: 'genre',
+ x_axis_sort: 'sum',
+ x_axis_sort_asc: false, // descending
+ };
+
+ const chartProps = new ChartProps({
+ width: 800,
+ height: 600,
+ formData,
+ queriesData: createMultiSeriesData(),
+ theme: supersetTheme,
+ }) as unknown as EchartsTimeseriesChartProps;
+
+ const transformedProps = transformProps(chartProps);
+
+ // Expected order by sum (descending):
+ // Sports: 200 + 180 = 380
+ // Action: 150 + 120 = 270
+ // RPG: 80 + 90 = 170
+
+ const xAxisData = (transformedProps.echartOptions.xAxis as any)?.data;
+
+ if (xAxisData && Array.isArray(xAxisData)) {
+ expect(xAxisData.length).toBe(3);
+ // Verify the order is correct
+ expect(xAxisData[0]).toBe('Sports');
+ expect(xAxisData[1]).toBe('Action');
+ expect(xAxisData[2]).toBe('RPG');
+ }
+ });
+
+ it('should sort categories alphabetically when xAxisSort=name', () => {
+ const formData = {
+ ...baseFormData,
+ groupby: ['platform'],
+ metrics: ['na_sales'],
+ x_axis: 'genre',
+ x_axis_sort: 'name',
+ x_axis_sort_asc: true,
+ };
+
+ const chartProps = new ChartProps({
+ width: 800,
+ height: 600,
+ formData,
+ queriesData: createMultiSeriesData(),
+ theme: supersetTheme,
+ }) as unknown as EchartsTimeseriesChartProps;
+
+ const transformedProps = transformProps(chartProps);
+ const xAxisData = (transformedProps.echartOptions.xAxis as any)?.data;
+
+ if (xAxisData && Array.isArray(xAxisData)) {
+ // Alphabetical order: Action, RPG, Sports
+ expect(xAxisData[0]).toBe('Action');
+ expect(xAxisData[1]).toBe('RPG');
+ expect(xAxisData[2]).toBe('Sports');
+ }
+ });
+
+ it('should apply ascending sort correctly', () => {
+ const formData = {
+ ...baseFormData,
+ groupby: ['platform'],
+ metrics: ['na_sales'],
+ x_axis: 'genre',
+ x_axis_sort: 'sum',
+ x_axis_sort_asc: true, // ascending
+ };
+
+ const chartProps = new ChartProps({
+ width: 800,
+ height: 600,
+ formData,
+ queriesData: createMultiSeriesData(),
+ theme: supersetTheme,
+ }) as unknown as EchartsTimeseriesChartProps;
+
+ const transformedProps = transformProps(chartProps);
+ const xAxisData = (transformedProps.echartOptions.xAxis as any)?.data;
+
+ if (xAxisData && Array.isArray(xAxisData)) {
+ // Ascending order by sum: RPG (170), Action (270), Sports (380)
+ expect(xAxisData[0]).toBe('RPG');
+ expect(xAxisData[1]).toBe('Action');
+ expect(xAxisData[2]).toBe('Sports');
+ }
+ });
+
+ it('should sort by average when xAxisSort=avg', () => {
+ const formData = {
+ ...baseFormData,
+ groupby: ['platform'],
+ metrics: ['na_sales'],
+ x_axis: 'genre',
+ x_axis_sort: 'avg',
+ x_axis_sort_asc: false,
+ };
+
+ const chartProps = new ChartProps({
+ width: 800,
+ height: 600,
+ formData,
+ queriesData: createMultiSeriesData(),
+ theme: supersetTheme,
+ }) as unknown as EchartsTimeseriesChartProps;
+
+ const transformedProps = transformProps(chartProps);
+ const xAxisData = (transformedProps.echartOptions.xAxis as any)?.data;
+
+ if (xAxisData && Array.isArray(xAxisData)) {
+ expect(xAxisData.length).toBe(3);
+ // Average values:
+ // Sports: (200 + 180) / 2 = 190
+ // Action: (150 + 120) / 2 = 135
+ // RPG: (80 + 90) / 2 = 85
+ expect(xAxisData[0]).toBe('Sports');
+ expect(xAxisData[1]).toBe('Action');
+ expect(xAxisData[2]).toBe('RPG');
+ }
+ });
+ });
+
+ describe('Day name sorting example', () => {
+ // This tests the specific use case mentioned in the issue:
+ // Sorting day names chronologically using a numeric helper column
+ const createDayData = () => [
+ {
+ data: [
+ {
+ day_name: 'Monday',
+ day_number: 1,
+ category: 'A',
+ sales: 100,
+ __timestamp: 1609459200000,
+ },
+ {
+ day_name: 'Wednesday',
+ day_number: 3,
+ category: 'A',
+ sales: 150,
+ __timestamp: 1609459200000,
+ },
+ {
+ day_name: 'Friday',
+ day_number: 5,
+ category: 'A',
+ sales: 200,
+ __timestamp: 1609459200000,
+ },
+ {
+ day_name: 'Monday',
+ day_number: 1,
+ category: 'B',
+ sales: 80,
+ __timestamp: 1609459200000,
+ },
+ {
+ day_name: 'Wednesday',
+ day_number: 3,
+ category: 'B',
+ sales: 120,
+ __timestamp: 1609459200000,
+ },
+ {
+ day_name: 'Friday',
+ day_number: 5,
+ category: 'B',
+ sales: 90,
+ __timestamp: 1609459200000,
+ },
+ ],
+ colnames: [
+ 'day_name',
+ 'day_number',
+ 'category',
+ 'sales',
+ '__timestamp',
+ ],
+ coltypes: ['STRING', 'BIGINT', 'STRING', 'BIGINT', 'TIMESTAMP'],
+ },
+ ];
+
+ it('should sort day names by numeric column when dimension is present', () => {
+ const formData = {
+ ...baseFormData,
+ groupby: ['category'], // dimension creates multi-series
+ metrics: ['sales'],
+ x_axis: 'day_name',
+ x_axis_sort: 'day_number', // sort by the numeric helper
+ x_axis_sort_asc: true,
+ };
+
+ const chartProps = new ChartProps({
+ width: 800,
+ height: 600,
+ formData,
+ queriesData: createDayData(),
+ theme: supersetTheme,
+ }) as unknown as EchartsTimeseriesChartProps;
+
+ const transformedProps = transformProps(chartProps);
+ const xAxisData = (transformedProps.echartOptions.xAxis as any)?.data;
+
+ if (xAxisData && Array.isArray(xAxisData)) {
+ // Should be in chronological order (by day_number)
+ // NOT alphabetical order (which would be Friday, Monday, Wednesday)
+ expect(xAxisData[0]).toBe('Monday');
+ expect(xAxisData[1]).toBe('Wednesday');
+ expect(xAxisData[2]).toBe('Friday');
+ }
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Unsupported x_axis_sort value</b></div>
<div id="fix">
The test uses x_axis_sort: 'day_number' expecting sorting by a numeric column, but mapXAxisSortToSeriesType only supports predefined keywords ('sum', 'avg', 'name', etc.) and returns undefined for 'day_number', preventing sorting.
</div>
</div>
<small><i>Code Review Run #d84cb8</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/transformPropsWithDimensions.test.ts:
##########
@@ -0,0 +1,510 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * Tests for X-Axis Sort By with Dimensions in Bar Charts
+ *
+ * These tests verify the fix for issue #34352, ensuring:
+ * 1. X-Axis Sort By actually works in multi-series scenarios
+ * 2. Categories are ordered correctly based on sort configuration
+ * 3. The mapXAxisSortToSeriesType function properly maps sort values
+ */
+
+import { ChartProps, SqlaFormData } from '@superset-ui/core';
+import { supersetTheme } from '@apache-superset/core/ui';
+import { EchartsTimeseriesChartProps } from '../../src/types';
+import transformProps from '../../src/Timeseries/transformProps';
+import { DEFAULT_FORM_DATA } from '../../src/Timeseries/constants';
+import { EchartsTimeseriesSeriesType } from '../../src/Timeseries/types';
+
+describe('Bar Chart X-Axis Sort By with Dimensions', () => {
+ const baseFormData: SqlaFormData = {
+ ...DEFAULT_FORM_DATA,
+ colorScheme: 'bnbColors',
+ datasource: '3__table',
+ granularity_sqla: '__timestamp',
+ viz_type: 'echarts_timeseries_bar',
+ seriesType: EchartsTimeseriesSeriesType.Bar,
+ orientation: 'vertical',
+ };
+
+ describe('Multi-series sorting (with groupby)', () => {
+ const createMultiSeriesData = () => [
+ {
+ data: [
+ {
+ genre: 'Action',
+ platform: 'Console',
+ na_sales: 150,
+ __timestamp: 1609459200000,
+ },
+ {
+ genre: 'Sports',
+ platform: 'Console',
+ na_sales: 200,
+ __timestamp: 1609459200000,
+ },
+ {
+ genre: 'RPG',
+ platform: 'Console',
+ na_sales: 80,
+ __timestamp: 1609459200000,
+ },
+ {
+ genre: 'Action',
+ platform: 'PC',
+ na_sales: 120,
+ __timestamp: 1609459200000,
+ },
+ {
+ genre: 'Sports',
+ platform: 'PC',
+ na_sales: 180,
+ __timestamp: 1609459200000,
+ },
+ {
+ genre: 'RPG',
+ platform: 'PC',
+ na_sales: 90,
+ __timestamp: 1609459200000,
+ },
+ ],
+ colnames: ['genre', 'platform', 'na_sales', '__timestamp'],
+ coltypes: ['STRING', 'STRING', 'BIGINT', 'TIMESTAMP'],
+ },
+ ];
+
+ it('should handle xAxisSort with dimensions present', () => {
+ const formData = {
+ ...baseFormData,
+ groupby: ['platform'], // This creates multi-series
+ metrics: ['na_sales'],
+ x_axis: 'genre',
+ x_axis_sort: 'sum',
+ x_axis_sort_asc: false,
+ };
+
+ const chartProps = new ChartProps({
+ width: 800,
+ height: 600,
+ formData,
+ queriesData: createMultiSeriesData(),
+ theme: supersetTheme,
+ }) as unknown as EchartsTimeseriesChartProps;
+
+ const transformedProps = transformProps(chartProps);
+
+ // Verify chart renders successfully
+ expect(transformedProps.echartOptions).toBeDefined();
+ expect(transformedProps.echartOptions.series).toBeDefined();
+
+ const series = transformedProps.echartOptions.series as any[];
+ expect(series.length).toBeGreaterThan(0);
+
+ // Each series should have data
+ series.forEach(s => {
+ expect(s.data).toBeDefined();
+ expect(Array.isArray(s.data)).toBe(true);
+ });
+ });
+
+ it('should sort categories by sum when xAxisSort=sum with dimension', () => {
+ const formData = {
+ ...baseFormData,
+ groupby: ['platform'],
+ metrics: ['na_sales'],
+ x_axis: 'genre',
+ x_axis_sort: 'sum',
+ x_axis_sort_asc: false, // descending
+ };
+
+ const chartProps = new ChartProps({
+ width: 800,
+ height: 600,
+ formData,
+ queriesData: createMultiSeriesData(),
+ theme: supersetTheme,
+ }) as unknown as EchartsTimeseriesChartProps;
+
+ const transformedProps = transformProps(chartProps);
+
+ // Expected order by sum (descending):
+ // Sports: 200 + 180 = 380
+ // Action: 150 + 120 = 270
+ // RPG: 80 + 90 = 170
+
+ const xAxisData = (transformedProps.echartOptions.xAxis as any)?.data;
+
+ if (xAxisData && Array.isArray(xAxisData)) {
+ expect(xAxisData.length).toBe(3);
+ // Verify the order is correct
+ expect(xAxisData[0]).toBe('Sports');
+ expect(xAxisData[1]).toBe('Action');
+ expect(xAxisData[2]).toBe('RPG');
+ }
+ });
+
+ it('should sort categories alphabetically when xAxisSort=name', () => {
+ const formData = {
+ ...baseFormData,
+ groupby: ['platform'],
+ metrics: ['na_sales'],
+ x_axis: 'genre',
+ x_axis_sort: 'name',
+ x_axis_sort_asc: true,
+ };
+
+ const chartProps = new ChartProps({
+ width: 800,
+ height: 600,
+ formData,
+ queriesData: createMultiSeriesData(),
+ theme: supersetTheme,
+ }) as unknown as EchartsTimeseriesChartProps;
+
+ const transformedProps = transformProps(chartProps);
+ const xAxisData = (transformedProps.echartOptions.xAxis as any)?.data;
+
+ if (xAxisData && Array.isArray(xAxisData)) {
+ // Alphabetical order: Action, RPG, Sports
+ expect(xAxisData[0]).toBe('Action');
+ expect(xAxisData[1]).toBe('RPG');
+ expect(xAxisData[2]).toBe('Sports');
+ }
+ });
+
+ it('should apply ascending sort correctly', () => {
+ const formData = {
+ ...baseFormData,
+ groupby: ['platform'],
+ metrics: ['na_sales'],
+ x_axis: 'genre',
+ x_axis_sort: 'sum',
+ x_axis_sort_asc: true, // ascending
+ };
+
+ const chartProps = new ChartProps({
+ width: 800,
+ height: 600,
+ formData,
+ queriesData: createMultiSeriesData(),
+ theme: supersetTheme,
+ }) as unknown as EchartsTimeseriesChartProps;
+
+ const transformedProps = transformProps(chartProps);
+ const xAxisData = (transformedProps.echartOptions.xAxis as any)?.data;
+
+ if (xAxisData && Array.isArray(xAxisData)) {
+ // Ascending order by sum: RPG (170), Action (270), Sports (380)
+ expect(xAxisData[0]).toBe('RPG');
+ expect(xAxisData[1]).toBe('Action');
+ expect(xAxisData[2]).toBe('Sports');
+ }
+ });
+
+ it('should sort by average when xAxisSort=avg', () => {
+ const formData = {
+ ...baseFormData,
+ groupby: ['platform'],
+ metrics: ['na_sales'],
+ x_axis: 'genre',
+ x_axis_sort: 'avg',
+ x_axis_sort_asc: false,
+ };
+
+ const chartProps = new ChartProps({
+ width: 800,
+ height: 600,
+ formData,
+ queriesData: createMultiSeriesData(),
+ theme: supersetTheme,
+ }) as unknown as EchartsTimeseriesChartProps;
+
+ const transformedProps = transformProps(chartProps);
+ const xAxisData = (transformedProps.echartOptions.xAxis as any)?.data;
+
+ if (xAxisData && Array.isArray(xAxisData)) {
+ expect(xAxisData.length).toBe(3);
+ // Average values:
+ // Sports: (200 + 180) / 2 = 190
+ // Action: (150 + 120) / 2 = 135
+ // RPG: (80 + 90) / 2 = 85
+ expect(xAxisData[0]).toBe('Sports');
+ expect(xAxisData[1]).toBe('Action');
+ expect(xAxisData[2]).toBe('RPG');
+ }
+ });
+ });
+
+ describe('Day name sorting example', () => {
+ // This tests the specific use case mentioned in the issue:
+ // Sorting day names chronologically using a numeric helper column
+ const createDayData = () => [
+ {
+ data: [
+ {
+ day_name: 'Monday',
+ day_number: 1,
+ category: 'A',
+ sales: 100,
+ __timestamp: 1609459200000,
+ },
+ {
+ day_name: 'Wednesday',
+ day_number: 3,
+ category: 'A',
+ sales: 150,
+ __timestamp: 1609459200000,
+ },
+ {
+ day_name: 'Friday',
+ day_number: 5,
+ category: 'A',
+ sales: 200,
+ __timestamp: 1609459200000,
+ },
+ {
+ day_name: 'Monday',
+ day_number: 1,
+ category: 'B',
+ sales: 80,
+ __timestamp: 1609459200000,
+ },
+ {
+ day_name: 'Wednesday',
+ day_number: 3,
+ category: 'B',
+ sales: 120,
+ __timestamp: 1609459200000,
+ },
+ {
+ day_name: 'Friday',
+ day_number: 5,
+ category: 'B',
+ sales: 90,
+ __timestamp: 1609459200000,
+ },
+ ],
+ colnames: [
+ 'day_name',
+ 'day_number',
+ 'category',
+ 'sales',
+ '__timestamp',
+ ],
+ coltypes: ['STRING', 'BIGINT', 'STRING', 'BIGINT', 'TIMESTAMP'],
+ },
+ ];
+
+ it('should sort day names by numeric column when dimension is present', () => {
+ const formData = {
+ ...baseFormData,
+ groupby: ['category'], // dimension creates multi-series
+ metrics: ['sales'],
+ x_axis: 'day_name',
+ x_axis_sort: 'day_number', // sort by the numeric helper
+ x_axis_sort_asc: true,
+ };
+
+ const chartProps = new ChartProps({
+ width: 800,
+ height: 600,
+ formData,
+ queriesData: createDayData(),
+ theme: supersetTheme,
+ }) as unknown as EchartsTimeseriesChartProps;
+
+ const transformedProps = transformProps(chartProps);
+ const xAxisData = (transformedProps.echartOptions.xAxis as any)?.data;
+
+ if (xAxisData && Array.isArray(xAxisData)) {
+ // Should be in chronological order (by day_number)
+ // NOT alphabetical order (which would be Friday, Monday, Wednesday)
+ expect(xAxisData[0]).toBe('Monday');
+ expect(xAxisData[1]).toBe('Wednesday');
+ expect(xAxisData[2]).toBe('Friday');
+ }
+ });
+ });
+
+ describe('Edge cases', () => {
+ it('should handle single-series charts (backward compatibility)', () => {
+ const formData = {
+ ...baseFormData,
+ groupby: [], // No dimension
+ metrics: ['sales'],
+ x_axis: 'category',
+ x_axis_sort: undefined,
+ };
+
+ const testData = [
+ {
+ data: [
+ { category: 'B', sales: 100, __timestamp: 1609459200000 },
+ { category: 'A', sales: 150, __timestamp: 1609459200000 },
+ ],
+ colnames: ['category', 'sales', '__timestamp'],
+ coltypes: ['STRING', 'BIGINT', 'TIMESTAMP'],
+ },
+ ];
+
+ const chartProps = new ChartProps({
+ width: 800,
+ height: 600,
+ formData,
+ queriesData: testData,
+ theme: supersetTheme,
+ }) as unknown as EchartsTimeseriesChartProps;
+
+ // Should not throw
+ expect(() => transformProps(chartProps)).not.toThrow();
+
+ const transformedProps = transformProps(chartProps);
+ expect(transformedProps.echartOptions.series).toBeDefined();
+ });
+
+ it('should handle empty data gracefully', () => {
+ const formData = {
+ ...baseFormData,
+ groupby: ['category'],
+ metrics: ['sales'],
+ x_axis: 'genre',
+ x_axis_sort: 'sum',
+ };
+
+ const emptyData = [
+ {
+ data: [],
+ colnames: ['genre', 'category', 'sales', '__timestamp'],
+ coltypes: ['STRING', 'STRING', 'BIGINT', 'TIMESTAMP'],
+ },
+ ];
+
+ const chartProps = new ChartProps({
+ width: 800,
+ height: 600,
+ formData,
+ queriesData: emptyData,
+ theme: supersetTheme,
+ }) as unknown as EchartsTimeseriesChartProps;
+
+ expect(() => transformProps(chartProps)).not.toThrow();
+ });
+
+ it('should handle horizontal orientation', () => {
+ const formData = {
+ ...baseFormData,
+ orientation: 'horizontal' as const,
+ groupby: ['category'],
+ metrics: ['sales'],
+ x_axis: 'genre',
+ x_axis_sort: 'sum',
+ };
+
+ const testData = [
+ {
+ data: [
+ {
+ genre: 'A',
+ category: 'X',
+ sales: 100,
+ __timestamp: 1609459200000,
+ },
+ {
+ genre: 'B',
+ category: 'X',
+ sales: 150,
+ __timestamp: 1609459200000,
+ },
+ ],
+ colnames: ['genre', 'category', 'sales', '__timestamp'],
+ coltypes: ['STRING', 'STRING', 'BIGINT', 'TIMESTAMP'],
+ },
+ ];
+
+ const chartProps = new ChartProps({
+ width: 800,
+ height: 600,
+ formData,
+ queriesData: testData,
+ theme: supersetTheme,
+ }) as unknown as EchartsTimeseriesChartProps;
+
+ expect(() => transformProps(chartProps)).not.toThrow();
+ });
+ });
+
+ describe('Multiple metrics with dimensions', () => {
+ it('should handle sorting with multiple metrics and dimensions', () => {
+ const formData = {
+ ...baseFormData,
+ groupby: ['platform'],
+ metrics: ['na_sales', 'eu_sales'],
+ x_axis: 'genre',
+ x_axis_sort: 'avg',
+ x_axis_sort_asc: false,
+ };
+
+ const testData = [
+ {
+ data: [
+ {
+ genre: 'Action',
+ platform: 'Console',
+ na_sales: 150,
+ eu_sales: 100,
+ __timestamp: 1609459200000,
+ },
+ {
+ genre: 'Sports',
+ platform: 'Console',
+ na_sales: 200,
+ eu_sales: 120,
+ __timestamp: 1609459200000,
+ },
+ ],
+ colnames: [
+ 'genre',
+ 'platform',
+ 'na_sales',
+ 'eu_sales',
+ '__timestamp',
+ ],
+ coltypes: ['STRING', 'STRING', 'BIGINT', 'BIGINT', 'TIMESTAMP'],
+ },
+ ];
+
+ const chartProps = new ChartProps({
+ width: 800,
+ height: 600,
+ formData,
+ queriesData: testData,
+ theme: supersetTheme,
+ }) as unknown as EchartsTimeseriesChartProps;
+
+ const transformedProps = transformProps(chartProps);
+
+ expect(transformedProps.echartOptions.series).toBeDefined();
+ const series = transformedProps.echartOptions.series as any[];
+
+ // Should have series for multiple metrics
+ expect(series.length).toBeGreaterThan(0);
+ });
+ });
+});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Weak test assertions</b></div>
<div id="fix">
The added test cases primarily check that `transformProps` doesn't throw and that basic output exists, but they don't validate the actual business logic like correct series generation, data transformation, or sorting behavior. For instance, the multiple metrics test only confirms `series.length > 0` without checking if series correspond to each metric or contain expected data. This risks missing regressions in data processing. Consider adding assertions on `transformedProps.echartOptions.series` content, x-axis ordering, and metric-specific series to align with testing best practices.
</div>
</div>
<small><i>Code Review Run #d84cb8</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/transformPropsWithDimensions.test.ts:
##########
@@ -0,0 +1,510 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * Tests for X-Axis Sort By with Dimensions in Bar Charts
+ *
+ * These tests verify the fix for issue #34352, ensuring:
+ * 1. X-Axis Sort By actually works in multi-series scenarios
+ * 2. Categories are ordered correctly based on sort configuration
+ * 3. The mapXAxisSortToSeriesType function properly maps sort values
+ */
+
+import { ChartProps, SqlaFormData } from '@superset-ui/core';
+import { supersetTheme } from '@apache-superset/core/ui';
+import { EchartsTimeseriesChartProps } from '../../src/types';
+import transformProps from '../../src/Timeseries/transformProps';
+import { DEFAULT_FORM_DATA } from '../../src/Timeseries/constants';
+import { EchartsTimeseriesSeriesType } from '../../src/Timeseries/types';
+
+describe('Bar Chart X-Axis Sort By with Dimensions', () => {
+ const baseFormData: SqlaFormData = {
+ ...DEFAULT_FORM_DATA,
+ colorScheme: 'bnbColors',
+ datasource: '3__table',
+ granularity_sqla: '__timestamp',
+ viz_type: 'echarts_timeseries_bar',
+ seriesType: EchartsTimeseriesSeriesType.Bar,
+ orientation: 'vertical',
+ };
+
+ describe('Multi-series sorting (with groupby)', () => {
+ const createMultiSeriesData = () => [
+ {
+ data: [
+ {
+ genre: 'Action',
+ platform: 'Console',
+ na_sales: 150,
+ __timestamp: 1609459200000,
+ },
+ {
+ genre: 'Sports',
+ platform: 'Console',
+ na_sales: 200,
+ __timestamp: 1609459200000,
+ },
+ {
+ genre: 'RPG',
+ platform: 'Console',
+ na_sales: 80,
+ __timestamp: 1609459200000,
+ },
+ {
+ genre: 'Action',
+ platform: 'PC',
+ na_sales: 120,
+ __timestamp: 1609459200000,
+ },
+ {
+ genre: 'Sports',
+ platform: 'PC',
+ na_sales: 180,
+ __timestamp: 1609459200000,
+ },
+ {
+ genre: 'RPG',
+ platform: 'PC',
+ na_sales: 90,
+ __timestamp: 1609459200000,
+ },
+ ],
+ colnames: ['genre', 'platform', 'na_sales', '__timestamp'],
+ coltypes: ['STRING', 'STRING', 'BIGINT', 'TIMESTAMP'],
+ },
+ ];
+
+ it('should handle xAxisSort with dimensions present', () => {
+ const formData = {
+ ...baseFormData,
+ groupby: ['platform'], // This creates multi-series
+ metrics: ['na_sales'],
+ x_axis: 'genre',
+ x_axis_sort: 'sum',
+ x_axis_sort_asc: false,
+ };
+
+ const chartProps = new ChartProps({
+ width: 800,
+ height: 600,
+ formData,
+ queriesData: createMultiSeriesData(),
+ theme: supersetTheme,
+ }) as unknown as EchartsTimeseriesChartProps;
+
+ const transformedProps = transformProps(chartProps);
+
+ // Verify chart renders successfully
+ expect(transformedProps.echartOptions).toBeDefined();
+ expect(transformedProps.echartOptions.series).toBeDefined();
+
+ const series = transformedProps.echartOptions.series as any[];
+ expect(series.length).toBeGreaterThan(0);
+
+ // Each series should have data
+ series.forEach(s => {
+ expect(s.data).toBeDefined();
+ expect(Array.isArray(s.data)).toBe(true);
+ });
+ });
+
+ it('should sort categories by sum when xAxisSort=sum with dimension', () => {
+ const formData = {
+ ...baseFormData,
+ groupby: ['platform'],
+ metrics: ['na_sales'],
+ x_axis: 'genre',
+ x_axis_sort: 'sum',
+ x_axis_sort_asc: false, // descending
+ };
+
+ const chartProps = new ChartProps({
+ width: 800,
+ height: 600,
+ formData,
+ queriesData: createMultiSeriesData(),
+ theme: supersetTheme,
+ }) as unknown as EchartsTimeseriesChartProps;
+
+ const transformedProps = transformProps(chartProps);
+
+ // Expected order by sum (descending):
+ // Sports: 200 + 180 = 380
+ // Action: 150 + 120 = 270
+ // RPG: 80 + 90 = 170
+
+ const xAxisData = (transformedProps.echartOptions.xAxis as any)?.data;
+
+ if (xAxisData && Array.isArray(xAxisData)) {
+ expect(xAxisData.length).toBe(3);
+ // Verify the order is correct
+ expect(xAxisData[0]).toBe('Sports');
+ expect(xAxisData[1]).toBe('Action');
+ expect(xAxisData[2]).toBe('RPG');
+ }
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Invalid xAxis.data assertion</b></div>
<div id="fix">
The test checks transformedProps.echartOptions.xAxis.data for sorted order, but the transformProps function does not set this property. ECharts may infer category data from series, but the test assertion won't execute since xAxis.data is undefined.
</div>
</div>
<small><i>Code Review Run #d84cb8</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] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "sfirke (via GitHub)" <gi...@apache.org>.
sfirke commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3935894677
@YousufFFFF - please see [my comment above where I have three screenshots](https://github.com/apache/superset/pull/37712#issuecomment-3873527775) and the third one - the ultimate test case - is not correct in the test environment. When I try to recreate that third chart in the current test environment, it's different than before, but I think it's wrong:
<img width="2166" height="1115" alt="image" src="https://github.com/user-attachments/assets/6910014f-df51-43f9-b71c-300a53fd56cd" />
It looks to me like despite setting X-Axis Sort By to `SUM(na_sales)`, the chart bars are ordering by the *metric*, `COUNT(*)`. The order of the bars should match my second screenshot in the linked comment and should not change when we add a dimension. Right now the x-axis sort order changes when we add or remove a dimension which is not right.
Please investigate. Before you mark this as ready for review again, can you document with a screenshot that it can produce the third chart correctly: one variable for the X-axis (defines the bar groups), another for the X-Axis sort by (how they are ordered), a third for the metric (how tall the bars are), and a fourth for the dimension (the colored segments of the bars)?
Let me know if you think my assessment of the situation is off. Thanks for your continued work on this - in my opinion, this is the biggest bug currently affecting Superset, so it would be amazing to get this merged!
--
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] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3935424979
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/22228256649) for [240c741](https://github.com/apache/superset/commit/240c741)
• **Environment:** http://35.93.205.47:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3916607276
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/22111381130) for [240c741](https://github.com/apache/superset/commit/240c741)
• **Environment:** http://35.86.83.178:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(bar chart): X-Axis Sort By behavior when Dimensions are set [superset]
Posted by "YousufFFFF (via GitHub)" <gi...@apache.org>.
YousufFFFF commented on code in PR #37712:
URL: https://github.com/apache/superset/pull/37712#discussion_r2775942636
##########
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/customControls.tsx:
##########
@@ -146,37 +146,44 @@ export const xAxisSortControl = {
const columns = [controls?.x_axis?.value as QueryFormColumn].filter(
Boolean,
);
- const isSingleSortAvailable =
- ensureIsArray(controls?.groupby?.value).length === 0;
- const isMultiSortAvailable =
- !!ensureIsArray(controls?.groupby?.value).length ||
+ const hasGroupBy = ensureIsArray(controls?.groupby?.value).length > 0;
+ const hasMultipleMetrics =
ensureIsArray(controls?.metrics?.value).length > 1;
+ const isMultiSortAvailable = hasGroupBy || hasMultipleMetrics;
const metrics = [
...ensureIsArray(controls?.metrics?.value as QueryFormMetric),
controls?.timeseries_limit_metric?.value as QueryFormMetric,
].filter(Boolean);
const metricLabels = [...new Set(metrics.map(getMetricLabel))];
- const options = [
- ...(isSingleSortAvailable
- ? [
- ...columns.map(column => {
- const value = getColumnLabel(column);
- return { value, label: dataset?.verbose_map?.[value] || value };
- }),
- ...metricLabels.map(value => ({
- value,
- label: dataset?.verbose_map?.[value] || value,
- })),
- ]
- : []),
- ...(isMultiSortAvailable
- ? SORT_SERIES_CHOICES.map(choice => ({
- value: choice[0],
- label: choice[1],
- }))
- : []),
+
+ // Base axis sort options: always include the explicit axis column and metrics,
+ // regardless of whether a dimension (groupby) is present. This keeps the
+ // "X-Axis Sort By" dropdown consistent when toggling dimensions.
+ const axisSortOptions = [
+ ...columns.map(column => {
Review Comment:
Agreed that in multi-series mode the ECharts pipeline interprets
xAxisSort as a SortSeriesType, and supporting arbitrary metric/axis values there would
require additional encoding/mapping and consumer changes.
To keep this PR scoped, the intent here is limited to keeping the “X-Axis Sort By”
dropdown options consistent when toggling dimensions, without changing the underlying
sorting semantics. Happy to follow up with a separate PR if we decide to expand support
for metric-based sorting with dimensions.
--
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] fix(bar chart): X-Axis Sort By behavior when Dimensions are set [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3866235332
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] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "YousufFFFF (via GitHub)" <gi...@apache.org>.
YousufFFFF commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3883391860
Hi @sfirke!
I've added unit tests covering the transform-level behavior for xAxisSort when dimensions are present.
The tests validate correct series reordering based on the selected sort type (e.g., sum) and ensure sorting works as expected in multi-series scenarios.
--
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] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3883649839
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] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-5647661333
@YousufFFFF given no update since I flagged the mapXAxisSortToSeriesType fallback and this now conflicts with master, I'm going to take it forward on a fresh PR — appreciate you nailing down the repro for #34352 and #38249.
--
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] fix(bar chart): X-Axis Sort By behavior when Dimensions are set [superset]
Posted by "YousufFFFF (via GitHub)" <gi...@apache.org>.
YousufFFFF commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3873632529
Would you prefer that I:
1. Extend this PR to also map metric-based sorting correctly in the ECharts multi-series path, or
2. Keep this PR focused on the UI fix and follow up with a separate PR for the sorting behavior?
Happy to proceed in whichever direction you recommend.
--
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] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "sfirke (via GitHub)" <gi...@apache.org>.
sfirke commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3884008809
Thank you@YousufFFFF ! Can you try to get that pre-commit check passing?
--
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] fix(bar chart): X-Axis Sort By behavior when Dimensions are set [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #37712:
URL: https://github.com/apache/superset/pull/37712#discussion_r2775458882
##########
superset-frontend/packages/superset-ui-chart-controls/test/shared-controls/xAxisSortControl.test.ts:
##########
@@ -0,0 +1,100 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { ControlPanelState } from '../../src/types';
+import { xAxisSortControl } from '../../src/shared-controls/customControls';
+
+const createState = (
+ overrides: Partial<ControlPanelState>,
+): ControlPanelState =>
+ ({
+ slice: { slice_id: 1 },
+ form_data: {},
+ datasource: {
+ column_formats: {},
+ verbose_map: {},
+ },
+ controls: {},
+ common: {},
+ metadata: {},
+ ...overrides,
+ }) as ControlPanelState;
+
+const createControlState = (value: unknown = undefined) => ({ value }) as any;
+
+test('xAxisSortControl includes axis and metric options when there is no dimension', () => {
+ const state = createState({
+ controls: {
+ x_axis: { value: 'ds', type: 'Select' },
+ groupby: { value: [], type: 'Select' },
+ metrics: { value: ['metric_1'], type: 'Select' },
+ timeseries_limit_metric: { value: 'sort_metric', type: 'Select' },
+ datasource: {
+ datasource: {
+ columns: [],
+ metrics: [],
+ },
+ type: 'Select',
+ },
+ },
+ });
+
+ const { options } = xAxisSortControl.config.mapStateToProps!(
+ state,
+ createControlState(),
+ ) as { options: { value: string }[] };
+
+ const values = options.map(opt => opt.value);
+
+ expect(values).toContain('ds');
+ expect(values).toContain('metric_1');
+});
+
+test('xAxisSortControl keeps axis and metric options when a dimension is set', () => {
+ const state = createState({
+ controls: {
+ x_axis: { value: 'ds', type: 'Select' },
+ groupby: { value: ['dim'], type: 'Select' },
+ metrics: { value: ['metric_1'], type: 'Select' },
+ timeseries_limit_metric: { value: 'sort_metric', type: 'Select' },
+ datasource: {
+ datasource: {
+ columns: [],
+ metrics: [],
+ },
+ type: 'Select',
+ },
+ },
+ });
+
+ const { options } = xAxisSortControl.config.mapStateToProps!(
+ state,
+ createControlState(),
+ ) as { options: { value: string }[] };
+
+ const values = options.map(opt => opt.value);
+
+ // Axis and metric choices should still be present
+ expect(values).toContain('ds');
+ expect(values).toContain('metric_1');
+
+ // And multi-series sort choices should also be available
Review Comment:
This test asserts that axis/metric labels (e.g. `ds`, `metric_1`) remain valid sort option values when `groupby` is set. However, in multi-series mode the ECharts Timeseries/Bar code path treats the sort value as a `SortSeriesType` enum (`name`/`sum`/etc), so allowing arbitrary metric labels can lead to unsupported values. Consider adjusting the implementation (and then the test) to verify only supported values are exposed, or add coverage for the value-encoding/mapping used to make metric-based sorting actually work with dimensions.
##########
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/customControls.tsx:
##########
@@ -146,37 +146,44 @@ export const xAxisSortControl = {
const columns = [controls?.x_axis?.value as QueryFormColumn].filter(
Boolean,
);
- const isSingleSortAvailable =
- ensureIsArray(controls?.groupby?.value).length === 0;
- const isMultiSortAvailable =
- !!ensureIsArray(controls?.groupby?.value).length ||
+ const hasGroupBy = ensureIsArray(controls?.groupby?.value).length > 0;
+ const hasMultipleMetrics =
ensureIsArray(controls?.metrics?.value).length > 1;
+ const isMultiSortAvailable = hasGroupBy || hasMultipleMetrics;
const metrics = [
...ensureIsArray(controls?.metrics?.value as QueryFormMetric),
controls?.timeseries_limit_metric?.value as QueryFormMetric,
].filter(Boolean);
const metricLabels = [...new Set(metrics.map(getMetricLabel))];
- const options = [
- ...(isSingleSortAvailable
- ? [
- ...columns.map(column => {
- const value = getColumnLabel(column);
- return { value, label: dataset?.verbose_map?.[value] || value };
- }),
- ...metricLabels.map(value => ({
- value,
- label: dataset?.verbose_map?.[value] || value,
- })),
- ]
- : []),
- ...(isMultiSortAvailable
- ? SORT_SERIES_CHOICES.map(choice => ({
- value: choice[0],
- label: choice[1],
- }))
- : []),
+
+ // Base axis sort options: always include the explicit axis column and metrics,
+ // regardless of whether a dimension (groupby) is present. This keeps the
+ // "X-Axis Sort By" dropdown consistent when toggling dimensions.
+ const axisSortOptions = [
+ ...columns.map(column => {
Review Comment:
`axisSortOptions` are now included even when `groupby` is set (multi-series). In the ECharts Timeseries/Bar pipeline, `xAxisSort` is interpreted as a `SortSeriesType` (name/sum/min/max/avg) for multi-series sorting; exposing arbitrary axis/metric labels here can yield unsupported values and result in incorrect/no sorting. Either keep multi-series options limited to `SORT_SERIES_CHOICES`, or add an explicit encoding/mapping (and update the consumer) so column/metric-based sorting is actually supported with dimensions.
##########
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/customControls.tsx:
##########
@@ -146,37 +146,44 @@ export const xAxisSortControl = {
const columns = [controls?.x_axis?.value as QueryFormColumn].filter(
Boolean,
);
- const isSingleSortAvailable =
- ensureIsArray(controls?.groupby?.value).length === 0;
- const isMultiSortAvailable =
- !!ensureIsArray(controls?.groupby?.value).length ||
+ const hasGroupBy = ensureIsArray(controls?.groupby?.value).length > 0;
+ const hasMultipleMetrics =
ensureIsArray(controls?.metrics?.value).length > 1;
+ const isMultiSortAvailable = hasGroupBy || hasMultipleMetrics;
const metrics = [
...ensureIsArray(controls?.metrics?.value as QueryFormMetric),
controls?.timeseries_limit_metric?.value as QueryFormMetric,
].filter(Boolean);
const metricLabels = [...new Set(metrics.map(getMetricLabel))];
- const options = [
- ...(isSingleSortAvailable
- ? [
- ...columns.map(column => {
- const value = getColumnLabel(column);
- return { value, label: dataset?.verbose_map?.[value] || value };
- }),
- ...metricLabels.map(value => ({
- value,
- label: dataset?.verbose_map?.[value] || value,
- })),
- ]
- : []),
- ...(isMultiSortAvailable
- ? SORT_SERIES_CHOICES.map(choice => ({
- value: choice[0],
- label: choice[1],
- }))
- : []),
+
+ // Base axis sort options: always include the explicit axis column and metrics,
+ // regardless of whether a dimension (groupby) is present. This keeps the
+ // "X-Axis Sort By" dropdown consistent when toggling dimensions.
+ const axisSortOptions = [
+ ...columns.map(column => {
+ const value = getColumnLabel(column);
+ return {
+ value,
+ label: dataset?.verbose_map?.[value as string] ?? value,
+ };
+ }),
+ ...metricLabels.map(value => ({
+ value,
+ label: dataset?.verbose_map?.[value as string] ?? value,
+ })),
];
+ // When there are multiple series (via groupby or multiple metrics),
+ // also expose the series-based sort options.
+ const multiSeriesOptions = isMultiSortAvailable
+ ? SORT_SERIES_CHOICES.map(([value, label]: [string, string]) => ({
+ value,
+ label,
+ }))
+ : [];
+
+ const options = [...axisSortOptions, ...multiSeriesOptions];
Review Comment:
`options` concatenates `axisSortOptions` with `multiSeriesOptions` without de-duping by `value`. Since `SORT_SERIES_CHOICES` uses values like `name`/`sum`/`min`/`max`/`avg`, it’s easy for a real column/metric label to collide (e.g. an x-axis column named `name`), producing duplicate option values with different labels and ambiguous selections. Consider de-duplicating by `value` or namespacing values to keep them unique.
```suggestion
// Combine axis and multi-series options, de-duplicating by `value`.
// Axis/metric-based options take precedence over generic series options
// when there is a collision.
const optionMap = new Map<string, { value: string; label: string }>();
axisSortOptions.forEach(option => {
optionMap.set(option.value, option);
});
multiSeriesOptions.forEach(option => {
if (!optionMap.has(option.value)) {
optionMap.set(option.value, option);
}
});
const options = Array.from(optionMap.values());
```
--
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] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "sfirke (via GitHub)" <gi...@apache.org>.
sfirke commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3915976974
Hello! Yesterday was a work holiday for a lot of the USA/Canada so I haven't had a a chance to take a look. Will do so this week
--
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] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "YousufFFFF (via GitHub)" <gi...@apache.org>.
YousufFFFF commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3915950946
Hi @sfirke!
Could you please let me know if any changes are required from my side?
--
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] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-4770802161
@YousufFFFF the blocker I flagged is still here: `mapXAxisSortToSeriesType` pattern-matches the chosen value into one of five `SortSeriesType` aggregations, so when the user sorts by an actual axis column or metric it returns `undefined` and `xAxisSortSeries` silently falls back to default (Sum) ordering — which is exactly the #34352 case. The substring match is fragile too (`total_summary`→Sum, `customer_name`→Name). I think the sort path needs to carry the chosen column, not a string we reverse-engineer into an enum. Can't land it as-is.
--
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] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-5086758855
Still blocked on the thing I flagged: `mapXAxisSortToSeriesType` can't express sorting by an arbitrary column, it just falls back to Sum ordering when the match fails. That needs to be fixed before a rebase off `master` is worth doing.
--
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] fix(bar chart): X-Axis Sort By behavior when Dimensions are set [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3858309865
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] fix(bar chart): X-Axis Sort By behavior when Dimensions are set [superset]
Posted by "YousufFFFF (via GitHub)" <gi...@apache.org>.
YousufFFFF commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3860220884
Hi @SBIN2010!
I’m seeing failures in the Storybook and Playwright CI jobs (test-storybook, playwright-tests, experimental Playwright).
From the logs, these appear to be CI/workflow issues (e.g. missing test-storybook:ci script) rather than failures caused by this change. Frontend build, linting, and Jest/unit tests are passing.
Happy to investigate further if needed, otherwise please let me know if these can be rerun or ignored. Thanks!
--
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] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3898716744
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] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3898330154
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] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "YousufFFFF (via GitHub)" <gi...@apache.org>.
YousufFFFF commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3904713485
Hi @sfirke!
This PR is now ready for review. I've addressed the X-Axis Sort By inconsistency issue and added comprehensive test coverage.
All pre-commits and CI checks are passing.
Let me know if you'd like me to make any adjustments!
--
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] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-5434934387
@YousufFFFF this one's now conflicting with `master`, and the core issue is still open: `mapXAxisSortToSeriesType` can't express sorting by an arbitrary column, it just falls back to Sum ordering when the match fails, so #34352 isn't actually fixed. @sfirke pinged you last week asking if you're still up for continuing, no word yet. If not, happy to adopt this on a fresh branch. I'll convert it to draft for now while it awaits updates. Say the word if you want us to jump in here with some commits and attempt to make it mergeable.
--
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] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "sfirke (via GitHub)" <gi...@apache.org>.
sfirke commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-5648342820
Thanks @rusackas. This would be a big one to cherry in to a possible 7.0 RC if we can swing 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] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "YousufFFFF (via GitHub)" <gi...@apache.org>.
YousufFFFF commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3916000666
Thanks, appreciate it. Looking forward to your feedback.
--
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] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "sfirke (via GitHub)" <gi...@apache.org>.
sfirke commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3930160381
Argh I'm having trouble accessing the showtime environment, my work computer blocks it and I don't understand why my personal laptop can't get on there. I'll try another PC soon.
--
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] fix(bar chart): X-Axis Sort By behavior when Dimensions are set [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 #37712:
URL: https://github.com/apache/superset/pull/37712#discussion_r2773496970
##########
superset-frontend/packages/superset-ui-chart-controls/test/shared-controls/xAxisSortControl.test.ts:
##########
@@ -0,0 +1,100 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { ControlPanelState } from '../../src/types';
+import { xAxisSortControl } from '../../src/shared-controls/customControls';
+
+const createState = (
+ overrides: Partial<ControlPanelState>,
+): ControlPanelState =>
+ ({
+ slice: { slice_id: 1 },
+ form_data: {},
+ datasource: {
+ column_formats: {},
+ verbose_map: {},
+ },
+ controls: {},
+ common: {},
+ metadata: {},
+ ...overrides,
+ }) as ControlPanelState;
+
+const createControlState = (value: unknown = undefined) => ({ value }) as any;
+
+test('xAxisSortControl includes axis and metric options when there is no dimension', () => {
+ const state = createState({
+ controls: {
+ x_axis: { value: 'ds', type: 'Select' },
+ groupby: { value: [], type: 'Select' },
+ metrics: { value: ['metric_1'], type: 'Select' },
+ timeseries_limit_metric: { value: 'sort_metric', type: 'Select' },
+ datasource: {
+ datasource: {
+ columns: [],
+ metrics: [],
+ },
+ type: 'Select',
+ },
+ },
+ });
+
+ const { options } = xAxisSortControl.config.mapStateToProps!(
+ state,
+ createControlState(),
+ ) as { options: { value: string }[] };
+
+ const values = options.map(opt => opt.value);
+
+ expect(values).toContain('ds');
+ expect(values).toContain('metric_1');
+});
+
+test('xAxisSortControl keeps axis and metric options when a dimension is set', () => {
+ const state = createState({
+ controls: {
+ x_axis: { value: 'ds', type: 'Select' },
+ groupby: { value: ['dim'], type: 'Select' },
+ metrics: { value: ['metric_1'], type: 'Select' },
+ timeseries_limit_metric: { value: 'sort_metric', type: 'Select' },
+ datasource: {
+ datasource: {
+ columns: [],
+ metrics: [],
Review Comment:
**Suggestion:** In the second test, the groupby dimension (`'dim'`), x-axis (`'ds'`), and metrics (`'metric_1'`, `'sort_metric'`) are referenced in control values, but the mocked `datasource` again has empty `columns` and `metrics` arrays; this inconsistency means any logic that relies on datasource metadata for these fields may receive `undefined`, leading to runtime errors or a test that doesn't faithfully match real control panel state. Populate the mocked `columns` and `metrics` arrays to align with the control values. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Tests for xAxisSortControl may be flaky.
- ❌ False negatives hide dimension-related regressions.
- ⚠️ CI feedback delayed by inaccurate unit tests.
```
</details>
```suggestion
columns: [{ column_name: 'ds' }, { column_name: 'dim' }],
metrics: [
{ metric_name: 'metric_1' },
{ metric_name: 'sort_metric' },
],
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the test file
superset-frontend/packages/superset-ui-chart-controls/test/shared-controls/xAxisSortControl.test.ts
(`yarn test` or `jest`) which executes the second test starting at line 69.
2. The test constructs a mocked ControlPanelState at lines 70-84 where groupby includes
'dim' and metrics include 'metric_1' but datasource.datasource.columns and metrics arrays
are empty (lines 76-80).
3. The test calls xAxisSortControl.config.mapStateToProps! at line 86 which may consult
datasource metadata to build the options list for X-Axis Sort By.
4. Because the mocked metadata lacks entries for 'ds', 'dim', 'metric_1', and
'sort_metric', lookups inside mapStateToProps will return undefined or omit expected
options; running the test will either fail the expect() assertions at lines 94-99 or not
exercise the intended code paths. Adding realistic mocked columns/metrics reproduces the
real control flow and validates the intended behavior.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/packages/superset-ui-chart-controls/test/shared-controls/xAxisSortControl.test.ts
**Line:** 78:79
**Comment:**
*Logic Error: In the second test, the groupby dimension (`'dim'`), x-axis (`'ds'`), and metrics (`'metric_1'`, `'sort_metric'`) are referenced in control values, but the mocked `datasource` again has empty `columns` and `metrics` arrays; this inconsistency means any logic that relies on datasource metadata for these fields may receive `undefined`, leading to runtime errors or a test that doesn't faithfully match real control panel state. Populate the mocked `columns` and `metrics` arrays to align with the control values.
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.
```
</details>
##########
superset-frontend/packages/superset-ui-chart-controls/test/shared-controls/xAxisSortControl.test.ts:
##########
@@ -0,0 +1,100 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { ControlPanelState } from '../../src/types';
+import { xAxisSortControl } from '../../src/shared-controls/customControls';
+
+const createState = (
+ overrides: Partial<ControlPanelState>,
+): ControlPanelState =>
+ ({
+ slice: { slice_id: 1 },
+ form_data: {},
+ datasource: {
+ column_formats: {},
+ verbose_map: {},
+ },
+ controls: {},
+ common: {},
+ metadata: {},
+ ...overrides,
+ }) as ControlPanelState;
+
+const createControlState = (value: unknown = undefined) => ({ value }) as any;
+
+test('xAxisSortControl includes axis and metric options when there is no dimension', () => {
+ const state = createState({
+ controls: {
+ x_axis: { value: 'ds', type: 'Select' },
+ groupby: { value: [], type: 'Select' },
+ metrics: { value: ['metric_1'], type: 'Select' },
+ timeseries_limit_metric: { value: 'sort_metric', type: 'Select' },
+ datasource: {
+ datasource: {
+ columns: [],
+ metrics: [],
Review Comment:
**Suggestion:** In the first test, the control state references an x-axis column (`'ds'`) and metrics (`'metric_1'`, `'sort_metric'`), but the mocked `datasource` metadata has empty `columns` and `metrics` arrays; any control logic that looks up these columns/metrics by name in the datasource will get `undefined`, causing errors or making the test fail to exercise the real behavior. Populate the mocked `columns` and `metrics` with entries matching the control values. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Tests for xAxisSortControl may be flaky.
- ❌ Incorrect test assertions hide X-Axis sort regressions.
- ⚠️ Dev CI runs produce misleading failures.
```
</details>
```suggestion
columns: [{ column_name: 'ds' }],
metrics: [
{ metric_name: 'metric_1' },
{ metric_name: 'sort_metric' },
],
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run unit tests that include this file:
superset-frontend/packages/superset-ui-chart-controls/test/shared-controls/xAxisSortControl.test.ts
(execute `yarn test` or `jest`).
2. Test execution starts the first test at line 41 which calls createState(...) to build
the mocked ControlPanelState (file:
superset-frontend/packages/superset-ui-chart-controls/test/shared-controls/xAxisSortControl.test.ts:41-56).
3. The test then calls xAxisSortControl.config.mapStateToProps! at line 58; this function
(imported from ../../src/shared-controls/customControls) often inspects
datasource.datasource.columns and datasource.datasource.metrics to resolve names like 'ds'
and 'metric_1'.
4. Because the mocked datasource has columns: [] and metrics: [] (lines 49-51), any lookup
by name (e.g., resolving 'ds' or 'metric_1') will return undefined; this either (a) causes
mapStateToProps to produce different options than expected (making the assertions at lines
65-66 invalid), or (b) throws when code assumes metadata existence — both outcomes cause
test failures or false positives. Reproducing the failure is done by running the single
test file and observing mismatched options or an exception from mapStateToProps.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/packages/superset-ui-chart-controls/test/shared-controls/xAxisSortControl.test.ts
**Line:** 50:51
**Comment:**
*Logic Error: In the first test, the control state references an x-axis column (`'ds'`) and metrics (`'metric_1'`, `'sort_metric'`), but the mocked `datasource` metadata has empty `columns` and `metrics` arrays; any control logic that looks up these columns/metrics by name in the datasource will get `undefined`, causing errors or making the test fail to exercise the real behavior. Populate the mocked `columns` and `metrics` with entries matching the control values.
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.
```
</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] fix(bar chart): X-Axis Sort By behavior when Dimensions are set [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3859984159
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] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #37712:
URL: https://github.com/apache/superset/pull/37712#discussion_r2792689783
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Bar/transformProps.test.ts:
##########
@@ -351,4 +351,55 @@ describe('Bar Chart X-axis Time Formatting', () => {
expect(chartProps.formData.xAxisTimeFormat).toBe('smart_date');
});
});
+
+ describe('X-Axis sorting with dimensions (series ordering)', () => {
+ it('reorders series correctly when xAxisSort is set to sum', () => {
+ const sortingFormData = {
+ ...baseFormData,
+ groupby: ['category'],
+ metrics: ['Sales', 'Marketing'],
+ xAxisSort: 'sum',
+ xAxisSortAsc: false,
+ };
+
+ const sortingData = [
+ {
+ data: [
+ {
+ Sales: 10,
+ Marketing: 5,
+ category: 'A',
+ __timestamp: 1609459200000,
+ },
+ {
+ Sales: 2,
+ Marketing: 12,
+ category: 'B',
+ __timestamp: 1609459200000,
+ },
+ ],
+ colnames: ['Sales', 'Marketing', 'category', '__timestamp'],
+ coltypes: ['BIGINT', 'BIGINT', 'STRING', 'TIMESTAMP'],
+ },
+ ];
+
+ const chartProps = new ChartProps({
+ width: 800,
+ height: 600,
+ formData: sortingFormData,
+ queriesData: sortingData,
+ theme: supersetTheme,
+ }) as unknown as EchartsTimeseriesChartProps;
+
+ const transformedProps = transformProps(chartProps);
+
+ const series = transformedProps.echartOptions.series as any[];
+
+ // The sum of Sales across points:
+ // A: 10, B: 2
+ expect(series[0].name).toContain('Marketing');
+ expect(series[1].name).toContain('Sales');
+ // Next lower sum metric
+ });
+ });
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Test expects unimplemented series sorting by xAxisSort</b></div>
<div id="fix">
The test sets xAxisSort to 'sum' and expects series to be ordered by their total sum descending (Marketing first, then Sales), but the current code sorts series alphabetically by name. This test will fail with the existing implementation. The intended behavior appears to be that xAxisSort controls series ordering in multi-series charts, so transformProps needs to override sortSeriesType with the mapped xAxisSort value when multiple series are present.
</div>
</div>
<small><i>Code Review Run #df9ed9</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] fix(bar chart): X-Axis Sort By behavior when Dimensions are set [superset]
Posted by "YousufFFFF (via GitHub)" <gi...@apache.org>.
YousufFFFF commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3858326079
I’m running pre-commit on all files locally now and will push an update shortly.
Initially I ran it only on the changed files, which passed locally, but CI is enforcing the full pre-commit run.
--
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] fix(bar chart): X-Axis Sort By behavior when Dimensions are set [superset]
Posted by "YousufFFFF (via GitHub)" <gi...@apache.org>.
YousufFFFF commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3873404417
Thanks for the review!
As mentioned in the PR description, this change ensures that when a dimension (groupby) is added,
the X-Axis Sort By options remain consistent and continue to expose other selected columns.
The demo video I shared focuses on the dimension case. You’re right that it doesn’t currently
show an example where a column is added via the “Sort By” (Query Sort By) control
(e.g. sorting DayName by Day_Number).
I can update the video to cover both scenarios:
• sorting DayName by Day_Number when a dimension is present
• demonstrating a value added via the “Sort By” control appearing in X-Axis Sort By
Please let me know if you’d like that additional demo or any further adjustments.
--
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] Fix Bar Chart X-Axis Sort By behavior when Dimensions are set [superset]
Posted by "YousufFFFF (via GitHub)" <gi...@apache.org>.
YousufFFFF commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3858251613
I tried updating the PR title to match the conventional format, but it doesn’t seem to be taking effect from my side (even when attempting via the GitHub CLI).
--
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] fix(bar chart): X-Axis Sort By behavior when Dimensions are set [superset]
Posted by "YousufFFFF (via GitHub)" <gi...@apache.org>.
YousufFFFF commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3866234067
Hello reviewers, while fixing the test failures I added a testbook dependency in package.json, which may have triggered review requests for everyone. Sorry for the inconvenience.
--
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] fix(bar chart): X-Axis Sort By behavior when Dimensions are set [superset]
Posted by "YousufFFFF (via GitHub)" <gi...@apache.org>.
YousufFFFF commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3866240726
Hi @kgabryje , @alexandrusoare! This PR is ready for review.
All CI checks are passing now after reruns.
--
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] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3904722275
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/22038580723) for [240c741](https://github.com/apache/superset/commit/240c741)
--
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] fix(bar chart): X-Axis Sort By behavior when Dimensions are set [superset]
Posted by "sfirke (via GitHub)" <gi...@apache.org>.
sfirke commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3873248923
@YousufFFFF I can try out this fix in the ephemeral environment that I just triggered to start, but I watched your video a few times and I don't think it addresses the case in #34352. There the user adds another variable to "SORT BY" / QUERY SORT BY (I think it got renamed for 6.0.0) and it becomes visible in the X-AXIS SORT BY. But in your video you're not using a variable from the SORT BY field.
For instance take the case you write about:
> Currently, when a dimension is added, the available sort options become restricted and prevent users from sorting the categorical axis by another numeric column (e.g. sorting DayName by Day_Number).
The video doesn't illustrate that this is now possible.
--
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] fix(bar chart): X-Axis Sort By behavior when Dimensions are set [superset]
Posted by "sfirke (via GitHub)" <gi...@apache.org>.
sfirke commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3873735866
Oh I see, this was meant to be UI-only? Let's fix this all in one PR if you're up for it. Having the feature actually working would be more motivating for reviewing + merging this.
--
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] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3897184461
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] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3916529525
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/22111381130) for [240c741](https://github.com/apache/superset/commit/240c741)
--
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] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-5787018605
@YousufFFFF the fresh PR is up at https://github.com/apache/superset/pull/44550. It carries the selected field through the grouped sort path (resolving the metric's pivoted columns via `label_map`) instead of mapping it back to an enum, and you're credited as co-author for the repro and control work. Closing this one in favor of it, thanks for getting it this far!
--
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] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas closed pull request #37712: fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions
URL: https://github.com/apache/superset/pull/37712
--
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] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "sfirke (via GitHub)" <gi...@apache.org>.
sfirke commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-5282064701
Hi @YousufFFFF are you still interested in working on this or are you open to someone else continuing this work?
--
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] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "YousufFFFF (via GitHub)" <gi...@apache.org>.
YousufFFFF commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3935707245
Ahh that’s frustrating 😅 I can totally relate! Environment issues can be really annoying. Hopefully the other PC works smoothly. Let me know if I can help from my side.
--
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] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "YousufFFFF (via GitHub)" <gi...@apache.org>.
YousufFFFF commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3885037989
> Thank you @YousufFFFF ! Can you try to get that pre-commit check passing?
Sure working on that!
--
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] Fix Bar Chart X-Axis Sort By behavior when Dimensions are set [superset]
Posted by "YousufFFFF (via GitHub)" <gi...@apache.org>.
YousufFFFF commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3858253915
@sfirke , would you mind updating the title? Happy to adjust anything else.
--
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] fix(bar chart): X-Axis Sort By behavior when Dimensions are set [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3860385423
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] Fix Bar Chart X-Axis Sort By behavior when Dimensions are set [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3855997059
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] fix(bar chart): X-Axis Sort By behavior when Dimensions are set [superset]
Posted by "YousufFFFF (via GitHub)" <gi...@apache.org>.
YousufFFFF commented on code in PR #37712:
URL: https://github.com/apache/superset/pull/37712#discussion_r2775942636
##########
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/customControls.tsx:
##########
@@ -146,37 +146,44 @@ export const xAxisSortControl = {
const columns = [controls?.x_axis?.value as QueryFormColumn].filter(
Boolean,
);
- const isSingleSortAvailable =
- ensureIsArray(controls?.groupby?.value).length === 0;
- const isMultiSortAvailable =
- !!ensureIsArray(controls?.groupby?.value).length ||
+ const hasGroupBy = ensureIsArray(controls?.groupby?.value).length > 0;
+ const hasMultipleMetrics =
ensureIsArray(controls?.metrics?.value).length > 1;
+ const isMultiSortAvailable = hasGroupBy || hasMultipleMetrics;
const metrics = [
...ensureIsArray(controls?.metrics?.value as QueryFormMetric),
controls?.timeseries_limit_metric?.value as QueryFormMetric,
].filter(Boolean);
const metricLabels = [...new Set(metrics.map(getMetricLabel))];
- const options = [
- ...(isSingleSortAvailable
- ? [
- ...columns.map(column => {
- const value = getColumnLabel(column);
- return { value, label: dataset?.verbose_map?.[value] || value };
- }),
- ...metricLabels.map(value => ({
- value,
- label: dataset?.verbose_map?.[value] || value,
- })),
- ]
- : []),
- ...(isMultiSortAvailable
- ? SORT_SERIES_CHOICES.map(choice => ({
- value: choice[0],
- label: choice[1],
- }))
- : []),
+
+ // Base axis sort options: always include the explicit axis column and metrics,
+ // regardless of whether a dimension (groupby) is present. This keeps the
+ // "X-Axis Sort By" dropdown consistent when toggling dimensions.
+ const axisSortOptions = [
+ ...columns.map(column => {
Review Comment:
Thanks for the note - agreed that in multi-series mode the ECharts pipeline interprets
xAxisSort as a SortSeriesType, and supporting arbitrary metric/axis values there would
require additional encoding/mapping and consumer changes.
To keep this PR scoped, the intent here is limited to keeping the “X-Axis Sort By”
dropdown options consistent when toggling dimensions, without changing the underlying
sorting semantics. Happy to follow up with a separate PR if we decide to expand support
for metric-based sorting with dimensions.
--
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] fix(bar chart): X-Axis Sort By behavior when Dimensions are set [superset]
Posted by "sfirke (via GitHub)" <gi...@apache.org>.
sfirke commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3873527775
Yes, please add an example showing that first bullet point. I just tested out this PR and I think it doesn't work. I'm including screenshots to describe what I'm seeing.
**Before sorting control is added**
The chart is sorted alphabetically by genre:
<img width="2180" height="1159" alt="image" src="https://github.com/user-attachments/assets/199bc47f-da94-45d6-8fa0-734d6291e56f" />
**After sorting control is added**
Now it's sorted by sum of North America sales:
--
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] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3904792907
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] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37712:
URL: https://github.com/apache/superset/pull/37712#issuecomment-3901560642
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] fix(echarts-bar): make X-Axis Sort By consistent and functional with dimensions [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #37712:
URL: https://github.com/apache/superset/pull/37712#discussion_r3827419843
##########
superset-frontend/packages/superset-ui-chart-controls/test/shared-controls/xAxisSortControl.test.ts:
##########
@@ -0,0 +1,100 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { ControlPanelState } from '../../src/types';
+import { xAxisSortControl } from '../../src/shared-controls/customControls';
+
+const createState = (
+ overrides: Partial<ControlPanelState>,
+): ControlPanelState =>
+ ({
+ slice: { slice_id: 1 },
+ form_data: {},
+ datasource: {
+ column_formats: {},
+ verbose_map: {},
+ },
+ controls: {},
+ common: {},
+ metadata: {},
+ ...overrides,
+ }) as ControlPanelState;
+
+const createControlState = (value: unknown = undefined) => ({ value }) as any;
+
+test('xAxisSortControl includes axis and metric options when there is no dimension', () => {
+ const state = createState({
+ controls: {
+ x_axis: { value: 'ds', type: 'Select' },
+ groupby: { value: [], type: 'Select' },
+ metrics: { value: ['metric_1'], type: 'Select' },
+ timeseries_limit_metric: { value: 'sort_metric', type: 'Select' },
+ datasource: {
+ datasource: {
+ columns: [],
+ metrics: [],
+ },
+ type: 'Select',
+ },
+ },
+ });
+
+ const { options } = xAxisSortControl.config.mapStateToProps!(
+ state,
+ createControlState(),
+ ) as { options: { value: string }[] };
+
+ const values = options.map(opt => opt.value);
+
+ expect(values).toContain('ds');
+ expect(values).toContain('metric_1');
+});
+
+test('xAxisSortControl keeps axis and metric options when a dimension is set', () => {
+ const state = createState({
+ controls: {
+ x_axis: { value: 'ds', type: 'Select' },
+ groupby: { value: ['dim'], type: 'Select' },
+ metrics: { value: ['metric_1'], type: 'Select' },
+ timeseries_limit_metric: { value: 'sort_metric', type: 'Select' },
+ datasource: {
+ datasource: {
+ columns: [],
+ metrics: [],
+ },
+ type: 'Select',
+ },
+ },
+ });
+
+ const { options } = xAxisSortControl.config.mapStateToProps!(
+ state,
+ createControlState(),
+ ) as { options: { value: string }[] };
+
+ const values = options.map(opt => opt.value);
+
+ // Axis and metric choices should still be present
+ expect(values).toContain('ds');
+ expect(values).toContain('metric_1');
+
+ // And multi-series sort choices should also be available
Review Comment:
Agreed—when a grouped chart selects a column such as `Day_Number`, this mapping returns no sort, while metric labels can be converted to an unrelated aggregate. Could we either keep these choices out of multi-series mode or carry the selected field through the grouped sorting path?
--
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