Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Axis sort in the Bar Chart V2 #21993

Merged
merged 14 commits into from
Nov 26, 2022

Conversation

zhaoyongjie
Copy link
Member

@zhaoyongjie zhaoyongjie commented Nov 1, 2022

Background

This PR intends to introduce a new control that XAxisSortControl and a new post-processing operator. The goal is to resolve whether the axis could be sorted by the x-axis or any metric.

Before introducing this feature, let's talk about some of the current sorting logic. There are two kinds of sorting logic on the current master branch.

  1. order by on the main query if Sort By is set on the control panel

image

  1. order by on the main query and the sub-query if Series limit and Dimensions is given.

image

Both of these sorting methods are sorting on query data results, which means that these sorting occurs at the database stage. The limitation of this approach is that we cannot sort the pivot results. For example, timeseries charts(echart version) are pivoted charts.

Purpose

Imagine that there are request for such analysis, but the data sorting is kept to a fixed metric(in the other words, the database should return static topN):

  1. Bar chart should sort the bar by one of several metrics, for example, there are sum(sales), max(sales) in the metrics, but the max(sales) should be sorted.
  2. Bar chart should sort the bar by the x-axis values.

This PR is designed for this purpose.

Limitation

The current design doesn't support multiple series(dimensions control in the control panel). The reason is that the extra query should be provided for this use case.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

