fix: time filter db migration optimization #13015

ktmud · 2021-02-08T22:16:05Z

SUMMARY

Optimizes free-text time range filter db migration added by #12505

Update the validation Exception so it can be exposed to the frontend (when migration hasn't applied).
Update the migration script so it handle cases like 100 years : 30 years ago. Previously it will incorrectly change it to 100 years ago : 30 years later when it should be 100 years ago : 30 years ago.

These are edge cases nonetheless.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

When opening a chart with "100 years" (i.e. periods without "ago" or "before/after"), it shows "Fatal error":

After

The error is more actionable:

(Still need some optimization here in order to render the backend validation error more properly but let's do it in another PR for other fields as well. Ideally the backend validation errors should come back to each control based on field name.)

TEST PLAN

#12552 is for fixing legacy charts. You will not be able to add bad data anymore because of the backend validation. You'd need to either find a legacy bad chart, or manually add one. A quick way to do it is this:

Connect to your Superset db using any database manager

For example I use psql:

psql "postgresql://superset:[email protected]:5432/superset_test"

Run following SQL command:

INSERT INTO
  slices (
    created_on,
    changed_on,
    slice_name,
    datasource_type,
    datasource_name,
    viz_type,
    params,
    perm,
    datasource_id
  )
VALUES
  (
    '2021-01-25T13:21:40.763807',
    '2021-01-25T13:21:40.763807',
    'Test time period',
    'table',
    null,
    'table',
    '{ "compare_lag": "10", "compare_suffix": "o10Y", "country_fieldtype": "cca3", "entity": "country_code", "granularity_sqla": "year", "groupby": [ "country_name" ], "limit": "25", "markup_type": "markdown", "metrics": [ "sum__SP_POP_TOTL" ], "row_limit": 50000, "show_bubbles": true, "time_range": "100 years : 30 years ago", "time_range_endpoints": [ "inclusive", "exclusive" ], "viz_type": "table" }',
    '[examples].[wb_health_population](id:2)',
    2
  );

Note we set the time range to be "100 years : 30 years ago"

Find the ID of the chart you just inserted:

SELECT *
FROM slices
WHERE slice_name like 'Test time period'

Go to /superset/slice/{slice_id}

Before migration, you should see the "Unexpected error" when you open the chart.

After migration, you should see the chart render correctly.

Note: if you have already upgraded superset db, you need to downgrade it first:

superset db downgrade c878781977c6 
superset db upgrade

When in current master, after migration, you will also see the filter being updated to "100 years ago : 30 years later":

With this PR, after migration, you should see the filter being updated to "100 years ago : 30 years ago":

ADDITIONAL INFORMATION

ktmud · 2021-02-08T22:21:20Z

superset/charts/commands/exceptions.py

@@ -14,7 +14,7 @@
 # KIND, either express or implied.  See the License for the
 # specific language governing permissions and limitations
 # under the License.
-from flask_babel import lazy_gettext as _
+from flask_babel import _


Bycatch, lazy text cannot be properly formatted by Marshmallow.

ktmud · 2021-02-08T22:22:59Z

superset/charts/commands/exceptions.py

 class DatabaseNotFoundValidationError(ValidationError):
    """
    Marshmallow validation error for database does not exist
    """

    def __init__(self) -> None:
-        super().__init__(_("Database does not exist"), field_names=["database"])
+        super().__init__(_("Database does not exist"), field_name="database")


Bycatch: should use field_name instead of field_names.

codecov-io · 2021-02-09T01:53:45Z

Codecov Report

Merging #13015 (0be83f0) into master (50fa100) will increase coverage by 14.32%.
The diff coverage is 60.00%.

@@             Coverage Diff             @@
##           master   #13015       +/-   ##
===========================================
+ Coverage   52.64%   66.97%   +14.32%     
===========================================
  Files         480      489        +9     
  Lines       17300    28766    +11466     
  Branches     4481        0     -4481     
===========================================
+ Hits         9108    19266    +10158     
- Misses       8192     9500     +1308

Flag	Coverage Δ
cypress	`?`
python	`66.97% <60.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...s/260bf0649a77_migrate_x_dateunit_in_time_range.py	`0.00% <0.00%> (ø)`
superset/charts/commands/exceptions.py	`92.85% <77.77%> (ø)`
superset/utils/date_parser.py	`96.87% <100.00%> (ø)`
superset-frontend/src/SqlLab/constants.ts
...rset-frontend/src/components/ImportModal/index.tsx
...end/src/dashboard/util/getKeyForFilterScopeTree.js
.../controls/MetricControl/FilterDefinitionOption.jsx
...rc/explore/components/controls/ColorMapControl.jsx
...tend/src/dashboard/containers/DashboardBuilder.jsx
superset-frontend/src/chart/chartReducer.js
... and 944 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb5dc38...0be83f0. Read the comment docs.

zhaoyongjie

LGTM, thanks for fixing it!

zhaoyongjie

One nit, add a test case :

tests/utils/date_parser_tests.py

zhaoyongjie

I will merge when the CI completed

* master: fix: UI toast typo (apache#13026) feat(db engines): add support for Opendistro Elasticsearch (AWS ES) (apache#12602) fix(build): black failing on master, add to required checks (apache#13039) fix: time filter db migration optimization (apache#13015) fix: untranslated text content of Dashboard page (apache#13024) fix(ci): remove signature requirements for commits to master (apache#13034) fix: add alerts and report to default config (apache#12999) docs(changelog): add entries for 1.0.1 (apache#12981) ci: skip cypress if no code changes (apache#12982) chore: add cypress required checks for branch protection (apache#12970) Refresh dashboard list after bulk delete (apache#12945) Updates storybook to version 6.1.17 (apache#13014) feat: Save datapanel state in local storage (apache#12996) fix: added text and changed margins (apache#12923) chore: Swap Slack Url 2 more places (apache#13004)

ktmud added 2 commits February 8, 2021 11:19

Update error message

88f2c10

Fix time range upgrade script

a29817e

pull-request-size bot added the size/M label Feb 8, 2021

ktmud force-pushed the time-filter-migration branch from 6f749e9 to a29817e Compare February 8, 2021 22:16

ktmud changed the title ~~fix: time filter migration optimization~~ fix: time filter db migration optimization Feb 8, 2021

ktmud commented Feb 8, 2021

View reviewed changes

ktmud requested review from zhaoyongjie and villebro February 8, 2021 22:23

fix tests

0907b6f

zhaoyongjie approved these changes Feb 9, 2021

View reviewed changes

zhaoyongjie reviewed Feb 9, 2021

View reviewed changes

tests/utils/date_parser_tests.py Show resolved Hide resolved

Add one more test

0be83f0

zhaoyongjie self-requested a review February 9, 2021 10:26

zhaoyongjie approved these changes Feb 9, 2021

View reviewed changes

zhaoyongjie closed this Feb 9, 2021

zhaoyongjie reopened this Feb 9, 2021

ktmud merged commit bbcb411 into apache:master Feb 9, 2021

ktmud deleted the time-filter-migration branch February 9, 2021 18:49

amitmiran137 pushed a commit to nielsen-oss/superset that referenced this pull request Feb 14, 2021

fix: time filter db migration optimization (apache#13015)

94adcb3

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: time filter db migration optimization #13015

fix: time filter db migration optimization #13015

ktmud commented Feb 8, 2021 •

edited

Loading

ktmud Feb 8, 2021

ktmud Feb 8, 2021

codecov-io commented Feb 9, 2021 •

edited

Loading

zhaoyongjie left a comment

zhaoyongjie left a comment •

edited

Loading

zhaoyongjie left a comment •

edited

Loading

fix: time filter db migration optimization #13015

fix: time filter db migration optimization #13015

Conversation

ktmud commented Feb 8, 2021 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

After

TEST PLAN

ADDITIONAL INFORMATION

ktmud Feb 8, 2021

Choose a reason for hiding this comment

ktmud Feb 8, 2021

Choose a reason for hiding this comment

codecov-io commented Feb 9, 2021 • edited Loading

Codecov Report

zhaoyongjie left a comment

Choose a reason for hiding this comment

zhaoyongjie left a comment • edited Loading

Choose a reason for hiding this comment

zhaoyongjie left a comment • edited Loading

Choose a reason for hiding this comment

ktmud commented Feb 8, 2021 •

edited

Loading

codecov-io commented Feb 9, 2021 •

edited

Loading

zhaoyongjie left a comment •

edited

Loading

zhaoyongjie left a comment •

edited

Loading