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/02/24 19:03:04 UTC

[GitHub] [incubator-superset] IndexOutOfRange opened a new pull request #9195: [WIP] Add Test SQL button to table explore view

IndexOutOfRange opened a new pull request #9195: [WIP] Add Test SQL button to table explore view
URL: https://github.com/apache/incubator-superset/pull/9195
 
 
   ### CATEGORY
   
   Choose one
   
   - [X] Bug Fix
   - [X] Enhancement (new features, refinement)
   - [ ] Refactor
   - [ ] Add tests
   - [ ] Build / Development Environment
   - [ ] Documentation
   
   ### SUMMARY
   Fix issue https://github.com/apache/incubator-superset/issues/5417
   This add a new button to check user SQL function in the table explore view when :
   - creating a new fields in a table
   - creating a new metrics in a table
   
   - I used the sql_json API to not create a new API
   - Used a popup as the test connection button. 
   - Launch a query SELECT {expression} FROM {table} LIMIT 1 as suggested in the bug description
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   the button is present in the column tab and metric tab. this display a popup saying OK if it seems OK or display the SQL error :
   ![testsql_feature_popup](https://user-images.githubusercontent.com/3482221/75182045-dec17d00-573f-11ea-97ee-ab5e1f9ad49c.png)
   ![testsql_feature](https://user-images.githubusercontent.com/3482221/75182059-e2ed9a80-573f-11ea-91a0-4be04e0fc7a7.png)
   ![testsql_feature_popup_error](https://user-images.githubusercontent.com/3482221/75182068-e5e88b00-573f-11ea-8215-9aa6ea1d3220.png)
   
   
   ### TEST PLAN
   Manual test for the most of it (different column, tables, bases and with user with restricted access or not)
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [X] Has associated issue: Fix #5417 
   - [X] Changes UI : see screenshot
   - [ ] 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


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] IndexOutOfRange commented on issue #9195: [Explore] Add Test SQL button to table explore view

Posted by GitBox <gi...@apache.org>.
IndexOutOfRange commented on issue #9195: [Explore] Add Test SQL button to table explore view
URL: https://github.com/apache/incubator-superset/pull/9195#issuecomment-594775760
 
 
   I don't know if you guys have notification on commit but changes are commited and CI passed.

----------------------------------------------------------------
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 #9195: [Explore] Add Test SQL button to table explore view

Posted by GitBox <gi...@apache.org>.
willbarrett commented on issue #9195: [Explore] Add Test SQL button to table explore view
URL: https://github.com/apache/incubator-superset/pull/9195#issuecomment-593574273
 
 
   @IndexOutOfRange that wording looks much better, thank you for updating. Please do commit the change :)

----------------------------------------------------------------
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 #9195: [Explore] Add Test SQL button to table explore view

Posted by GitBox <gi...@apache.org>.
willbarrett commented on issue #9195: [Explore] Add Test SQL button to table explore view
URL: https://github.com/apache/incubator-superset/pull/9195#issuecomment-592115462
 
 
   By "test SQL" it appears you mean "run this arbitrary SQL" - I think we should make it clear in the UI that the content of the field will be executed.

----------------------------------------------------------------
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] IndexOutOfRange commented on issue #9195: [Explore] Add Test SQL button to table explore view

