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/05/10 16:47:03 UTC

[GitHub] [incubator-superset] bryanck opened a new pull request #9777: Check for empty dataframes before processing

bryanck opened a new pull request #9777:
URL: https://github.com/apache/incubator-superset/pull/9777


   ### CATEGORY
   
   Choose one
   
   - [X] Bug Fix
   - [ ] Enhancement (new features, refinement)
   - [ ] Refactor
   - [ ] Add tests
   - [ ] Build / Development Environment
   - [ ] Documentation
   
   ### SUMMARY
   <!--- Describe the change below, including rationale and design decisions -->
   
   This is a small change to check for an empty dataframe before accessing it by column name for processing. This allows charts to display an empty results message instead of a cryptic error message.
   
   I also added my company to the README, which I hope is OK.
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   <!--- Skip this if not applicable -->
   
   Before:
   ![Screen Shot 2020-05-10 at 9 43 56 AM](https://user-images.githubusercontent.com/5475421/81505296-fe7a3300-92a2-11ea-8121-3c9dcf1b938f.png)
   
   After:
   ![Screen Shot 2020-05-10 at 9 45 01 AM](https://user-images.githubusercontent.com/5475421/81505298-02a65080-92a3-11ea-9c2e-30500ae7c26c.png)
   
   
   ### TEST PLAN
   <!--- What steps should be taken to verify the changes -->
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [ ] Changes UI
   - [ ] Requires DB Migration.
   - [ ] Confirm DB Migration upgrade and downgrade tested.
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   
   ### REVIEWERS
   


----------------------------------------------------------------
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] [incubator-superset] john-bodley edited a comment on pull request #9777: Check for empty dataframes before processing

Posted by GitBox <gi...@apache.org>.
john-bodley edited a comment on pull request #9777:
URL: https://github.com/apache/incubator-superset/pull/9777#issuecomment-629866989


   @villebro we have these empty DataFrame checks for a subset of visualizations right now. I’m not certain how this ties in with SIP-38 or whether any viz types really support no data, but it seems like we should probably short circuit the logic somewhere upstream to prevent every method requiring said check.
   
   cc @etr2460 for context who has been looking into error reporting.


----------------------------------------------------------------
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] [incubator-superset] bryanck commented on pull request #9777: Check for empty dataframes before processing

Posted by GitBox <gi...@apache.org>.
bryanck commented on pull request #9777:
URL: https://github.com/apache/incubator-superset/pull/9777#issuecomment-629872341


   I did try handling it upstream first, checking before the `get_data` call in `BaseViz`, but some visualizations, such as FilterBox, didn't work with that approach, so I felt it was better to leave it in the viz handler function, at least for now in 0.36.


----------------------------------------------------------------
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] [incubator-superset] bryanck commented on pull request #9777: Check for empty dataframes before processing

Posted by GitBox <gi...@apache.org>.
bryanck commented on pull request #9777:
URL: https://github.com/apache/incubator-superset/pull/9777#issuecomment-660497660


   I can create a new PR for 0.37 if you want, I have that applied to our fork


----------------------------------------------------------------
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] [incubator-superset] villebro commented on pull request #9777: Check for empty dataframes before processing

Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #9777:
URL: https://github.com/apache/incubator-superset/pull/9777#issuecomment-660498493


   @bryanck please do 👍 Also, can you update this PRto add your company to the appropriate section in `INTHEWILD.md`? It has been added since this PR was originally opened (will require a rebase).


----------------------------------------------------------------
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] [incubator-superset] bryanck closed pull request #9777: Check for empty dataframes before processing

Posted by GitBox <gi...@apache.org>.
bryanck closed pull request #9777:
URL: https://github.com/apache/incubator-superset/pull/9777


   


----------------------------------------------------------------
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] [incubator-superset] villebro commented on pull request #9777: Check for empty dataframes before processing

Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #9777:
URL: https://github.com/apache/incubator-superset/pull/9777#issuecomment-660454027


   @bryanck did you ensure that all changed viz' worked correctly with the new logic? I would be inclined to merge this, as the "No Results" notification is much more intuitive than the myriad exceptions that tend to surface otherwise on legacy viz plugins.


----------------------------------------------------------------
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] [incubator-superset] stale[bot] commented on pull request #9777: Check for empty dataframes before processing

Posted by GitBox <gi...@apache.org>.
stale[bot] commented on pull request #9777:
URL: https://github.com/apache/incubator-superset/pull/9777#issuecomment-660435935


   This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue `.pinned` to prevent stale bot from closing 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



---------------------------------------------------------------------
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 pull request #9777: Check for empty dataframes before processing

Posted by GitBox <gi...@apache.org>.
john-bodley commented on pull request #9777:
URL: https://github.com/apache/incubator-superset/pull/9777#issuecomment-629866989


   @villebro we have these empty DataFrame checks for a subset of visualizations right now. I’m not certain how this ties in with SIP-38 or whether any viz types really support no data, but it seems like we should probably short circuit the logic somewhere upstream to prevent every method requiring said check.


----------------------------------------------------------------
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] [incubator-superset] bryanck commented on pull request #9777: Check for empty dataframes before processing

Posted by GitBox <gi...@apache.org>.
bryanck commented on pull request #9777:
URL: https://github.com/apache/incubator-superset/pull/9777#issuecomment-660515343


   Sure, this is done, I have a new PR up at https://github.com/apache/incubator-superset/pull/10361 so I'll close this one


----------------------------------------------------------------
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] [incubator-superset] bryanck commented on pull request #9777: Check for empty dataframes before processing

Posted by GitBox <gi...@apache.org>.
bryanck commented on pull request #9777:
URL: https://github.com/apache/incubator-superset/pull/9777#issuecomment-660497169


   Yes we've been running with this change for a while


----------------------------------------------------------------
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] [incubator-superset] bryanck edited a comment on pull request #9777: Check for empty dataframes before processing

Posted by GitBox <gi...@apache.org>.
bryanck edited a comment on pull request #9777:
URL: https://github.com/apache/incubator-superset/pull/9777#issuecomment-629872341


   I did try handling it upstream first, checking before the `self.get_data` call in `BaseViz.getPayload`, but some visualizations, such as FilterBox, didn't work with that approach, so I felt it was better to leave it in the viz handler function, at least for now in 0.36.


----------------------------------------------------------------
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