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

fix: Time Column on Generic X-axis #23021

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,11 @@
import buildQueryObject from './buildQueryObject';
import DatasourceKey from './DatasourceKey';
import { QueryFieldAliases, QueryFormData } from './types/QueryFormData';
import {
BinaryQueryObjectFilterClause,
QueryContext,
QueryObject,
} from './types/Query';
import { QueryContext, QueryObject } from './types/Query';
import { SetDataMaskHook } from '../chart';
import { JsonObject } from '../connection';
import { normalizeTimeColumn } from './normalizeTimeColumn';
import { hasGenericChartAxes, isXAxisSet } from './getXAxis';
import { ensureIsArray } from '../utils';
import { isXAxisSet } from './getXAxis';

const WRAP_IN_ARRAY = (baseQueryObject: QueryObject) => [baseQueryObject];

Expand Down Expand Up @@ -60,14 +55,6 @@ export default function buildQueryContext(
// eslint-disable-next-line no-param-reassign
query.post_processing = query.post_processing.filter(Boolean);
}
if (hasGenericChartAxes && query.time_range) {
// eslint-disable-next-line no-param-reassign
query.filters = ensureIsArray(query.filters).map(flt =>
flt?.op === 'TEMPORAL_RANGE'
? ({ ...flt, val: query.time_range } as BinaryQueryObjectFilterClause)
: flt,
);
}
});
if (isXAxisSet(formData)) {
queries = queries.map(query => normalizeTimeColumn(formData, query));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -164,41 +164,4 @@ describe('buildQueryContext', () => {
expect(spyNormalizeTimeColumn).not.toBeCalled();
spyNormalizeTimeColumn.mockRestore();
});
it('should orverride time filter if GENERIC_CHART_AXES is enabled', () => {
Object.defineProperty(getXAxisModule, 'hasGenericChartAxes', {
value: true,
});

const queryContext = buildQueryContext(
{
datasource: '5__table',
viz_type: 'table',
},
() => [
{
filters: [
{
col: 'col1',
op: 'TEMPORAL_RANGE',
val: '2001 : 2002',
},
{
col: 'col2',
op: 'IN',
val: ['a', 'b'],
},
],
time_range: '1990 : 1991',
},
],
);
expect(queryContext.queries[0].filters).toEqual([
{ col: 'col1', op: 'TEMPORAL_RANGE', val: '1990 : 1991' },
{
col: 'col2',
op: 'IN',
val: ['a', 'b'],
},
]);
});
});
3 changes: 2 additions & 1 deletion superset-frontend/src/explore/components/SaveModal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,8 @@ class SaveModal extends React.Component<SaveModalProps, SaveModalState> {
);
}

const { url_params, ...formData } = this.props.form_data || {};
const formData = this.props.form_data || {};
delete formData.url_params;

