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: Dashboard time grain in Pivot Table #24665

Merged
merged 2 commits into from
Jul 13, 2023
Merged

fix: Dashboard time grain in Pivot Table #24665

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5523576
fix: Dashboard time grain in Pivot Table
michael-s-molina Jul 11, 2023
1a06d5a
Adds tests
michael-s-molina Jul 11, 2023
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
9 changes: 6 additions & 3 deletions superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/buildQuery.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,10 @@ import {
import { PivotTableQueryFormData } from '../types';

export default function buildQuery(formData: PivotTableQueryFormData) {
const { groupbyColumns = [], groupbyRows = [] } = formData;
const { groupbyColumns = [], groupbyRows = [], extra_form_data } = formData;
Copy link
Member

@hughhhh hughhhh Jul 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

can we write a small unit test to for buildQuery to verify the grains are being properly set?

I ran this thru chat gpt, but it would be good to have this to make sure that queries are being built properly

import buildQuery from './path-to-your-file';

describe('buildQuery', () => {
  it('should build query with expected columns', () => {
    const formData = {
      groupbyColumns: ['column1', 'column2'],
      groupbyRows: ['row1', 'row2'],
      extra_form_data: {
        time_grain_sqla: 'monthly'
      },
      time_grain_sqla: 'weekly',
      temporal_columns_lookup: { column1: true },
      granularity_sqla: 'column1'
    };
    const result = buildQuery(formData as any);
    // Define your expected output here.
    const expected = [
      // Fill in with expected transformed columns
    ];

    expect(result).toEqual(expected);
  });
  it('should prefer extra_form_data.time_grain_sqla over formData.time_grain_sqla', () => {
    const formData = {
      groupbyColumns: ['column1'],
      groupbyRows: [],
      extra_form_data: {
        time_grain_sqla: 'monthly'
      },
      time_grain_sqla: 'weekly',
      temporal_columns_lookup: { column1: true },
      granularity_sqla: 'column1'
    };

    const result = buildQuery(formData as any);

    // Assuming that the time_grain_sqla is reflected in the 'timeGrain' property of the result
    const timeGrain = result[0]?.timeGrain;
    expect(timeGrain).toEqual('monthly');
  });

  it('should fallback to formData.time_grain_sqla if extra_form_data.time_grain_sqla is not set', () => {
    const formData = {
      groupbyColumns: ['column1'],
      groupbyRows: [],
      extra_form_data: {},
      time_grain_sqla: 'weekly',
      temporal_columns_lookup: { column1: true },
      granularity_sqla: 'column1'
    };

    const result = buildQuery(formData as any);

    // Assuming that the time_grain_sqla is reflected in the 'timeGrain' property of the result
    const timeGrain = result[0]?.timeGrain;
    expect(timeGrain).toEqual('weekly');
  });
});

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for the references @hughhhh. I added the tests with modifications.

One tip: you can ask Chat GPT to avoid using describe and it blocks to generate tests that conform to our best practices 😉

const time_grain_sqla =
extra_form_data?.time_grain_sqla || formData.time_grain_sqla;

// TODO: add deduping of AdhocColumns
const columns = Array.from(
new Set([
Expand All @@ -40,7 +43,7 @@ export default function buildQuery(formData: PivotTableQueryFormData) {
).map(col => {
if (
isPhysicalColumn(col) &&
formData.time_grain_sqla &&
time_grain_sqla &&
hasGenericChartAxes &&
/* Charts created before `GENERIC_CHART_AXES` is enabled have a different
* form data, with `granularity_sqla` set instead.
Expand All @@ -49,7 +52,7 @@ export default function buildQuery(formData: PivotTableQueryFormData) {
formData.granularity_sqla === col)
) {
return {
timeGrain: formData.time_grain_sqla,
timeGrain: time_grain_sqla,
columnType: 'BASE_AXIS',
sqlExpression: col,
label: col,
Expand Down
146 changes: 90 additions & 56 deletions superset-frontend/plugins/plugin-chart-pivot-table/test/plugin/buildQuery.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,65 +22,99 @@ import * as supersetCoreModule from '@superset-ui/core';
import buildQuery from '../../src/plugin/buildQuery';
import { PivotTableQueryFormData } from '../../src/types';

describe('PivotTableChart buildQuery', () => {
const formData: PivotTableQueryFormData = {
groupbyRows: ['row1', 'row2'],
groupbyColumns: ['col1', 'col2'],
metrics: ['metric1', 'metric2'],
tableRenderer: 'Table With Subtotal',
colOrder: 'key_a_to_z',
rowOrder: 'key_a_to_z',
aggregateFunction: 'Sum',
transposePivot: true,
rowSubtotalPosition: true,
colSubtotalPosition: true,
colTotals: true,
rowTotals: true,
valueFormat: 'SMART_NUMBER',
datasource: '5__table',
viz_type: 'my_chart',
width: 800,
height: 600,
combineMetric: false,
verboseMap: {},
columnFormats: {},
currencyFormats: {},
metricColorFormatters: [],
dateFormatters: {},
setDataMask: () => {},
legacy_order_by: 'count',
order_desc: true,
margin: 0,
const formData: PivotTableQueryFormData = {
groupbyRows: ['row1', 'row2'],
groupbyColumns: ['col1', 'col2'],
metrics: ['metric1', 'metric2'],
tableRenderer: 'Table With Subtotal',
colOrder: 'key_a_to_z',
rowOrder: 'key_a_to_z',
aggregateFunction: 'Sum',
transposePivot: true,
rowSubtotalPosition: true,
colSubtotalPosition: true,
colTotals: true,
rowTotals: true,
valueFormat: 'SMART_NUMBER',
datasource: '5__table',
viz_type: 'my_chart',
width: 800,
height: 600,
combineMetric: false,
verboseMap: {},
columnFormats: {},
currencyFormats: {},
metricColorFormatters: [],
dateFormatters: {},
setDataMask: () => {},
legacy_order_by: 'count',
order_desc: true,
margin: 0,
time_grain_sqla: TimeGranularity.MONTH,
temporal_columns_lookup: { col1: true },
};

test('should build groupby with series in form data', () => {
const queryContext = buildQuery(formData);
const [query] = queryContext.queries;
expect(query.columns).toEqual(['col1', 'col2', 'row1', 'row2']);
});

test('should work with old charts after GENERIC_CHART_AXES is enabled', () => {
Object.defineProperty(supersetCoreModule, 'hasGenericChartAxes', {
value: true,
});
const modifiedFormData = {
...formData,
time_grain_sqla: TimeGranularity.MONTH,
granularity_sqla: 'col1',
};
const queryContext = buildQuery(modifiedFormData);
const [query] = queryContext.queries;
expect(query.columns).toEqual([
{
timeGrain: 'P1M',
columnType: 'BASE_AXIS',
sqlExpression: 'col1',
label: 'col1',
expressionType: 'SQL',
},
'col2',
'row1',
'row2',
]);
});

it('should build groupby with series in form data', () => {
const queryContext = buildQuery(formData);
const [query] = queryContext.queries;
expect(query.columns).toEqual(['col1', 'col2', 'row1', 'row2']);
test('should prefer extra_form_data.time_grain_sqla over formData.time_grain_sqla', () => {
Object.defineProperty(supersetCoreModule, 'hasGenericChartAxes', {
value: true,
});
const modifiedFormData = {
...formData,
extra_form_data: { time_grain_sqla: TimeGranularity.QUARTER },
};
const queryContext = buildQuery(modifiedFormData);
const [query] = queryContext.queries;
expect(query.columns?.[0]).toEqual({
timeGrain: TimeGranularity.QUARTER,
columnType: 'BASE_AXIS',
sqlExpression: 'col1',
label: 'col1',
expressionType: 'SQL',
});
});

it('should work with old charts after GENERIC_CHART_AXES is enabled', () => {
Object.defineProperty(supersetCoreModule, 'hasGenericChartAxes', {
value: true,
});
const modifiedFormData = {
...formData,
time_grain_sqla: TimeGranularity.MONTH,
granularity_sqla: 'col1',
};
const queryContext = buildQuery(modifiedFormData);
const [query] = queryContext.queries;
expect(query.columns).toEqual([
{
timeGrain: 'P1M',
columnType: 'BASE_AXIS',
sqlExpression: 'col1',
label: 'col1',
expressionType: 'SQL',
},
'col2',
'row1',
'row2',
]);
test('should fallback to formData.time_grain_sqla if extra_form_data.time_grain_sqla is not set', () => {
Object.defineProperty(supersetCoreModule, 'hasGenericChartAxes', {
value: true,
});
const queryContext = buildQuery(formData);
const [query] = queryContext.queries;
expect(query.columns?.[0]).toEqual({
timeGrain: formData.time_grain_sqla,
columnType: 'BASE_AXIS',
sqlExpression: 'col1',
label: 'col1',
expressionType: 'SQL',
});
});