[SIP-15] Adding initial framework (#8398)

* [sip-15] Adding initial framework * [toast] Addressing etr2460's comments * [fix] Addressing etr2460's comments
2019-10-28 14:23:12 -07:00 · 2019-10-28 14:23:12 -07:00 · 8b74745f9e
parent f7f0be502d
commit 8b74745f9e
15 changed files with 182 additions and 29 deletions
--- 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 <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.
+
+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"]
+    }
--- 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=="
        }
      }
    },
--- 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",
--- 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(<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 => {
--- 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 (
        <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} />
--- 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'],
  ],
 };

--- 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'),
--- 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}
+        <Interweave content={text} />
      </Alert>
    );
  }
--- 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
 }

--- 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 <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.
--- 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)
--- 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"
--- 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
--- 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)
--- 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 = {