-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
feat: implement create view as functionality in Sqllab #9794
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9794 +/- ##
==========================================
+ Coverage 70.79% 70.80% +0.01%
==========================================
Files 587 183 -404
Lines 30435 17834 -12601
Branches 3152 0 -3152
==========================================
- Hits 21545 12627 -8918
+ Misses 8776 5207 -3569
+ Partials 114 0 -114
Continue to review full report at Codecov.
|
230e094
to
0523ac4
Compare
Codecov Report
@@ Coverage Diff @@
## master #9794 +/- ##
===========================================
- Coverage 70.56% 59.55% -11.01%
===========================================
Files 590 400 -190
Lines 31150 12951 -18199
Branches 3164 3195 +31
===========================================
- Hits 21981 7713 -14268
+ Misses 9060 5057 -4003
- Partials 109 181 +72
Continue to review full report at Codecov.
|
c6ac7c1
to
81755a5
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.
The Python LGTM. Note I would suggest adding a line item to UPDATING.md
noting that downtime may be required for the migration given that the locking of the query
table may be problematic due to the frequency of use.
tests/celery_tests.py
Outdated
|
||
def test_run_sync_query_cta(self): | ||
@parameterized.expand([CtaMethod.TABLE, CtaMethod.VIEW]) |
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 like this pattern and frequently use it with pytest
via @pytest.mark.parametrize
decorator. Unrelated to this change I wonder if we should consider using pytest
as opposed to unittest
.
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.
+1000
I would love to!
Pytest supports unittests frameworks, so it would be easy to bootstrap the migration. Sounds like a really good topic for the next community meetup.
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.
Noted. I added this to the agenda.
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.
This pattern could really help clean up many of our tests 👍
3feacf2
to
b055f02
Compare
Here is what i think might be a better experience as a short term solution: My concern is that there are a lot of save type buttons (save query, CTAS, CVAS) in a zone that has limited real estate especially in smaller windows. Consolidating similar functions has the opportunity to empower users to make decisions quicker by minimizing options. If strong tech constraints exist that dont allow for the suggestions here. I think that this current layout is still worth considering. |
Quick note that SIP-34 does not take CTAS into account. It's probably been missed since it's behind a feature flag (a database configuration flag really...). Personally my incline would be - for now - to expand the current @Steejay what do you think? Also it would be good to find the longer term solution and incorporate to the Figma designs |
@mistercrunch if im not mistaken SIP-34 does include CTAS and CVAS but different nomenclature is used. Designs use the terms "Dynamic" and "Static" instead. lmk if im not understanding this correctly.. Save dropdown is clicked > dropdown reveals Save Query, Save Dataset, and Add to Dashboard > Save Dataset is selected > reveals this modal |
5157c53
to
afbe8c7
Compare
Great feedback, thank you! As a short term solution I've added a database level control button that enables / disables the CVAS not to pollute the space. As a medium / long term solution - we will start looking with design & users on the cleaner solutions on this front. |
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, but a few minor nits. To avoid being blocked by design efforts, perhaps we could put this behind a feature flag for now so as to not put more strain on UX before the design implications have been properly vetted?
tests/celery_tests.py
Outdated
|
||
def test_run_sync_query_cta(self): | ||
@parameterized.expand([CtaMethod.TABLE, CtaMethod.VIEW]) |
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.
This pattern could really help clean up many of our tests 👍
@villebro done, not a feature flag -> but DB level control that allows to add CVAS button to the view. |
Implement create view as button in sqllab Make CVAS configurable
Implement create view as button in sqllab Make CVAS configurable Co-authored-by: bogdan kyryliuk <[email protected]> (cherry picked from commit 3db76c6)
@bkyryliuk can we add a loading state to the "Explore" button while it's creating the data source? Otherwise clicking on it multiple times will create multiple data sources: (The backend should probably also check for duplications, too?) |
Implement create view as button in sqllab Make CVAS configurable Co-authored-by: bogdan kyryliuk <[email protected]>
SUMMARY
Introduces create view as button to the sql lab
![image](https://user-images.githubusercontent.com/5727938/81878896-1caf8f80-953e-11ea-88c7-154dfeaf6c0f.png)
Main use case - simplify the user data viz flow and keep it more maintainable.
This feature is controlled on the database level in the configurations.
![image](https://user-images.githubusercontent.com/5727938/85056758-a3b2e180-b154-11ea-89eb-c1887af5a2ce.png)
TEST PLAN
[x] unit tests
[x] dropbox staging
[x] dropbox production
ADDITIONAL INFORMATION
More screenshots:
![image](https://user-images.githubusercontent.com/5727938/85056926-e5dc2300-b154-11ea-8fba-70742ab8b768.png)
![image](https://user-images.githubusercontent.com/5727938/85057014-04dab500-b155-11ea-98c7-48f149f5c49b.png)