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

[SIP-15] Adding initial framework #8398

Merged
merged 3 commits into from
Oct 28, 2019
Merged
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
15 changes: 14 additions & 1 deletion docs/installation.rst
Original file line number Diff line number Diff line change
@@ -1242,7 +1242,7 @@ Then we can add this two lines to ``superset_config.py``:
CUSTOM_SECURITY_MANAGER = CustomSsoSecurityManager

Feature Flags
---------------------------
-------------

Because of a wide variety of users, Superset has some features that are not enabled by default. For example, some users have stronger security restrictions, while some others may not. So Superset allow users to enable or disable some features by config. For feature owners, you can add optional functionalities in Superset, but will be only affected by a subset of users.

@@ -1267,3 +1267,16 @@ Here is a list of flags and descriptions:
* PRESTO_EXPAND_DATA

* When this feature is enabled, nested types in Presto will be expanded into extra columns and/or arrays. This is experimental, and doesn't work with all nested types.


SIP-15
------

`SIP-15 <https://github.com/apache/incubator-superset/issues/6360>`_ aims to ensure that time intervals are handled in a consistent and transparent manner for both the Druid and SQLAlchemy connectors.
Copy link
Member

Choose a reason for hiding this comment

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

nit: remove extra underscore here

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

ah i see.


Prior to SIP-15 SQLAlchemy used inclusive endpoints however these may behave like exclusive depending on the time column (refer to the SIP for details) and thus the endpoint behavior could be unknown. 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 ::

{
"time_range_endpoints": ["inclusive", "inclusive"]
}
13 changes: 6 additions & 7 deletions superset/assets/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions superset/assets/package.json
Original file line number Diff line number Diff line change
@@ -101,6 +101,7 @@
"dompurify": "^1.0.3",
"geolib": "^2.0.24",
"immutable": "^3.8.2",
"interweave": "^11.2.0",
"jquery": "^3.4.1",
"json-bigint": "^0.3.0",
"lodash": "^4.17.15",
Original file line number Diff line number Diff line change
@@ -18,7 +18,7 @@
*/
import { Alert } from 'react-bootstrap';
import React from 'react';
import { shallow } from 'enzyme';
import { mount } from 'enzyme';

