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 2020/03/16 20:56:52 UTC
[GitHub] [incubator-superset] etr2460 opened a new pull request #9313:
Revert "feat: add rolling window support to 'Big Number with Trendline' viz"
etr2460 opened a new pull request #9313: Revert "feat: add rolling window support to 'Big Number with Trendline' viz"
URL: https://github.com/apache/incubator-superset/pull/9313
Reverts apache/incubator-superset#9107
This change broke rendering of NULL values in the Big Number with Trendline viz. NULL values now render as 0, which is misleading. I'm reverting as this is a data accuracy bug and is fairly high priority.
Repro steps:
- Run a query like this in SQL Lab:
```
SELECT *
FROM (
VALUES ('2020-02-29',
4), ('2020-03-01',
1), ('2020-03-02',
NULL), ('2020-03-03',
2), ('2020-03-04',
3)) AS t (ds, value)
```
- Explore it and select the big number with trend line viz
- See 0 instead of NULL in the trendline
<img width="1920" alt="Screen Shot 2020-03-16 at 1 47 37 PM" src="https://user-images.githubusercontent.com/7409244/76799019-f8c60c80-678d-11ea-8cf0-04943dd65b7b.png">
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] mistercrunch commented on issue #9313: Revert
"feat: add rolling window support to 'Big Number with Trendline' viz"
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on issue #9313: Revert "feat: add rolling window support to 'Big Number with Trendline' viz"
URL: https://github.com/apache/incubator-superset/pull/9313#issuecomment-599776614
Sorry about the inconvenience, how about: https://github.com/apache/incubator-superset/pull/9314 ?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 closed pull request #9313: Revert
"feat: add rolling window support to 'Big Number with Trendline' viz"
Posted by GitBox <gi...@apache.org>.
etr2460 closed pull request #9313: Revert "feat: add rolling window support to 'Big Number with Trendline' viz"
URL: https://github.com/apache/incubator-superset/pull/9313
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on issue #9313: Revert
"feat: add rolling window support to 'Big Number with Trendline' viz"
Posted by GitBox <gi...@apache.org>.
john-bodley commented on issue #9313: Revert "feat: add rolling window support to 'Big Number with Trendline' viz"
URL: https://github.com/apache/incubator-superset/pull/9313#issuecomment-599781504
Personally I think we should revert per the guidelines as the pending PR doesn’t have a unit test. We should strive to keep `master` as stable as possible.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on issue #9313: Revert
"feat: add rolling window support to 'Big Number with Trendline' viz"
Posted by GitBox <gi...@apache.org>.
willbarrett commented on issue #9313: Revert "feat: add rolling window support to 'Big Number with Trendline' viz"
URL: https://github.com/apache/incubator-superset/pull/9313#issuecomment-599775533
Preset has been discussing this internally, and we believe this is a one-line fix. We think that counter-indicates a revert, and are happy to provide a PR so that we can roll forward here instead of roll back.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on issue #9313: Revert
"feat: add rolling window support to 'Big Number with Trendline' viz"
Posted by GitBox <gi...@apache.org>.
etr2460 commented on issue #9313: Revert "feat: add rolling window support to 'Big Number with Trendline' viz"
URL: https://github.com/apache/incubator-superset/pull/9313#issuecomment-600376490
closing as the fix was merged
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on issue #9313: Revert
"feat: add rolling window support to 'Big Number with Trendline' viz"
Posted by GitBox <gi...@apache.org>.
willbarrett commented on issue #9313: Revert "feat: add rolling window support to 'Big Number with Trendline' viz"
URL: https://github.com/apache/incubator-superset/pull/9313#issuecomment-599770916
@etr2460 was an effort made to reach out to @mistercrunch or anyone else at Preset to come up with a solution to this bug before issuing a revert?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on issue #9313: Revert
"feat: add rolling window support to 'Big Number with Trendline' viz"
Posted by GitBox <gi...@apache.org>.
etr2460 commented on issue #9313: Revert "feat: add rolling window support to 'Big Number with Trendline' viz"
URL: https://github.com/apache/incubator-superset/pull/9313#issuecomment-599779565
Pausing this pending #9314
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on issue #9313: Revert
"feat: add rolling window support to 'Big Number with Trendline' viz"
Posted by GitBox <gi...@apache.org>.
willbarrett commented on issue #9313: Revert "feat: add rolling window support to 'Big Number with Trendline' viz"
URL: https://github.com/apache/incubator-superset/pull/9313#issuecomment-599783511
I'm working on a unit test for the related PR now. I do not think waiting a small period of time for the fix to become stable is unreasonable. I do think pushing through a revert when engineers responsible for the relevant code are available and responding is a questionable choice. It creates unnecessary friction in the community for only momentary gain.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on issue #9313: Revert
"feat: add rolling window support to 'Big Number with Trendline' viz"
Posted by GitBox <gi...@apache.org>.
etr2460 commented on issue #9313: Revert "feat: add rolling window support to 'Big Number with Trendline' viz"
URL: https://github.com/apache/incubator-superset/pull/9313#issuecomment-599776250
A couple questions (note that regardless of fix difficulty/complexity I'm still strongly in favor of reverting first, then re PR-ing with both the original commit and the fix):
- What is the ETA on the fix?
- Does the fix include a unit test?
- How confident are we that the fix will completely resolve the 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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on issue #9313: Revert
"feat: add rolling window support to 'Big Number with Trendline' viz"
Posted by GitBox <gi...@apache.org>.
etr2460 commented on issue #9313: Revert "feat: add rolling window support to 'Big Number with Trendline' viz"
URL: https://github.com/apache/incubator-superset/pull/9313#issuecomment-599772922
As per the revert guidelines, I've informed the original PR author on the revert, provided concise repro steps, and gotten the revert reviewed.
As this is a regression caused by a new feature and not a bug fix, and because this is a pretty severe data accuracy issue, I moved towards reverting the PR vs attempting a fix. A fix can be added to the original commit, unit test added, PR-ed out, and reviewed pretty quickly
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org