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.

Sign up for GitHub

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

fix(plugin-chart-echarts): support adhoc x-axis #20055

Merged
merged 2 commits into from
May 13, 2022
Merged

fix(plugin-chart-echarts): support adhoc x-axis #20055

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3ca3b82
fix(plugin-chart-echarts): support adhoc x-axis
villebro May 13, 2022
1da6428
simplify code
villebro May 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion superset-frontend/packages/superset-ui-chart-controls/src/operators/pivotOperator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,13 @@ export const pivotOperator: PostProcessingFactory<PostProcessingPivot> = (
) => {
const metricLabels = ensureIsArray(queryObject.metrics).map(getMetricLabel);
const { x_axis: xAxis } = formData;

if ((xAxis || queryObject.is_timeseries) && metricLabels.length) {
const index = [getColumnLabel(formData.x_axis || DTTM_ALIAS)];
villebro marked this conversation as resolved.
Show resolved Hide resolved
return {
operation: 'pivot',
options: {
index: [xAxis || DTTM_ALIAS],
index,
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure which is better/more readable, inlining it or declaring it as a variable first. I went with declaring a const first, but looking at it now I should maybe have inlined it..

Copy link
Member

Choose a reason for hiding this comment

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

I think this looks good 👍

columns: ensureIsArray(queryObject.columns).map(getColumnLabel),
// Create 'dummy' mean aggregates to assign cell values in pivot table
// use the 'mean' aggregates to avoid drop NaN. PR: https://github.com/apache-superset/superset-ui/pull/1231
Expand Down
9 changes: 7 additions & 2 deletions superset-frontend/packages/superset-ui-chart-controls/src/operators/prophetOperator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,18 @@
* specific language governing permissions and limitationsxw
* under the License.
*/
import { DTTM_ALIAS, PostProcessingProphet } from '@superset-ui/core';
import {
DTTM_ALIAS,
getColumnLabel,
PostProcessingProphet,
} from '@superset-ui/core';
import { PostProcessingFactory } from './types';

export const prophetOperator: PostProcessingFactory<PostProcessingProphet> = (
formData,
queryObject,
) => {
const index = getColumnLabel(formData.x_axis || DTTM_ALIAS);
if (formData.forecastEnabled) {
return {
operation: 'prophet',
Expand All @@ -33,7 +38,7 @@ export const prophetOperator: PostProcessingFactory<PostProcessingProphet> = (
yearly_seasonality: formData.forecastSeasonalityYearly,
weekly_seasonality: formData.forecastSeasonalityWeekly,
daily_seasonality: formData.forecastSeasonalityDaily,
index: formData.x_axis || DTTM_ALIAS,
index,
},
};
}
Expand Down
3 changes: 2 additions & 1 deletion ...et-frontend/packages/superset-ui-chart-controls/src/operators/timeComparePivotOperator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,12 @@ export const timeComparePivotOperator: PostProcessingFactory<PostProcessingPivot
{ operator: 'mean' as NumpyFunction },
]),
);
const index = [getColumnLabel(formData.x_axis || DTTM_ALIAS)];

return {
operation: 'pivot',
options: {
index: [formData.x_axis || DTTM_ALIAS],
index,
columns: ensureIsArray(queryObject.columns).map(getColumnLabel),
drop_missing_columns: false,
flatten_columns: false,
Expand Down
32 changes: 32 additions & 0 deletions ...t-frontend/packages/superset-ui-chart-controls/test/utils/operators/pivotOperator.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,3 +136,35 @@ test('pivot by x_axis with groupby', () => {
},
});
});

test('pivot by adhoc x_axis', () => {
expect(
pivotOperator(
{
...formData,
x_axis: {
label: 'my_case_expr',
expressionType: 'SQL',
expression: 'case when a = 1 then 1 else 0 end',
},
},
{
...queryObject,
columns: ['foo', 'bar'],
},
),
).toEqual({
operation: 'pivot',
options: {
index: ['my_case_expr'],
columns: ['foo', 'bar'],
aggregates: {
'count(*)': { operator: 'mean' },
'sum(val)': { operator: 'mean' },
},
drop_missing_columns: false,
flatten_columns: false,
reset_index: false,
},
});
});
33 changes: 33 additions & 0 deletions ...frontend/packages/superset-ui-chart-controls/test/utils/operators/prophetOperator.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,3 +98,36 @@ test('should do prophetOperator over named column', () => {
},
});
});

test('should do prophetOperator over adhoc column', () => {
expect(
prophetOperator(
{
...formData,
x_axis: {
label: 'my_case_expr',
expressionType: 'SQL',
expression: 'case when a = 1 then 1 else 0 end',
},
forecastEnabled: true,
forecastPeriods: '3',
forecastInterval: '5',
forecastSeasonalityYearly: true,
forecastSeasonalityWeekly: false,
forecastSeasonalityDaily: false,
},
queryObject,
),
).toEqual({
operation: 'prophet',
options: {
time_grain: 'P1Y',
periods: 3.0,
confidence_interval: 5.0,
yearly_seasonality: true,
weekly_seasonality: false,
daily_seasonality: false,
index: 'my_case_expr',
},
});
});
41 changes: 41 additions & 0 deletions ...packages/superset-ui-chart-controls/test/utils/operators/timeComparePivotOperator.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,3 +135,44 @@ test('should pivot on x-axis', () => {
},
});
});

test('should pivot on adhoc x-axis', () => {
expect(
timeComparePivotOperator(
{
...formData,
comparison_type: 'values',
time_compare: ['1 year ago', '1 year later'],
x_axis: {
label: 'my_case_expr',
expressionType: 'SQL',
expression: 'case when a = 1 then 1 else 0 end',
},
},
queryObject,
),
).toEqual({
operation: 'pivot',
options: {
aggregates: {
'count(*)': { operator: 'mean' },
'count(*)__1 year ago': { operator: 'mean' },
'count(*)__1 year later': { operator: 'mean' },
'sum(val)': {
operator: 'mean',
},
'sum(val)__1 year ago': {
operator: 'mean',
},
'sum(val)__1 year later': {
operator: 'mean',
},
},
drop_missing_columns: false,
columns: ['foo', 'bar'],
index: ['my_case_expr'],
flatten_columns: false,
reset_index: false,
},
});
});
4 changes: 3 additions & 1 deletion superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
DataRecordValue,
DTTM_ALIAS,
GenericDataType,
getColumnLabel,
getNumberFormatter,
isEventAnnotationLayer,
isFormulaAnnotationLayer,
Expand Down Expand Up @@ -141,7 +142,8 @@ export default function transformProps(

const colorScale = CategoricalColorNamespace.getScale(colorScheme as string);
const rebasedData = rebaseForecastDatum(data, verboseMap);
const xAxisCol = verboseMap[xAxisOrig] || xAxisOrig || DTTM_ALIAS;
const xAxisCol =
verboseMap[xAxisOrig] || getColumnLabel(xAxisOrig || DTTM_ALIAS);
const rawSeries = extractSeries(rebasedData, {
fillNeighborValue: stack && !forecastEnabled ? 0 : undefined,
xAxis: xAxisCol,
Expand Down