import mockMessageToasts from '../mockMessageToasts';
import Toast from '../../../../src/messageToasts/components/Toast';
@@ -30,7 +30,7 @@ describe('Toast', () => {
};

function setup(overrideProps) {
const wrapper = shallow(<Toast {...props} {...overrideProps} />);
const wrapper = mount(<Toast {...props} {...overrideProps} />);
return wrapper;
}

@@ -41,9 +41,14 @@ describe('Toast', () => {

it('should render toastText within the alert', () => {
const wrapper = setup();
const alert = wrapper.find(Alert).dive();
const alert = wrapper.find(Alert);

expect(alert.childAt(1).text()).toBe(props.toast.text);
expect(
alert
.childAt(0)
.childAt(1)
.text(),
).toBe(props.toast.text);
});

it('should call onCloseToast upon alert dismissal', done => {
Original file line number Diff line number Diff line change
@@ -83,6 +83,7 @@ const FREEFORM_TOOLTIP = t(
);

const DATE_FILTER_POPOVER_STYLE = { width: '250px' };
const ISO_8601_REGEX_MATCH = /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}$/;

const propTypes = {
animation: PropTypes.bool,
@@ -94,6 +95,7 @@ const propTypes = {
height: PropTypes.number,
onOpenDateFilterControl: PropTypes.func,
onCloseDateFilterControl: PropTypes.func,
endpoints: PropTypes.arrayOf(PropTypes.string),
};

const defaultProps = {
@@ -359,13 +361,14 @@ export default class DateFilterControl extends React.Component {
));
const timeFrames = COMMON_TIME_FRAMES.map((timeFrame) => {
const nextState = getStateFromCommonTimeFrame(timeFrame);
const endpoints = this.props.endpoints;
return (
<OverlayTrigger
key={timeFrame}
placement="left"
overlay={
<Tooltip id={`tooltip-${timeFrame}`}>
{nextState.since}<br />{nextState.until}
{nextState.since} {endpoints && `(${endpoints[0]})`}<br />{nextState.until} {endpoints && `(${endpoints[1]})`}
</Tooltip>
}
>
@@ -507,7 +510,15 @@ export default class DateFilterControl extends React.Component {
}
render() {
let value = this.props.value || defaultProps.value;
value = value.split(SEPARATOR).map((v, idx) => v.replace('T00:00:00', '') || (idx === 0 ? '-∞' : '∞')).join(SEPARATOR);
const endpoints = this.props.endpoints;
value = value
.split(SEPARATOR)
.map((v, idx) =>
ISO_8601_REGEX_MATCH.test(v)
? v.replace('T00:00:00', '') + (endpoints ? ` (${endpoints[idx]})` : '')
: v || (idx === 0 ? '-∞' : '∞'),
)
.join(SEPARATOR);
return (
<div>
<ControlHeader {...this.props} />
2 changes: 1 addition & 1 deletion superset/assets/src/explore/controlPanels/sections.jsx
Original file line number Diff line number Diff line change
@@ -35,7 +35,7 @@ export const datasourceAndVizType = {
controlSetRows: [
['datasource'],
['viz_type'],
['slice_id', 'cache_timeout', 'url_params'],
['slice_id', 'cache_timeout', 'url_params', 'time_range_endpoints'],
],
};

10 changes: 10 additions & 0 deletions superset/assets/src/explore/controls.jsx
Original file line number Diff line number Diff line change
@@ -959,6 +959,9 @@ export const controls = {
'using the engine\'s local timezone. Note one can explicitly set the timezone ' +
'per the ISO 8601 format if specifying either the start and/or end time.',
),
mapStateToProps: state => ({
endpoints: state.form_data ? state.form_data.time_range_endpoints : null,
}),
},

max_bubble_size: {
@@ -2034,6 +2037,13 @@ export const controls = {
description: t('Extra parameters for use in jinja templated queries'),
},

time_range_endpoints: {
type: 'HiddenControl',
label: t('Time range endpoints'),
hidden: true,
description: t('Time range endpoints (SIP-15)'),
Copy link
Member

Choose a reason for hiding this comment

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

Should we elaborate more about SIP-15 here? Provide a link or something?

Copy link
Member Author

Choose a reason for hiding this comment

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

@etr2460 this control is always hidden and only exists to ensure that associated form-data is preserved.

},

order_by_entity: {
type: 'CheckboxControl',
label: t('Order by entity id'),
3 changes: 2 additions & 1 deletion superset/assets/src/messageToasts/components/Toast.jsx
Original file line number Diff line number Diff line change
@@ -18,6 +18,7 @@
*/
import { Alert } from 'react-bootstrap';
import cx from 'classnames';
import Interweave from 'interweave';
import PropTypes from 'prop-types';
import React from 'react';

@@ -96,7 +97,7 @@ class Toast extends React.Component {
toastType === DANGER_TOAST && 'toast--danger',
)}
>
{text}
<Interweave content={text} />
</Alert>
);
}
2 changes: 1 addition & 1 deletion superset/assets/src/messageToasts/stylesheets/toast.less
Original file line number Diff line number Diff line change
@@ -23,7 +23,7 @@
bottom: 16px;
left: 50%;
transform: translate(-50%, 0);
width: 500px;
width: 600px;
z-index: 3000; // top of the world
}

5 changes: 5 additions & 0 deletions superset/config.py
Original file line number Diff line number Diff line change
@@ -679,6 +679,11 @@ class CeleryConfig(object):
# SQLALCHEMY_DATABASE_URI by default if set to `None`
SQLALCHEMY_EXAMPLES_URI = None

# Note currently SIP-15 feature is WIP and should not be enabled.
SIP_15_ENABLED = False
SIP_15_DEFAULT_TIME_RANGE_ENDPOINTS = ["unknown", "inclusive"]
SIP_15_TOAST_MESSAGE = 'Action Required: Preview then save your chart using the new time range endpoints <a href="{url}" class="alert-link">here</a>.'

if CONFIG_PATH_ENV_VAR in os.environ:
# Explicitly import config module that is not necessarily in pythonpath; useful
# for case where app is being executed via pex.
32 changes: 25 additions & 7 deletions superset/connectors/sqla/models.py
Original file line number Diff line number Diff line change
@@ -19,7 +19,7 @@
import re
from collections import OrderedDict
from datetime import datetime
from typing import Any, Dict, List, NamedTuple, Optional, Union
from typing import Any, Dict, List, NamedTuple, Optional, Tuple, Union

import pandas as pd
import sqlalchemy as sa
@@ -159,14 +159,25 @@ def datasource(self) -> RelationshipProperty:
return self.table

def get_time_filter(
self, start_dttm: DateTime, end_dttm: DateTime
self,
start_dttm: DateTime,
end_dttm: DateTime,
time_range_endpoints: Optional[
Tuple[utils.TimeRangeEndpoint, utils.TimeRangeEndpoint]
],
) -> ColumnElement:
col = self.get_sqla_col(label="__time")
l = []
if start_dttm:
l.append(col >= text(self.dttm_sql_literal(start_dttm)))
if end_dttm:
l.append(col <= text(self.dttm_sql_literal(end_dttm)))
if (
time_range_endpoints
and time_range_endpoints[1] == utils.TimeRangeEndpoint.EXCLUSIVE
):
l.append(col < text(self.dttm_sql_literal(end_dttm)))
else:
l.append(col <= text(self.dttm_sql_literal(end_dttm)))
return and_(*l)

def get_timestamp_expression(
@@ -705,6 +716,7 @@ def get_sqla_query( # sqla
)
metrics_exprs = []

time_range_endpoints = extras.get("time_range_endpoints")
groupby_exprs_with_timestamp = OrderedDict(groupby_exprs_sans_timestamp.items())
if granularity:
dttm_col = cols[granularity]
@@ -716,16 +728,20 @@ def get_sqla_query( # sqla
select_exprs += [timestamp]
groupby_exprs_with_timestamp[timestamp.name] = timestamp

# Use main dttm column to support index with secondary dttm columns
# Use main dttm column to support index with secondary dttm columns.
if (
db_engine_spec.time_secondary_columns
and self.main_dttm_col in self.dttm_cols
and self.main_dttm_col != dttm_col.column_name
):
time_filters.append(
cols[self.main_dttm_col].get_time_filter(from_dttm, to_dttm)
cols[self.main_dttm_col].get_time_filter(
from_dttm, to_dttm, time_range_endpoints
)
)
time_filters.append(dttm_col.get_time_filter(from_dttm, to_dttm))
time_filters.append(
dttm_col.get_time_filter(from_dttm, to_dttm, time_range_endpoints)
)

select_exprs += metrics_exprs

@@ -831,7 +847,9 @@ def get_sqla_query( # sqla
inner_select_exprs += [inner_main_metric_expr]
subq = select(inner_select_exprs).select_from(tbl)
inner_time_filter = dttm_col.get_time_filter(
inner_from_dttm or from_dttm, inner_to_dttm or to_dttm
inner_from_dttm or from_dttm,
inner_to_dttm or to_dttm,
time_range_endpoints,
)
subq = subq.where(and_(*(where_clause_and + [inner_time_filter])))
subq = subq.group_by(*inner_groupby_exprs)
17 changes: 17 additions & 0 deletions superset/utils/core.py
Original file line number Diff line number Diff line change
@@ -33,6 +33,7 @@
from email.mime.multipart import MIMEMultipart
from email.mime.text import MIMEText
from email.utils import formatdate
from enum import Enum
from time import struct_time
from typing import Iterator, List, NamedTuple, Optional, Tuple, Union
from urllib.parse import unquote_plus
@@ -1244,3 +1245,19 @@ def split(
elif not quotes:
quotes = True
yield s[i:]


class TimeRangeEndpoint(str, Enum):
"""
The time range endpoint types which represent inclusive, exclusive, or unknown.

Unknown represents endpoints which are ill-defined as though the interval may be
[start, end] the filter may behave like (start, end] due to mixed data types and
lexicographical ordering.

:see: https://github.com/apache/incubator-superset/issues/6360
"""

EXCLUSIVE = "exclusive"
INCLUSIVE = "inclusive"
UNKNOWN = "unknown"
35 changes: 33 additions & 2 deletions superset/views/core.py
Original file line number Diff line number Diff line change
@@ -48,6 +48,7 @@
from sqlalchemy.exc import SQLAlchemyError
from sqlalchemy.orm.session import Session
from werkzeug.routing import BaseConverter
from werkzeug.urls import Href

import superset.models.core as models
from superset import (
@@ -1071,7 +1072,6 @@ def explore_json(self, datasource_type=None, datasource_id=None):
results = request.args.get("results") == "true"
samples = request.args.get("samples") == "true"
force = request.args.get("force") == "true"

form_data = get_form_data()[0]

try:
@@ -1143,8 +1143,39 @@ def explorev2(self, datasource_type, datasource_id):
def explore(self, datasource_type=None, datasource_id=None):
user_id = g.user.get_id() if g.user else None
form_data, slc = get_form_data(use_slice_data=True)
error_redirect = "/chart/list/"

# Flash the SIP-15 message if the slice is owned by the current user and has not
# been updated, i.e., is not using the [start, end) interval.
if (
config["SIP_15_ENABLED"]
and slc
and g.user in slc.owners
and (
not form_data.get("time_range_endpoints")
or form_data["time_range_endpoints"]
!= (
utils.TimeRangeEndpoint.INCLUSIVE,
utils.TimeRangeEndpoint.EXCLUSIVE,
)
)
):
url = Href("/superset/explore/")(
{
"form_data": json.dumps(
{
"slice_id": slc.id,
"time_range_endpoints": (
utils.TimeRangeEndpoint.INCLUSIVE.value,
utils.TimeRangeEndpoint.EXCLUSIVE.value,
),
}
)
}
)

flash(Markup(config["SIP_15_TOAST_MESSAGE"].format(url=url)))

error_redirect = "/chart/list/"
try:
datasource_id, datasource_type = get_datasource_info(
datasource_id, datasource_type, form_data
Loading