You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2021/01/14 01:40:21 UTC
[GitHub] [superset] ktmud opened a new issue #12508: [Explore] New time picker breaks old charts with "x days" in time picker start date
ktmud opened a new issue #12508:
URL: https://github.com/apache/superset/issues/12508
### Expected results
In 0.38 and earlier, if you set "30 days" or "5 months" in start date, 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. Time ranges that break in the new time picker but valid for the old one:
1. "30 days: " -> "30 days ago : now"
3. "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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud closed issue #12508: [Explore] New time picker breaks old charts with "x days" in time picker start date
Posted by GitBox <gi...@apache.org>.
ktmud closed issue #12508:
URL: https://github.com/apache/superset/issues/12508
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on issue #12508: [Explore] New time picker breaks old charts with "x days" in time picker start date
Posted by GitBox <gi...@apache.org>.
ktmud commented on issue #12508:
URL: https://github.com/apache/superset/issues/12508#issuecomment-759889071
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on issue #12508: [Explore] New time picker breaks old charts with "x days" in time picker start date
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on issue #12508:
URL: https://github.com/apache/superset/issues/12508#issuecomment-759892870
This is a good point. When we detect the user time range value is "X [dateunit]" then respond to an error message.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on issue #12508: [Explore] New time picker breaks old charts with "x days" in time picker start date
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on issue #12508:
URL: https://github.com/apache/superset/issues/12508#issuecomment-759885448
I'm thinking of maybe doing some adaptations in `get_since_until` https://github.com/apache/superset/blob/241f380e2efe91ad58106cdb4848e0829f432e1e/superset/utils/date_parser.py#L135 @ktmud
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud edited a comment on issue #12508: [Explore] New time picker breaks old charts with "x days" in time picker start date
Posted by GitBox <gi...@apache.org>.
ktmud edited a comment on issue #12508:
URL: https://github.com/apache/superset/issues/12508#issuecomment-759881884
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on issue #12508: [Explore] New time picker breaks old charts with "x days" in time picker start date
Posted by GitBox <gi...@apache.org>.
ktmud commented on issue #12508:
URL: https://github.com/apache/superset/issues/12508#issuecomment-759881884
I think we should at least adding 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] junlincc commented on issue #12508: [Explore] New time picker breaks old charts with "x days" in time picker start date
Posted by GitBox <gi...@apache.org>.
junlincc commented on issue #12508:
URL: https://github.com/apache/superset/issues/12508#issuecomment-759878046
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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie edited a comment on issue #12508: [Explore] New time picker breaks old charts with "x days" in time picker start date
Posted by GitBox <gi...@apache.org>.
zhaoyongjie edited a comment on issue #12508:
URL: https://github.com/apache/superset/issues/12508#issuecomment-759885448
I'm thinking maybe we can do some adaptations in `get_since_until` https://github.com/apache/superset/blob/241f380e2efe91ad58106cdb4848e0829f432e1e/superset/utils/date_parser.py#L135 @ktmud
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on issue #12508: [Explore] New time picker breaks old charts with "x days" in time picker start date
Posted by GitBox <gi...@apache.org>.
ktmud commented on issue #12508:
URL: https://github.com/apache/superset/issues/12508#issuecomment-759873863
Thanks @michellethomas for spotting this issue!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org