diff --git a/docs/installation.rst b/docs/installation.rst index 5a19d0906ac..d41a0cdf1f1 100644 --- a/docs/installation.rst +++ b/docs/installation.rst @@ -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 `_ 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 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"] + } diff --git a/superset/assets/package-lock.json b/superset/assets/package-lock.json index 298fc1c16c0..0e86e8edb22 100644 --- a/superset/assets/package-lock.json +++ b/superset/assets/package-lock.json @@ -12982,11 +12982,10 @@ "dev": true }, "interweave": { - "version": "11.1.0", - "resolved": "https://registry.npmjs.org/interweave/-/interweave-11.1.0.tgz", - "integrity": "sha512-Y4x3K9eYnhgHCLcQls1gyLn0DXDtuMOdArmpjdgrM+Y/kt6IOYOa6AxbqhRPxh+3UaSZZ53APf4SAJIDqn9ORg==", + "version": "11.2.0", + "resolved": "https://registry.npmjs.org/interweave/-/interweave-11.2.0.tgz", + "integrity": "sha512-33h9LOXbT52tMin3IyLBPcd5RbiwroP/Sxr0OamnJJU7A/jh0XtZKGvdcSNKYRC7sLZuDk+ZJ2XVrmkcMU5i6w==", "requires": { - "@types/prop-types": "*", "@types/react": "*", "escape-html": "^1.0.3", "prop-types": "^15.7.2" @@ -13003,9 +13002,9 @@ } }, "react-is": { - "version": "16.9.0", - "resolved": "https://registry.npmjs.org/react-is/-/react-is-16.9.0.tgz", - "integrity": "sha512-tJBzzzIgnnRfEm046qRcURvwQnZVXmuCbscxUO5RWrGTXpon2d4c8mI0D8WE6ydVIm29JiLB6+RslkIvym9Rjw==" + "version": "16.11.0", + "resolved": "https://registry.npmjs.org/react-is/-/react-is-16.11.0.tgz", + "integrity": "sha512-gbBVYR2p8mnriqAwWx9LbuUrShnAuSCNnuPGyc7GJrMVQtPDAh8iLpv7FRuMPFb56KkaVZIYSz1PrjI9q0QPCw==" } } }, diff --git a/superset/assets/package.json b/superset/assets/package.json index d275e865da7..93324e01269 100644 --- a/superset/assets/package.json +++ b/superset/assets/package.json @@ -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", diff --git a/superset/assets/spec/javascripts/messageToasts/components/Toast_spec.jsx b/superset/assets/spec/javascripts/messageToasts/components/Toast_spec.jsx index 72675223cc6..901dee81ca2 100644 --- a/superset/assets/spec/javascripts/messageToasts/components/Toast_spec.jsx +++ b/superset/assets/spec/javascripts/messageToasts/components/Toast_spec.jsx @@ -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(); + const wrapper = mount(); 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 => { diff --git a/superset/assets/src/explore/components/controls/DateFilterControl.jsx b/superset/assets/src/explore/components/controls/DateFilterControl.jsx index eb16064ac86..43fd3b7ac67 100644 --- a/superset/assets/src/explore/components/controls/DateFilterControl.jsx +++ b/superset/assets/src/explore/components/controls/DateFilterControl.jsx @@ -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 ( - {nextState.since}
{nextState.until} + {nextState.since} {endpoints && `(${endpoints[0]})`}
{nextState.until} {endpoints && `(${endpoints[1]})`} } > @@ -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 (
diff --git a/superset/assets/src/explore/controlPanels/sections.jsx b/superset/assets/src/explore/controlPanels/sections.jsx index 7df04886973..db5ea6e77d6 100644 --- a/superset/assets/src/explore/controlPanels/sections.jsx +++ b/superset/assets/src/explore/controlPanels/sections.jsx @@ -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'], ], }; diff --git a/superset/assets/src/explore/controls.jsx b/superset/assets/src/explore/controls.jsx index bfa8586891b..475276d61df 100644 --- a/superset/assets/src/explore/controls.jsx +++ b/superset/assets/src/explore/controls.jsx @@ -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)'), + }, + order_by_entity: { type: 'CheckboxControl', label: t('Order by entity id'), diff --git a/superset/assets/src/messageToasts/components/Toast.jsx b/superset/assets/src/messageToasts/components/Toast.jsx index 02deb3d5e69..bbce2e97be6 100644 --- a/superset/assets/src/messageToasts/components/Toast.jsx +++ b/superset/assets/src/messageToasts/components/Toast.jsx @@ -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} + ); } diff --git a/superset/assets/src/messageToasts/stylesheets/toast.less b/superset/assets/src/messageToasts/stylesheets/toast.less index 227e954a47b..bd1e80dacf9 100644 --- a/superset/assets/src/messageToasts/stylesheets/toast.less +++ b/superset/assets/src/messageToasts/stylesheets/toast.less @@ -23,7 +23,7 @@ bottom: 16px; left: 50%; transform: translate(-50%, 0); - width: 500px; + width: 600px; z-index: 3000; // top of the world } diff --git a/superset/config.py b/superset/config.py index b2632855106..bf2deac511e 100644 --- a/superset/config.py +++ b/superset/config.py @@ -679,6 +679,11 @@ SEND_FILE_MAX_AGE_DEFAULT = 60 * 60 * 24 * 365 # Cache static resources # 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 here.' + 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. diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index 5504eb370c7..de72dd4fe90 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -19,7 +19,7 @@ import logging 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 @@ class TableColumn(Model, BaseColumn): 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 @@ class SqlaTable(Model, BaseDatasource): ) 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 @@ class SqlaTable(Model, BaseDatasource): 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 @@ class SqlaTable(Model, BaseDatasource): 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) diff --git a/superset/utils/core.py b/superset/utils/core.py index a27ff71a0fc..f1115b6e3a6 100644 --- a/superset/utils/core.py +++ b/superset/utils/core.py @@ -33,6 +33,7 @@ from email.mime.image import MIMEImage 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" diff --git a/superset/views/core.py b/superset/views/core.py index 69a4ff1e58a..01c0cdc706e 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -48,6 +48,7 @@ from sqlalchemy import and_, or_, select 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 @@ class Superset(BaseSupersetView): 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 @@ class Superset(BaseSupersetView): 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 diff --git a/superset/views/utils.py b/superset/views/utils.py index 231373537a1..a5d5073c895 100644 --- a/superset/views/utils.py +++ b/superset/views/utils.py @@ -27,7 +27,7 @@ from superset import app, db, viz from superset.connectors.connector_registry import ConnectorRegistry from superset.exceptions import SupersetException from superset.legacy import update_time_range -from superset.utils.core import QueryStatus +from superset.utils.core import QueryStatus, TimeRangeEndpoint FORM_DATA_KEY_BLACKLIST: List[str] = [] if not app.config.get("ENABLE_JAVASCRIPT_CONTROLS"): @@ -135,6 +135,9 @@ def get_form_data(slice_id=None, use_slice_data=False): update_time_range(form_data) + if app.config["SIP_15_ENABLED"]: + form_data["time_range_endpoints"] = get_time_range_endpoints(form_data, slc) + return form_data, slc @@ -198,3 +201,41 @@ def apply_display_max_row_limit( sql_results["data"] = sql_results["data"][:display_limit] sql_results["displayLimitReached"] = True return sql_results + + +def get_time_range_endpoints( + form_data: Dict[str, Any], slc: Optional[models.Slice] +) -> Optional[Tuple[TimeRangeEndpoint, TimeRangeEndpoint]]: + """ + Get the slice aware time range endpoints falling back to the SQL database specific + definition or default if not defined. + + For SIP-15 all new slices use the [start, end) interval which is consistent with the + Druid REST API. + + :param form_data: The form-data + :param slc: The chart + :returns: The time range endpoints tuple + """ + + time_range_endpoints = form_data.get("time_range_endpoints") + + if time_range_endpoints: + return time_range_endpoints + + try: + _, datasource_type = get_datasource_info(None, None, form_data) + except SupersetException: + return None + + if datasource_type == "table": + if slc: + endpoints = slc.datasource.database.get_extra().get("time_range_endpoints") + + if not endpoints: + endpoints = app.config["SIP_15_DEFAULT_TIME_RANGE_ENDPOINTS"] + + start, end = endpoints + return (TimeRangeEndpoint(start), TimeRangeEndpoint(end)) + + return (TimeRangeEndpoint.INCLUSIVE, TimeRangeEndpoint.EXCLUSIVE) diff --git a/superset/viz.py b/superset/viz.py index 0d11a9d07d9..d0b2ecd13d2 100644 --- a/superset/viz.py +++ b/superset/viz.py @@ -302,11 +302,12 @@ class BaseViz(object): # extras are used to query elements specific to a datasource type # for instance the extra where clause that applies only to Tables extras = { - "where": form_data.get("where", ""), + "druid_time_origin": form_data.get("druid_time_origin", ""), "having": form_data.get("having", ""), "having_druid": form_data.get("having_filters", []), "time_grain_sqla": form_data.get("time_grain_sqla", ""), - "druid_time_origin": form_data.get("druid_time_origin", ""), + "time_range_endpoints": form_data.get("time_range_endpoints"), + "where": form_data.get("where", ""), } d = {