image

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@zhaoyongjie zhaoyongjie force-pushed the fix/barchart_sort branch 2 times, most recently from 265654e to 72c9d96 Compare November 2, 2022 16:17
@pull-request-size pull-request-size bot added size/XL and removed size/L labels Nov 3, 2022
@zhaoyongjie zhaoyongjie force-pushed the fix/barchart_sort branch 2 times, most recently from bc48011 to 31ecef9 Compare November 4, 2022 09:49
@zhaoyongjie zhaoyongjie marked this pull request as ready for review November 4, 2022 09:50
def sort(df: DataFrame, columns: Dict[str, bool]) -> DataFrame:
# pylint: disable=invalid-name
@validate_column_args("by")
def sort(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is useless as before so redesigning the interface is safe. The interface refers to Pandas.

@codecov
Copy link

codecov bot commented Nov 4, 2022

Codecov Report

Merging #21993 (c225baa) into master (b2fcdc5) will decrease coverage by 0.07%.
The diff coverage is 52.83%.

@@            Coverage Diff             @@
##           master   #21993      +/-   ##
==========================================
- Coverage   66.92%   66.85%   -0.08%     
==========================================
  Files        1834     1841       +7     
  Lines       69986    70197     +211     
  Branches     7612     7662      +50     
==========================================
+ Hits        46837    46927      +90     
- Misses      21183    21296     +113     
- Partials     1966     1974       +8     
Flag Coverage Δ
hive 52.56% <33.33%> (-0.01%) ⬇️
javascript 53.67% <36.84%> (-0.08%) ⬇️
mysql 78.07% <73.33%> (-0.01%) ⬇️
postgres 78.14% <73.33%> (-0.01%) ⬇️
presto 52.46% <33.33%> (-0.01%) ⬇️
python 81.34% <93.33%> (+<0.01%) ⬆️
sqlite 76.60% <73.33%> (-0.01%) ⬇️
unit 50.87% <93.33%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...superset-ui-core/src/query/types/PostProcessing.ts 100.00% <ø> (ø)
...ins/plugin-chart-echarts/src/BoxPlot/buildQuery.ts 66.66% <ø> (ø)
...s/plugin-chart-echarts/src/BoxPlot/controlPanel.ts 5.55% <ø> (ø)
...charts/src/Timeseries/Regular/Bar/controlPanel.tsx 35.71% <ø> (+4.46%) ⬆️
.../plugin-chart-echarts/src/Timeseries/buildQuery.ts 71.42% <ø> (ø)
.../plugin-chart-pivot-table/src/plugin/buildQuery.ts 42.85% <ø> (ø)
...ugin-chart-pivot-table/src/plugin/controlPanel.tsx 4.16% <0.00%> (ø)
...ntend/plugins/plugin-chart-table/src/buildQuery.ts 56.25% <ø> (ø)
...nd/plugins/plugin-chart-table/src/controlPanel.tsx 53.12% <0.00%> (ø)
...c/explore/components/controls/XAxisSortControl.tsx 0.00% <0.00%> (ø)
... and 28 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@zhaoyongjie zhaoyongjie changed the title [WIP]feat: Axis sort in the Bar Chart V2 feat: Axis sort in the Bar Chart V2 Nov 7, 2022
@zhaoyongjie zhaoyongjie marked this pull request as draft November 7, 2022 15:35
@zhaoyongjie zhaoyongjie force-pushed the fix/barchart_sort branch 2 times, most recently from ab25b4e to d60566a Compare November 9, 2022 03:50
@zhaoyongjie zhaoyongjie changed the title feat: Axis sort in the Bar Chart V2 [WIP]feat: Axis sort in the Bar Chart V2 Nov 9, 2022
@zhaoyongjie zhaoyongjie marked this pull request as ready for review November 9, 2022 12:46
@pull-request-size pull-request-size bot added size/L and removed size/XL labels Nov 9, 2022
@zhaoyongjie zhaoyongjie changed the title [WIP]feat: Axis sort in the Bar Chart V2 feat: Axis sort in the Bar Chart V2 Nov 9, 2022
@zhaoyongjie zhaoyongjie force-pushed the fix/barchart_sort branch 3 times, most recently from 23bfc01 to 53df1e0 Compare November 13, 2022 02:49
@apache apache deleted a comment from stephenLYZ Nov 26, 2022
@apache apache deleted a comment from github-actions bot Nov 26, 2022
Comment on lines -32 to -44
{
name: 'contributionMode',
config: {
type: 'SelectControl',
label: t('Contribution Mode'),
default: null,
choices: [
[null, t('None')],
[ContributionType.Row, t('Row')],
[ContributionType.Column, t('Series')],
],
description: t('Calculate contribution per series or row'),
},
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move contributionMode into CustomControls.ts

@@ -354,7 +354,7 @@ const show_empty_columns: SharedControlConfig<'CheckboxControl'> = {
description: t('Show empty columns'),
};

const datetime_columns_lookup: SharedControlConfig<'HiddenControl'> = {
const temporal_columns_lookup: SharedControlConfig<'HiddenControl'> = {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bycatch: rename the datetime_columns_lookup into temporal_columns_lookup.

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is amazing and probably one of the most requested features over time! Tested to work as expected. Just a few nits, and wondering if we couldn't enable this for the other chart types, too (e.g. line), as that will probably be equally requested.

isDefined(formData?.x_axis_sort_asc) &&
labelsInMetricsAndXAxis.includes(formData.x_axis_sort) &&
// the sort operator doesn't support sort-by multiple series.
isEmpty(formData.groupby)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought for the future: As formData will always be unstandardized (can contain more or less any property names), it will be difficult to harmonize functionality based on formData. So at some point we should move to using a base queryObject, which has already been converted to specific format (columns for dimensional grouping).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interestingly, these operators are used to "build" part of Query Object. I recently found that we might only use formData to generate an operator and don't use the QueryObject.

However, we need to consider more and more to see how to make an efficient and convenient abstraction on the queryObject generation.

superset/utils/pandas_postprocessing/utils.py Outdated Show resolved Hide resolved
@zhaoyongjie zhaoyongjie merged commit 22fab5e into apache:master Nov 26, 2022
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@sfirke sfirke mentioned this pull request Jan 10, 2023
@Cmagno13
Copy link

Sorting on the 2v bar graph is not working. Any suggestions for correction.

@Cmagno13
Copy link

000000image

@sunsikai
Copy link

sunsikai commented Mar 8, 2023

Mix chart need Axis sort too.

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XL 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants