[Explore] New time picker breaks old charts with "x days" in time picker start date

[ktmud]

Expected results

In 0.38 and earlier, if you set "30 days" or "5 months" in start date (without "ago", "before", or "last"), it will be automatically converted to the equivalent of "30 days ago". The new time picker will treat it as "30 days after today", causing an Unexpected time range: From date cannot be larger than to date error for some existing charts.

Here are examples of time ranges that will break in the new time picker but are valid for the old one:

1. "30 days: " -> "30 days ago : now"
2. "7 days: 7 days" -> "7 days ago : 7 days after today"

Actual results

Existing charts with "x periods" set in start date should behave the same.

Since the original behavior is a little bit ambiguous, we may actually want to force users to use "30 days ago" or "- 30 days" for this use case. So I think it's OK to keep current logic, but to minimize disruption, we should do a db migration to automatically fix such charts and add a breaking change warning in the v1.0 changelog (albeit numbers users who used this syntax is low).

Environment

Latest master

[ktmud]

Thanks @michellethomas for spotting this issue!

[junlincc]

thanks for reporting!
we are happy to include the proposed change in patch release, however, we recommend users to implement their own db migration to eliminate the migration risk. lmk if this sounds reasonable. @ktmud

[ktmud]

I think we should at least add an entry in UPDATING.md to highlight this breaking change. DB migration script is a low priority considering the low volume of such charts, but it should be eventually committed to the Superset codebase because this is not a company-specific bug.

[zhaoyongjie]

I'm thinking maybe we can do some adaptations in get_since_until

superset/superset/utils/date_parser.py

Line 135 in 241f380

def get_since_until(

@ktmud

[ktmud]

That works too. But I do find the previous behavior too ambiguous and somewhat confusing so would appreciate it if we can properly deprecate it. Maybe even throw an error message when "x periods" are used without the look back/forward descriptors.

[zhaoyongjie]

This is a good point. When we detect the user time range value is "X [dateunit]" then respond to an error message.