Skip to content

Commit

Permalink
chore: Remove legacy SIP-15 logic
Browse files Browse the repository at this point in the history
  • Loading branch information
John Bodley committed Feb 25, 2022
1 parent 420a63f commit e029d56
Show file tree
Hide file tree
Showing 111 changed files with 123 additions and 744 deletions.
2 changes: 2 additions & 0 deletions UPDATING.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ assists people when migrating to a new version.

## Next

- [18936](https://github.com/apache/superset/pull/18936): Removes legacy SIP-15 interm logic/flags—specifically the `SIP_15_ENABLED`, `SIP_15_GRACE_PERIOD_END`, `SIP_15_DEFAULT_TIME_RANGE_ENDPOINTS`, and `SIP_15_TOAST_MESSAGE` flags. Time range endpoints are no longer configurable and strictly adhere to the `[start, end)` paradigm, i.e., inclusive of the start and exclusive of the end.

### Breaking Changes

- [17881](https://github.com/apache/superset/pull/17881): Previously simple adhoc filter values on string columns were stripped of enclosing single and double quotes. To fully support literal quotes in filters, both single and double quotes will no longer be removed from filter values.
Expand Down
61 changes: 0 additions & 61 deletions docs/docs/installation/configuring-superset.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -242,64 +242,3 @@ FEATURE_FLAGS = {
```

A current list of feature flags can be found in [RESOURCES/FEATURE_FLAGS.md](https://github.com/apache/superset/blob/master/RESOURCES/FEATURE_FLAGS.md).

### SIP 15

[Superset Improvement Proposal 15](https://github.com/apache/superset/issues/6360) aims to
ensure that time intervals are handled in a consistent and transparent manner for both the Druid and
SQLAlchemy connectors.

Prior to SIP-15 SQLAlchemy used inclusive endpoints however these may behave like exclusive for
string columns (due to lexicographical ordering) if no formatting was defined and the column
formatting did not conform to an ISO 8601 date-time (refer to the SIP for details).

To remedy this rather than having to define the date/time format for every non-IS0 8601 date-time
column, once can define a default column mapping on a per database level via the `extra` parameter:

```
{
"python_date_format_by_column_name": {
"ds": "%Y-%m-%d"
}
}
```

**New Deployments**

All new deployments should enable SIP-15 by setting this value in `superset_config.py`:

```
SIP_15_ENABLED = True
```

**Existing Deployments**

Given that it is not apparent whether the chart creator was aware of the time range inconsistencies
(and adjusted the endpoints accordingly) changing the behavior of all charts is overly aggressive.
Instead SIP-15 proivides a soft transistion allowing producers (chart owners) to see the impact of
the proposed change and adjust their charts accordingly.

Prior to enabling SIP-15, existing deployments should communicate to their users the impact of the
change and define a grace period end date (exclusive of course) after which all charts will conform
to the [start, end) interval.

```python
from datetime import date

SIP_15_ENABLED = True
SIP_15_GRACE_PERIOD_END = date(<YYYY>, <MM>, <DD>)
```

To aid with transparency the current endpoint behavior is explicitly called out in the chart time
range (post SIP-15 this will be [start, end) for all connectors and databases). One can override the
defaults on a per database level via the `extra` parameter.

```python
{
"time_range_endpoints": ["inclusive", "inclusive"]
}
```

Note in a future release the interim SIP-15 logic will be removed (including the
`time_grain_endpoints` form-data field) via a code change and Alembic migration.
4 changes: 0 additions & 4 deletions docs/static/resources/openapi.json
Original file line number Diff line number Diff line change
Expand Up @@ -683,10 +683,6 @@
"nullable": true,
"type": "string"
},
"time_range_endpoints": {
"items": {},
"type": "array"
},
"where": {
"description": "WHERE clause to be added to queries using AND operator.",
"type": "string"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -419,7 +419,6 @@ describe('Nativefilters Sanity test', () => {
viz_type: 'echarts_timeseries',
datasource: '3__table',
granularity_sqla: 'purpose__last_set',
time_range_endpoints: ['inclusive', 'exclusive'],
time_grain_sqla: 'P1D',
time_range: 'No filter',
metrics: ['count'],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,15 +91,6 @@ export const datasourceAndVizType: ControlPanelSectionConfig = {
),
},
},
{
name: 'time_range_endpoints',
config: {
type: 'HiddenControl',
label: t('Time range endpoints'),
hidden: true,
description: t('Time range endpoints (SIP-15)'),
},
},
],
],
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,6 @@ const time_range: SharedControlConfig<'DateFilterControl'> = {
),
mapStateToProps: ({ datasource, form_data }) => ({
datasource,
endpoints: form_data?.time_range_endpoints || null,
}),
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,7 @@ import {
export const DTTM_ALIAS = '__timestamp';

export const EXTRA_FORM_DATA_OVERRIDE_EXTRA_KEYS: (keyof ExtraFormDataOverrideExtras)[] =
[
'druid_time_origin',
'relative_start',
'relative_end',
'time_grain_sqla',
'time_range_endpoints',
];
['druid_time_origin', 'relative_start', 'relative_end', 'time_grain_sqla'];

export const EXTRA_FORM_DATA_APPEND_KEYS: (keyof ExtraFormDataAppend)[] = [
'adhoc_filters',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,9 +89,5 @@ export default function extractExtras(formData: QueryFormData): ExtractedExtra {
delete extract.time_grain_sqla;
}

// map time range endpoints:
if (formData.time_range_endpoints)
extras.time_range_endpoints = formData.time_range_endpoints;

return extract;
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
*/
import { DatasourceType } from './Datasource';
import { BinaryOperator, SetOperator, UnaryOperator } from './Operator';
import { AppliedTimeExtras, TimeRange, TimeRangeEndpoints } from './Time';
import { AppliedTimeExtras, TimeRange } from './Time';
import { AnnotationLayer } from './AnnotationLayer';
import {
QueryFields,
Expand Down Expand Up @@ -59,7 +59,6 @@ export type QueryObjectExtras = Partial<{
relative_start?: string;
relative_end?: string;
time_grain_sqla?: TimeGranularity;
time_range_endpoints?: TimeRangeEndpoints;
/** WHERE condition */
where?: string;
}>;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import {
QueryObjectExtras,
QueryObjectFilterClause,
} from './Query';
import { TimeRange, TimeRangeEndpoints } from './Time';
import { TimeRange } from './Time';
import { TimeGranularity } from '../../time-format';
import { JsonObject } from '../../connection';
import { AdhocColumn, PhysicalColumn } from './Column';
Expand Down Expand Up @@ -120,11 +120,7 @@ export type ExtraFormDataAppend = {
* filter clauses can't be overridden */
export type ExtraFormDataOverrideExtras = Pick<
QueryObjectExtras,
| 'druid_time_origin'
| 'relative_start'
| 'relative_end'
| 'time_grain_sqla'
| 'time_range_endpoints'
'druid_time_origin' | 'relative_start' | 'relative_end' | 'time_grain_sqla'
>;

/** These parameters override those already present in the form data/query object */
Expand Down Expand Up @@ -180,7 +176,6 @@ export interface BaseFormData extends TimeRange, FormDataResidual {
force?: boolean;
result_format?: string;
result_type?: string;
time_range_endpoints?: TimeRangeEndpoints;
annotation_layers?: AnnotationLayer[];
url_params?: Record<string, string>;
custom_params?: Record<string, string>;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,4 @@ export type AppliedTimeExtras = Partial<
Record<TimeColumnConfigKey, keyof QueryObject>
>;

export type TimeRangeEndpoint = 'unknown' | 'inclusive' | 'exclusive';
export type TimeRangeEndpoints = [TimeRangeEndpoint, TimeRangeEndpoint];

export default {};
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ describe('extractExtras', () => {
expect(
extractExtras({
...baseQueryFormData,
time_range_endpoints: ['inclusive', 'exclusive'],
extra_filters: [
{
col: '__time_col',
Expand All @@ -57,7 +56,6 @@ describe('extractExtras', () => {
},
extras: {
time_grain_sqla: 'PT5M',
time_range_endpoints: ['inclusive', 'exclusive'],
},
filters: [],
granularity: 'ds2',
Expand Down Expand Up @@ -107,7 +105,6 @@ describe('extractExtras', () => {
expect(
extractExtras({
...baseQueryFormData,
time_range_endpoints: ['inclusive', 'exclusive'],
extra_filters: [
{
col: 'gender',
Expand Down Expand Up @@ -139,7 +136,6 @@ describe('extractExtras', () => {
},
extras: {
time_grain_sqla: 'PT5M',
time_range_endpoints: ['inclusive', 'exclusive'],
},
filters: [
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ export default {
datasource: '93829__table',
viz_type: 'deck_polygon',
url_params: {},
time_range_endpoints: ['inclusive', 'exclusive'],
granularity_sqla: null,
time_range: '100 years ago : ',
line_column: 'geometry',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,6 @@ const createProps = () => ({
row_limit: 10000,
show_legend: false,
time_range: 'No filter',
time_range_endpoints: ['inclusive', 'exclusive'],
url_params: {},
viz_type: 'dist_bar',
x_ticks_layout: 'auto',
y_axis_format: 'SMART_NUMBER',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@ const createProps = (viz_type = 'sunburst') => ({
row_limit: 10000,
slice_id: 371,
time_range: 'No filter',
time_range_endpoints: ['inclusive', 'exclusive'],
url_params: {},
viz_type,
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -967,7 +967,6 @@ const FiltersConfigForm = (
>
<DateFilterControl
name="time_range"
endpoints={['inclusive', 'exclusive']}
onChange={timeRange => {
setNativeFilterFieldValues(form, filterId, {
time_range: timeRange,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@ export const getFormData = ({
showSearch: true,
defaultValue: defaultDataMask?.filterState?.value,
time_range,
time_range_endpoints: ['inclusive', 'exclusive'],
url_params: extractUrlParams('regular'),
inView: true,
viz_type: filterType,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ const regionFilter = {
show_bubbles: true,
slice_id: 32,
time_range: '2014-01-01 : 2014-01-02',
time_range_endpoints: ['inclusive', 'exclusive'],
viz_type: 'filter_box',
},
modified: '<bound method AuditMixinNullable.modified of Region Filter>',
Expand Down Expand Up @@ -85,7 +84,6 @@ const chart1 = {
show_bubbles: true,
slice_id: 33,
time_range: '2000 : 2014-01-02',
time_range_endpoints: ['inclusive', 'exclusive'],
viz_type: 'big_number',
},
modified: "<bound method AuditMixinNullable.modified of World's Population>",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ const createProps = () => ({
datasource: '34__table',
slice_id: 456,
url_params: {},
time_range_endpoints: ['unknown', 'inclusive'],
time_range: 'Last week',
all_columns_x: 'source',
all_columns_y: 'target',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ const createProps = () => ({
datasource: '49__table',
slice_id: 318,
url_params: {},
time_range_endpoints: ['inclusive', 'exclusive'],
granularity_sqla: 'time_start',
time_range: 'No filter',
all_columns_x: ['age'],
Expand Down Expand Up @@ -65,7 +64,6 @@ const createProps = () => ({
row_limit: 10000,
slice_id: 318,
time_range: 'No filter',
time_range_endpoints: ['inclusive', 'exclusive'],
url_params: {},
viz_type: 'histogram',
x_axis_label: 'age',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ const createProps = () => ({
datasource: '49__table',
slice_id: 318,
url_params: {},
time_range_endpoints: ['inclusive', 'exclusive'],
granularity_sqla: 'time_start',
time_range: 'No filter',
all_columns_x: ['age'],
Expand Down Expand Up @@ -66,7 +65,6 @@ const createProps = () => ({
row_limit: 10000,
slice_id: 318,
time_range: 'No filter',
time_range_endpoints: ['inclusive', 'exclusive'],
url_params: {},
viz_type: 'histogram',
x_axis_label: 'age',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ const createProps = () => ({
row_limit: 10000,
slice_id: 318,
time_range: 'No filter',
time_range_endpoints: ['inclusive', 'exclusive'],
url_params: {},
viz_type: 'histogram',
x_axis_label: 'age',
Expand Down Expand Up @@ -108,7 +107,7 @@ fetchMock.get('glob:*/api/v1/chart/318', {
},
],
params:
'{"adhoc_filters": [], "all_columns_x": ["age"], "color_scheme": "supersetColors", "datasource": "42__table", "granularity_sqla": "time_start", "groupby": null, "label_colors": {}, "link_length": "25", "queryFields": {"groupby": "groupby"}, "row_limit": 10000, "slice_id": 1380, "time_range": "No filter", "time_range_endpoints": ["inclusive", "exclusive"], "url_params": {}, "viz_type": "histogram", "x_axis_label": "age", "y_axis_label": "count"}',
'{"adhoc_filters": [], "all_columns_x": ["age"], "color_scheme": "supersetColors", "datasource": "42__table", "granularity_sqla": "time_start", "groupby": null, "label_colors": {}, "link_length": "25", "queryFields": {"groupby": "groupby"}, "row_limit": 10000, "slice_id": 1380, "time_range": "No filter", "url_params": {}, "viz_type": "histogram", "x_axis_label": "age", "y_axis_label": "count"}',
slice_name: 'Age distribution of respondents',
viz_type: 'histogram',
},
Expand Down
Loading

0 comments on commit e029d56

Please sign in to comment.