-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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
Conversation
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
5caf5d3
to
18aadbe
Compare
There was a problem hiding this 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.
How about
|
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. |
There was a problem hiding this 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:
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'll vote for a db migration as initially proposed in #12508 |
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 superset/superset/utils/date_parser.py Lines 78 to 79 in 84266bf
If the datetime derived(parse) is to be improved. It is need to specify a fixed format for date parse. |
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:
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. |
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 |
@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). |
@ktmud , I will open a new PR to improve that. |
bc6c5b1
to
9ce6b50
Compare
x_dateunit_in_until = DateRangeMigration.x_dateunit_in_until | ||
|
||
if isinstance(bind.dialect, SQLiteDialect): | ||
try: |
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
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
There was a problem hiding this 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
Co-authored-by: Jesse Yang <[email protected]>
@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 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. |
@betodealmeida , The user is no longer allowed to enter This issue migrates |
SUMMARY
closes: #12508
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
1. "No filter" show in the date-picker control pill
2. deprecated x periods
3. catch arbitrary input
TEST PLAN
UT covered these case
ADDITIONAL INFORMATION