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

fix(explore): deprecated x periods pattern in new time picker value #12552

Merged
merged 15 commits into from
Jan 24, 2021

Conversation

zhaoyongjie
Copy link
Member

@zhaoyongjie zhaoyongjie commented Jan 15, 2021

SUMMARY

closes: #12508

  1. changed "No filter" show in the date-picker control pill.
  2. deprecated x periods, if user input "x [dateunit][s]" response an error.
  3. raise input error when the user input as an arbitrary string.
  4. added [x periods] -> [x periods ago/later] DB migration script

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

1. "No filter" show in the date-picker control pill

image

2. deprecated x periods

image

3. catch arbitrary input

image

TEST PLAN

UT covered these case

ADDITIONAL INFORMATION

@zhaoyongjie zhaoyongjie changed the title fix(explore): deprecated x periods pattern in time_range value fix(explore): deprecated x periods pattern in new time picker value Jan 15, 2021
@codecov-io
Copy link

codecov-io commented Jan 15, 2021

Codecov Report

Merging #12552 (49679b4) into master (0143a98) will decrease coverage by 0.06%.
The diff coverage is 27.27%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12552      +/-   ##
==========================================
- Coverage   66.89%   66.83%   -0.07%     
==========================================
  Files        1020     1021       +1     
  Lines       49905    49967      +62     
  Branches     4888     4890       +2     
==========================================
+ Hits        33384    33394      +10     
- Misses      16396    16448      +52     
  Partials      125      125              
Flag Coverage Δ
cypress 50.90% <85.71%> (-0.01%) ⬇️
javascript 60.91% <0.00%> (-0.01%) ⬇️
python 64.00% <21.42%> (-0.10%) ⬇️

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%> (ø)
...controls/DateFilterControl/frame/AdvancedFrame.tsx 50.00% <75.00%> (+2.38%) ⬆️
superset/utils/date_parser.py 96.85% <93.75%> (+1.72%) ⬆️
...s/controls/DateFilterControl/DateFilterControl.tsx 85.32% <100.00%> (-0.14%) ⬇️

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 0143a98...49679b4. Read the comment docs.

@zhaoyongjie zhaoyongjie reopened this Jan 16, 2021
@junlincc junlincc added explore:time Related to the time filters in Explore hold:testing! On hold for testing labels Jan 16, 2021
Copy link
Member

@junlincc junlincc left a comment

Choose a reason for hiding this comment

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

Screen.Recording.2021-01-16.at.11.28.11.AM.mov

Thanks for the PR! the error msg is not quite human readable or actionable still... @mihir174 we need your help here.

@junlincc junlincc added the need:design-review Requires input/approval from a Designer label Jan 16, 2021
@ktmud
Copy link
Member

ktmud commented Jan 16, 2021

How about

Start time is too ambiguous. Do you mean "30 days ago" or "now + 30 days"?

@junlincc
Copy link
Member

junlincc commented Jan 18, 2021

thanks for the fix! @zhaoyongjie, all changes LGTM!

Question: when i type '3' in the box, the modal populates 2021-01-03, with 3 as date.
vs. when i type in '40', modal takes it as year... what's the logic behind? should we consider both as unclear input and prompt users to be more specific before generating a time range as well?

Screen Shot 2021-01-17 at 6 14 48 PM

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

I agree with the change, but this change will break existing charts relying on the new logic. This is a chart that was working before but now is failing with the new validation:
image
Since it seems dateutil.parser.parse interprets "7 days" as interchangeable with "7 days ago", we should perhaps do a migration that adds the "ago" suffix to old time ranges to avoid breaking them? Alternatively we should potentially only do the validation for new charts, and let old charts keep working with the deprecated ranges.

@ktmud
Copy link
Member

ktmud commented Jan 18, 2021

I'll vote for a db migration as initially proposed in #12508

@zhaoyongjie
Copy link
Member Author

zhaoyongjie commented Jan 19, 2021

thanks for the fix! @zhaoyongjie, all changes LGTM!

Question: when i type '3' in the box, the modal populates 2021-01-03, with 3 as date.
vs. when i type in '40', modal takes it as year... what's the logic behind? should we consider both as unclear input and prompt users to be more specific before generating a time range as well?

Screen Shot 2021-01-17 at 6 14 48 PM

Hi @junlincc

This is a good point. This behaviour looks like this: when the input string is less than 31, it will be derived as the day of current month, when it is greater than 31 it will be derived as the year. This is parsedatetime.Calendar() behavior.

cal = parsedatetime.Calendar()
parsed_dttm, parsed_flags = cal.parseDT(human_readable)

If the datetime derived(parse) is to be improved. It is need to specify a fixed format for date parse.

@zhaoyongjie
Copy link
Member Author

I agree with the change, but this change will break existing charts relying on the new logic. This is a chart that was working before but now is failing with the new validation:
image
Since it seems dateutil.parser.parse interprets "7 days" as interchangeable with "7 days ago", we should perhaps do a migration that adds the "ago" suffix to old time ranges to avoid breaking them? Alternatively we should potentially only do the validation for new charts, and let old charts keep working with the deprecated ranges.

I'm concerned that a db metadata with a large number of charts will require some migration risk to do this migration.

Are we considering compatibility with the old syntax? I would prefer to be compatible with the original syntax.

What do you think about this? @ktmud @villebro

@villebro
Copy link
Member

villebro commented Jan 19, 2021

I'm concerned that a db metadata with a large number of charts will require some migration risk to do this migration.

Are we considering compatibility with the old syntax? I would prefer to be compatible with the original syntax.

It's true that migrations are risky, and migrations should be avoided unless required. However, similar migrations have been done before, and I'm pretty confident we can write a migration that handles this correctly, as it's not that complex. At least we should be able to fix the vast majority of cases that would otherwise break with just a simple regex. The reason I personally prefer to either support the old format or create a migration is because:

  • For SIP-15 there was a deprecation warning that urged people to edit the chart and save it after checking that the new time range endpoints were ok. See e.g. fix(examples): specify 'time_range_endpoints' to prevent toast warning #11417 . Point being we should try to avoid breaking existing charts unless it is unavoidable, especially post 1.0.
  • People tend to learn to learn to use shorthand whenever supported, and if 7 days has historically worked equally well as 7 days ago, I would suspect many charts out there will be using the old notation. This is probably also highly skewed - some users might not even notice, while other's might have always use the deprecated syntax, hence breaking lots of charts for certain users.

If it's not difficult to implement, I would prefer to let old charts work, but force the user to update the time range if they edit the chart. But if that's difficult to implement or gets messy, I'd prefer to do a migration.

@ktmud
Copy link
Member

ktmud commented Jan 19, 2021

I don't think the migration is particularly risky. You can pre-select the list of charts that use the invalid format of date string and update them one by one (for Airbnb, it's only a couple of hundreds such charts among about 16 million).

The query is as simple as a list of RegExp matches:

SELECT
  id,
  params
FROM "slices"
WHERE regexp_like(params, '"time_range": "\d+ (days|weeks|months|years|quarters)')
   AND not regexp_like(params, '"time_range": "\d+.*(ago|before).*:')

For MySQL and SQLite, you can use .op('regexp')(...), for PostgreSQL, you can use .op('similar to')(...).

@ktmud
Copy link
Member

ktmud commented Jan 20, 2021

@zhaoyongjie can we also add debounce to the backend verification API? The API should probably also return 200 instead of 400 when verification failed (400 should be reserved for malformed/invalid payload).

@zhaoyongjie
Copy link
Member Author

@ktmud , I will open a new PR to improve that.

x_dateunit_in_until = DateRangeMigration.x_dateunit_in_until

if isinstance(bind.dialect, SQLiteDialect):
try:
Copy link
Member Author

Choose a reason for hiding this comment

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

SQLite does not support REGEXP syntax by default.
https://www.sqlite.org/lang_expr.html#regexp

except OperationalError:
return

if isinstance(bind.dialect, MySQLDialect):
Copy link
Member Author

Choose a reason for hiding this comment

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

MySQL 5.7 support REGEXP operator, the REGEXP_LIKE function is not supported until MySQL 8.x
https://dev.mysql.com/doc/refman/5.7/en/regexp.html#operator_regexp

@zhaoyongjie zhaoyongjie reopened this Jan 23, 2021
Copy link
Member

@ktmud ktmud left a comment

Choose a reason for hiding this comment

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

LGTM with one more nit

superset/utils/date_parser.py Outdated Show resolved Hide resolved
Co-authored-by: Jesse Yang <jesse.yang@airbnb.com>
@ktmud ktmud merged commit 9e58eb8 into apache:master Jan 24, 2021
@junlincc junlincc removed the hold:testing! On hold for testing label Mar 15, 2021
@betodealmeida
Copy link
Member

@zhaoyongjie I'm writing the notes for 1.1 and saw this PR. I just wanted to clarify something for the notes: this PR doesn't deprecate the usage of x periods, it removes it, right?

Looking at the screenshots it seems that the user cannot input "30 days", since "apply" is disabled. If this were a deprecation the user would see the warning, but would still be able to use the deprecated format, until we eventually removed it in the future.

@zhaoyongjie
Copy link
Member Author

@betodealmeida , The user is no longer allowed to enter x periods

This issue migrates x [periods] to x [periods] last/ago in database

@mistercrunch 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels explore:time Related to the time filters in Explore need:design-review Requires input/approval from a Designer size/L 🚢 1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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