let dashboard: DashboardGetResponse | null = null;
if (this.state.newDashboardName || this.state.saveToDashboardId) {
Expand Down
96 changes: 93 additions & 3 deletions superset/common/query_context_factory.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
from superset.charts.dao import ChartDAO
from superset.common.chart_data import ChartDataResultFormat, ChartDataResultType
from superset.common.query_context import QueryContext
from superset.common.query_object import QueryObject
from superset.common.query_object_factory import QueryObjectFactory
from superset.datasource.dao import DatasourceDAO
from superset.models.slice import Slice
Expand Down Expand Up @@ -65,8 +66,12 @@ def create(
result_type = result_type or ChartDataResultType.FULL
result_format = result_format or ChartDataResultFormat.JSON
queries_ = [
self._query_object_factory.create(
result_type, datasource=datasource, **query_obj
self._process_query_object(
datasource_model_instance,
form_data,
self._query_object_factory.create(
result_type, datasource=datasource, **query_obj
),
)
for query_obj in queries
]
Expand All @@ -90,7 +95,6 @@ def create(

# pylint: disable=no-self-use
def _convert_to_model(self, datasource: DatasourceDict) -> BaseDatasource:

return DatasourceDAO.get_datasource(
session=db.session,
datasource_type=DatasourceType(datasource["type"]),
Expand All @@ -99,3 +103,89 @@ def _convert_to_model(self, datasource: DatasourceDict) -> BaseDatasource:

def _get_slice(self, slice_id: Any) -> Optional[Slice]:
return ChartDAO.find_by_id(slice_id)

def _process_query_object(
self,
datasource: BaseDatasource,
form_data: Optional[Dict[str, Any]],
query_object: QueryObject,
) -> QueryObject:
self._apply_granularity(query_object, form_data, datasource)
self._apply_filters(query_object)
return query_object

def _apply_granularity(
self,
query_object: QueryObject,
form_data: Optional[Dict[str, Any]],
datasource: BaseDatasource,
) -> None:
temporal_columns = {
column.column_name
for column in datasource.columns
if (column["is_dttm"] if isinstance(column, dict) else column.is_dttm)
}
granularity = query_object.granularity
x_axis = form_data and form_data.get("x_axis")

if granularity:
filter_to_remove = None
if x_axis and x_axis in temporal_columns:
Copy link
Contributor

Choose a reason for hiding this comment

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

hey @michael-s-molina we recently found a regression where the x_axis could be a dict while temporal_columns is a set of string and it throws.

Steps to reproduce:

1. Create a Bar Chart using the Vehicle Sales dataset.
2. Use Custom SQL for the X-Axis field and type in order_date.
3. Use SUM(price_each) for the Metrics field.
4. Use the SQL Lab to duplicate the Vehicle Sales dataset.
5. Perform a dataset swap to the duplicated Vehicle Sales dataset.
6. Update and Save the chart.

I am trying to use x_axis["sqlExpression"] to get the proper str to compare down the road and it doesn't seem to have the same effect.

Would love your thoughts here thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @zephyring. Can you open a draft PR with your changes (x_axis["sqlExpression"])? Can you also add a comment to your PR explaining what do you mean by

it doesn't seem to have the same effect

Copy link
Member

Choose a reason for hiding this comment

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

@michael-s-molina @zephyring It appears the issue here happens due to using an adhoc column as an x-axis. Previously I've also noticed that adhoc x-axes may not always work as expected, and this logic probably needs to be refined somewhat. Maybe we should have a Zoom session to work on a proper fix for this?

Copy link

Choose a reason for hiding this comment

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

Is there an issue or PR linked to this?

filter_to_remove = x_axis
x_axis_column = next(
(
column
for column in query_object.columns
if column == x_axis
or (
isinstance(column, dict)
and column["sqlExpression"] == x_axis
)
),
None,
)
# Replaces x-axis column values with granularity
if x_axis_column:
if isinstance(x_axis_column, dict):
x_axis_column["sqlExpression"] = granularity
x_axis_column["label"] = granularity
else:
query_object.columns = [
granularity if column == x_axis_column else column
for column in query_object.columns
]
for post_processing in query_object.post_processing:
if post_processing.get("operation") == "pivot":
post_processing["options"]["index"] = [granularity]

# If no temporal x-axis, then get the default temporal filter
if not filter_to_remove:
temporal_filters = [
filter["col"]
for filter in query_object.filter
if filter["op"] == "TEMPORAL_RANGE"
]
if len(temporal_filters) > 0:
# Use granularity if it's already in the filters
if granularity in temporal_filters:
filter_to_remove = granularity
else:
# Use the first temporal filter
filter_to_remove = temporal_filters[0]

# Removes the temporal filter which may be an x-axis or
# another temporal filter. A new filter based on the value of
# the granularity will be added later in the code.
# In practice, this is replacing the previous default temporal filter.
if filter_to_remove:
query_object.filter = [
filter
for filter in query_object.filter
if filter["col"] != filter_to_remove
]

def _apply_filters(self, query_object: QueryObject) -> None:
if query_object.time_range:
for filter_object in query_object.filter:
if filter_object["op"] == "TEMPORAL_RANGE":
filter_object["val"] = query_object.time_range