-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
Conversation
265654e
to
72c9d96
Compare
bc48011
to
31ecef9
Compare
def sort(df: DataFrame, columns: Dict[str, bool]) -> DataFrame: | ||
# pylint: disable=invalid-name | ||
@validate_column_args("by") | ||
def sort( |
There was a problem hiding this comment.
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.
31ecef9
to
fadb6d4
Compare
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
fadb6d4
to
ead16c8
Compare
ab25b4e
to
d60566a
Compare
23bfc01
to
53df1e0
Compare
8949537
to
c5a762b
Compare
c5a762b
to
100ff11
Compare
{ | ||
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'), | ||
}, |
There was a problem hiding this comment.
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'> = { |
There was a problem hiding this comment.
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
.
There was a problem hiding this 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.
superset-frontend/packages/superset-ui-chart-controls/src/operators/sortOperator.ts
Outdated
Show resolved
Hide resolved
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) |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
Ephemeral environment shutdown and build artifacts deleted. |
Sorting on the 2v bar graph is not working. Any suggestions for correction. |
Mix chart need Axis sort too. |
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 thex-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.
Sort By
is set on the control panelSeries limit
andDimensions
is given.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):
sum(sales)
,max(sales)
in the metrics, but themax(sales)
should be sorted.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
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION