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: Default temporal column in Datasource #21857

Merged
merged 4 commits into from
Oct 25, 2022
+142 −11
Merged

fix: Default temporal column in Datasource #21857

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5bf6059
WIP
geido Oct 18, 2022
0dcac0a
Fix and add tests
geido Oct 18, 2022
3fdca22
Use util
geido Oct 19, 2022
18dda9e
Fix default temporal
geido Oct 20, 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
124 changes: 122 additions & 2 deletions ...set-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,10 @@
*/

import React from 'react';
import { render, screen, act } from 'spec/helpers/testing-library';
import { render, screen, act, waitFor } from 'spec/helpers/testing-library';
import userEvent from '@testing-library/user-event';
import { SupersetClient, DatasourceType } from '@superset-ui/core';
import fetchMock from 'fetch-mock';
import DatasourceControl from '.';

const SupersetClientGet = jest.spyOn(SupersetClient, 'get');
Expand All @@ -45,7 +46,10 @@ const createProps = () => ({
},
validationErrors: [],
name: 'datasource',
actions: {},
actions: {
changeDatasource: jest.fn(),
setControlValue: jest.fn(),
},
isEditable: true,
user: {
createdOn: '2021-04-27T18:12:38.952304',
Expand All @@ -62,6 +66,17 @@ const createProps = () => ({
onDatasourceSave: jest.fn(),
});

fetchMock.post('glob:*/datasource/save/', {
...createProps().datasource,
});

async function openAndSaveChanges() {
userEvent.click(screen.getByTestId('datasource-menu-trigger'));
userEvent.click(await screen.findByTestId('edit-dataset'));
userEvent.click(await screen.findByTestId('datasource-modal-save'));
userEvent.click(await screen.findByText('OK'));
}

test('Should render', async () => {
const props = createProps();
render(<DatasourceControl {...props} />);
Expand Down Expand Up @@ -229,3 +244,108 @@ test('Click on Save as dataset', async () => {
expect(screen.getByRole('button', { name: /close/i })).toBeVisible();
expect(dropdownField).toBeVisible();
});

test('should set the default temporal column', async () => {
const props = createProps();
const overrideProps = {
...props,
form_data: {
granularity_sqla: 'test-col',
},
datasource: {
...props.datasource,
main_dttm_col: 'test-default',
columns: [
{
column_name: 'test-col',
is_dttm: false,
},
{
column_name: 'test-default',
is_dttm: true,
},
],
},
};
render(<DatasourceControl {...props} {...overrideProps} />, {
useRedux: true,
});

await openAndSaveChanges();
await waitFor(() => {
expect(props.actions.setControlValue).toHaveBeenCalledWith(
'granularity_sqla',
'test-default',
);
});
});

test('should set the first available temporal column', async () => {
const props = createProps();
const overrideProps = {
...props,
form_data: {
granularity_sqla: 'test-col',
},
datasource: {
...props.datasource,
main_dttm_col: null,
columns: [
{
column_name: 'test-col',
is_dttm: false,
},
{
column_name: 'test-first',
is_dttm: true,
},
],
},
};
render(<DatasourceControl {...props} {...overrideProps} />, {
useRedux: true,
});

await openAndSaveChanges();
await waitFor(() => {
expect(props.actions.setControlValue).toHaveBeenCalledWith(
'granularity_sqla',
'test-first',
);
});
});

test('should not set the temporal column', async () => {
const props = createProps();
const overrideProps = {
...props,
form_data: {
granularity_sqla: null,
},
datasource: {
...props.datasource,
main_dttm_col: null,
columns: [
{
column_name: 'test-col',
is_dttm: false,
},
{
column_name: 'test-col-2',
is_dttm: false,
},
],
},
};
render(<DatasourceControl {...props} {...overrideProps} />, {
useRedux: true,
});

await openAndSaveChanges();
await waitFor(() => {
expect(props.actions.setControlValue).toHaveBeenCalledWith(
'granularity_sqla',
undefined,
);
});
});
27 changes: 15 additions & 12 deletions superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import {
t,
withTheme,
} from '@superset-ui/core';

import { getTemporalColumns } from '@superset-ui/chart-controls';
import { getUrlParam } from 'src/utils/urlUtils';
import { AntdDropdown } from 'src/components';
import { Menu } from 'src/components/Menu';
Expand Down Expand Up @@ -171,19 +171,22 @@ class DatasourceControl extends React.PureComponent {

onDatasourceSave = datasource => {
this.props.actions.changeDatasource(datasource);
const timeCol = this.props.form_data?.granularity_sqla;
geido marked this conversation as resolved.
Show resolved Hide resolved
const { columns } = this.props.datasource;
const firstDttmCol = columns.find(column => column.is_dttm);
if (
datasource.type === 'table' &&
!columns.find(({ column_name }) => column_name === timeCol)?.is_dttm
) {
// set `granularity_sqla` to first datatime column name or null
this.props.actions.setControlValue(
'granularity_sqla',
firstDttmCol ? firstDttmCol.column_name : null,
);
const timeCol = this.props.form_data?.granularity_sqla;
const timeColConf = columns.find(
({ column_name }) => column_name === timeCol,
);
if (datasource.type === 'table') {
// resets new default time col or picks the first available one
if (!timeColConf?.is_dttm) {
const setDttmCol = getTemporalColumns(this.props.datasource);
this.props.actions.setControlValue(
'granularity_sqla',
setDttmCol.defaultTemporalColumn,
);
}
}

Copy link
Member

@zhaoyongjie zhaoyongjie Oct 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The getTemporalColumns will return a well-designed data structure so the below snippet will handle it. this logic is same as dndGranularitySqlaControl. notice that the granularity_sqla default value is null.

const {temporalColumns, defaultTemporalColumn} = getTemporalColumns(this.props.datasource)
this.props.actions.setControlValue(
  'granularity_sqla',
  defaultTemporalColumn ?? temporalColumns?.[0] ?? null,
);

Copy link
Member Author

Choose a reason for hiding this comment

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

@zhaoyongjie I still need to check if the current time col is null, because I only want to replace the existing value if that is null, not otherwise

Copy link
Member

Choose a reason for hiding this comment

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

@geido timeColConf must be a time column, otherwise it wouldn't be set to granularity_sqla.

Copy link
Member

Choose a reason for hiding this comment

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

@geido i jsut reread this codes. the datasource is new datasource setting, so we should use the new datasouce instead of this.props.datasouce.

const {temporalColumns, defaultTemporalColumn} = getTemporalColumns(datasource)
this.props.actions.setControlValue(
  'granularity_sqla',
  defaultTemporalColumn ?? temporalColumns?.[0] ?? null,
);

Copy link
Member Author

Choose a reason for hiding this comment

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

@zhaoyongjie granularity_sqla can indeed return also a column that is not a time column anymore after the user has unselected the "Is temporal" from the dataset. To try, just select a default time column for the time column control, then edit the dataset and keep the column as default but remove the "Is temporal" flag, you will see that granularity_sqla will still return that value as if it was temporal as indeed that's the last value known when saving the dataset.

That's useful because we want to change the control value only when it's empty. This is a product requirement that I confirmed with Kasia as well.

I also found out that the getTemporalColumns util isn't working properly. It's setting as default temporal column also columns that are not temporal anymore. In fact, the datasource.main_dttm_col can hold a column that was kept as default but removed from "Is temporal". I think this requires a fix in the util itself, but for now I am just making an additional check in my code to account for this issue as I don't want to potentially cause troubles to other parts of the application that are using that util.

I made some more improvements to the code but I believe this should be ready to go. I will spin up a testenv if you want to manual test it.

The product requirements for manual testing are the following:

  • If the control is empty, change it with the new default or the first time column found if no default is there
  • If the control is not empty, changing the default will only affect the dataset but not the control which will keep the existing value
  • If the column that is currently used in the control is removed from the dataset as time column and if there is no other time column available, the control should be emptied

Thanks

Copy link
Member

@zhaoyongjie zhaoyongjie Oct 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@geido Thank you for the explanation. I understand what you are saying. This has been a long-time-coming issue in the Superset.
The main_dttm_col is saved in the tables table on the Superset metadata. According to Superset's original design(for Druid native query), each dataset must have a main_dttm_col, it is granularity_sqla in the frontend, AKA time column.

image

Every column in the datasource is saved in the table_columns table, it has a column is_dttm for is_temporal usages.
image

the time column query order is

  1. main_dttm_col.
  2. Arbitrary column is in table_columns and is_dttm is true.

The issue should be that even if we unselected the default column's is_dttm attribute, it is still saved in the main_dttm_col column. We should fix this logic --- remove a non-temporal recorded from main_dttm_col, and not just ignore main_dttm_col on the frontend, which would cause inconsistencies between the front and backend.

Copy link
Member Author

Choose a reason for hiding this comment

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

@zhaoyongjie I understand that but the fix would be out of scope right now. I need to get the fix for the control out first and I can open a separate issue to investigate the core problem that you are describing. Thanks

if (this.props.onDatasourceSave) {
this.props.onDatasourceSave(datasource);
}
Expand Down