Posted by GitBox <gi...@apache.org>.
IndexOutOfRange commented on issue #9195: [Explore] Add Test SQL button to table explore view
URL: https://github.com/apache/incubator-superset/pull/9195#issuecomment-592714144
 
 
   Thank you for your feedback. I agree the wording of the button is not clear. I suggest a new wording "Run SQL Expression" with a tooltip indicating "This will run the query to test it".
   ![run_query_tooltip](https://user-images.githubusercontent.com/3482221/75584873-134f7480-5a71-11ea-904f-031f4cd0bf9a.png)
   
   Open to any other/better suggestion. If we agree on wording/Ui modification I can commit corresponding code.
   

----------------------------------------------------------------
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] codecov-io commented on issue #9195: [WIP] Add Test SQL button to table explore view

Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #9195: [WIP] Add Test SQL button to table explore view
URL: https://github.com/apache/incubator-superset/pull/9195#issuecomment-590527926
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9195?src=pr&el=h1) Report
   > Merging [#9195](https://codecov.io/gh/apache/incubator-superset/pull/9195?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/78b1561820df7b3c655ede67fba665964ea932c1?src=pr&el=desc) will **decrease** coverage by `0.19%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/9195/graphs/tree.svg?width=650&token=KsB0fHcx6l&height=150&src=pr)](https://codecov.io/gh/apache/incubator-superset/pull/9195?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff            @@
   ##           master    #9195     +/-   ##
   =========================================
   - Coverage   59.13%   58.93%   -0.2%     
   =========================================
     Files         372      372             
     Lines       11938    11987     +49     
     Branches     2925     2936     +11     
   =========================================
   + Hits         7059     7065      +6     
   - Misses       4699     4742     +43     
     Partials      180      180
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9195?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...uperset-frontend/src/views/chartList/ChartList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/9195/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2NoYXJ0TGlzdC9DaGFydExpc3QudHN4) | `63.39% <0%> (-11.1%)` | :arrow_down: |
   | [...frontend/src/views/dashboardList/DashboardList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/9195/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2Rhc2hib2FyZExpc3QvRGFzaGJvYXJkTGlzdC50c3g=) | `59.34% <0%> (-10.38%)` | :arrow_down: |
   | [...rset-frontend/src/components/ListView/ListView.tsx](https://codecov.io/gh/apache/incubator-superset/pull/9195/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXcvTGlzdFZpZXcudHN4) | `86.02% <0%> (-6.39%)` | :arrow_down: |
   | [superset-frontend/src/components/ListView/utils.ts](https://codecov.io/gh/apache/incubator-superset/pull/9195/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXcvdXRpbHMudHM=) | `94.44% <0%> (-4.02%)` | :arrow_down: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9195?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9195?src=pr&el=footer). Last update [78b1561...8669a75](https://codecov.io/gh/apache/incubator-superset/pull/9195?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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] villebro commented on issue #9195: [Explore] Add Test SQL button to table explore view

Posted by GitBox <gi...@apache.org>.
villebro commented on issue #9195: [Explore] Add Test SQL button to table explore view
URL: https://github.com/apache/incubator-superset/pull/9195#issuecomment-594832401
 
 
   To address @willbarrett 's concern, instead of actually running the query, I wonder if it would be feasible to implement some type of sql statement dry run functionality in `BaseEngineSpec`. For instance, in Postgres one can prefix the query with `EXPLAIN`, which returns the following for a valid query:
   ![image](https://user-images.githubusercontent.com/33317356/75921684-38295a80-5e6a-11ea-9f1f-7ffa904cd241.png)
   When doing the same for an invalid query, it returns the following:
   ![image](https://user-images.githubusercontent.com/33317356/75921766-5f802780-5e6a-11ea-88bb-8a1cc7caca77.png)
   For other dbs without `EXPLAIN`, one could potentially do a `ROLLBACK` at the end or some other trickery.
   
   Personally, I don't particularly like the idea of running a query out of context like this, and can imagine some dbs potentially getting overloaded if the query generates a large number of rows.

----------------------------------------------------------------
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] codecov-io edited a comment on issue #9195: [Explore] Add Test SQL button to table explore view

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #9195: [Explore] Add Test SQL button to table explore view
URL: https://github.com/apache/incubator-superset/pull/9195#issuecomment-590527926
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9195?src=pr&el=h1) Report
   > Merging [#9195](https://codecov.io/gh/apache/incubator-superset/pull/9195?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/78b1561820df7b3c655ede67fba665964ea932c1?src=pr&el=desc) will **decrease** coverage by `0.19%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/9195/graphs/tree.svg?width=650&token=KsB0fHcx6l&height=150&src=pr)](https://codecov.io/gh/apache/incubator-superset/pull/9195?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff            @@
   ##           master    #9195     +/-   ##
   =========================================
   - Coverage   59.13%   58.93%   -0.2%     
   =========================================
     Files         372      373      +1     
     Lines       11938    12014     +76     
     Branches     2925     2945     +20     
   =========================================
   + Hits         7059     7080     +21     
   - Misses       4699     4755     +56     
   + Partials      180      179      -1
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9195?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...uperset-frontend/src/views/chartList/ChartList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/9195/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2NoYXJ0TGlzdC9DaGFydExpc3QudHN4) | `63.39% <0%> (-11.1%)` | :arrow_down: |
   | [...frontend/src/views/dashboardList/DashboardList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/9195/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2Rhc2hib2FyZExpc3QvRGFzaGJvYXJkTGlzdC50c3g=) | `59.34% <0%> (-10.38%)` | :arrow_down: |
   | [...rset-frontend/src/components/ListView/ListView.tsx](https://codecov.io/gh/apache/incubator-superset/pull/9195/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXcvTGlzdFZpZXcudHN4) | `86.02% <0%> (-6.39%)` | :arrow_down: |
   | [superset-frontend/src/components/ListView/utils.ts](https://codecov.io/gh/apache/incubator-superset/pull/9195/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXcvdXRpbHMudHM=) | `94.44% <0%> (-4.02%)` | :arrow_down: |
   | [...erset-frontend/src/SqlLab/components/ResultSet.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9195/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1Jlc3VsdFNldC5qc3g=) | `75.7% <0%> (-1.86%)` | :arrow_down: |
   | [...et-frontend/src/dashboard/components/Dashboard.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9195/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0Rhc2hib2FyZC5qc3g=) | `84.37% <0%> (-0.63%)` | :arrow_down: |
   | [superset-frontend/src/setup/setupClient.js](https://codecov.io/gh/apache/incubator-superset/pull/9195/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwQ2xpZW50Lmpz) | `0% <0%> (ø)` | :arrow_up: |
   | [superset-frontend/src/setup/setupApp.js](https://codecov.io/gh/apache/incubator-superset/pull/9195/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwQXBwLmpz) | | |
   | [superset-frontend/src/utils/parseCookie.ts](https://codecov.io/gh/apache/incubator-superset/pull/9195/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3V0aWxzL3BhcnNlQ29va2llLnRz) | `100% <0%> (ø)` | |
   | [superset-frontend/src/setup/setupApp.ts](https://codecov.io/gh/apache/incubator-superset/pull/9195/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwQXBwLnRz) | `0% <0%> (ø)` | |
   | ... and [1 more](https://codecov.io/gh/apache/incubator-superset/pull/9195/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9195?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9195?src=pr&el=footer). Last update [78b1561...7753024](https://codecov.io/gh/apache/incubator-superset/pull/9195?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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] stale[bot] commented on pull request #9195: [Explore] Add Test SQL button to table explore view

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


   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