-
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
chore: Hugh/migrate estimate query cost to v1 #23226
Conversation
…stimate_query_cost-to-v1
…stimate_query_cost-to-v1
unsubscribe.
***@***.***
From: Hugh A. Miles II
Date: 2023-02-28 08:29
To: apache/superset
CC: Subscribed
Subject: [apache/superset] Hugh/migrate estimate query cost to v1 (PR #23226)
SUMMARY
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION
Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
Migration is atomic, supports rollback & is backwards-compatible
Confirm DB migration upgrade and downgrade tested
Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API
You can view, comment on, or merge this pull request online at:
#23226
Commit Summary
f929d56 chore: Migrate /superset/estimate_query_cost/<database_id>/<schema>/ to API v1
a4ec247 Merge branch 'master' of github.com:apache/superset into dm/migrate-estimate_query_cost-to-v1
a7234a8 cleanup
efc3e0e Merge branch 'master' of github.com:apache/superset into dm/migrate-estimate_query_cost-to-v1
c660773 PR comments
File Changes
(9 files)
M docs/static/resources/openapi.json (201)
M superset-frontend/src/SqlLab/actions/sqlLab.js (20)
M superset-frontend/src/SqlLab/reducers/sqlLab.js (2)
M superset/sqllab/api.py (58)
A superset/sqllab/commands/estimate.py (106)
M superset/sqllab/schemas.py (9)
M superset/views/core.py (1)
M tests/integration_tests/sql_lab/api_tests.py (67)
M tests/integration_tests/sql_lab/commands_tests.py (78)
Patch Links:
https://github.com/apache/superset/pull/23226.patch
https://github.com/apache/superset/pull/23226.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>
|
Codecov Report
@@ Coverage Diff @@
## master #23226 +/- ##
==========================================
+ Coverage 67.61% 67.66% +0.05%
==========================================
Files 1907 1908 +1
Lines 73590 73661 +71
Branches 7982 7982
==========================================
+ Hits 49755 49840 +85
+ Misses 21787 21767 -20
- Partials 2048 2054 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
superset/sqllab/api.py
Outdated
$ref: '#/components/responses/401' | ||
403: | ||
$ref: '#/components/responses/403' | ||
404: |
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.
is 404 possible?
…/migrate-estimate_query_cost-to-v1
ExecutePayloadSchema, | ||
QueryExecutionResponseSchema, | ||
) | ||
|
||
@expose("/estimate/", methods=["POST"]) | ||
@protect() |
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.
what do you think about adding this endpoint to a can read on SQLLab
permission name?
…/migrate-estimate_query_cost-to-v1
superset/sqllab/api.py
Outdated
@protect() | ||
@statsd_metrics | ||
@requires_json | ||
@permission_name("get") |
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.
sorry, having second thoughts on this, let's remove it and leave the permission has it is. can estimate query cost on SQLLab
. Also add that permission here: https://github.com/apache/superset/blob/master/superset/security/manager.py#L253
let's add a comment here also: https://github.com/apache/superset/blob/master/UPDATING.md regarding permissions
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.
is that a valid permission? I only see allows_cost_estimate
as a boolean on the Database model?
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.
it should be can you find estimate_query_cost
on ab_permission table?
UPDATING.md
Outdated
@@ -24,6 +24,7 @@ assists people when migrating to a new version. | |||
|
|||
## Next | |||
|
|||
- [23226](https://github.com/apache/superset/pull/23226) Migrated endpoint `/estimate_query_cost/<int:database_id>` to `/api/v1/sqllab/estimate/`. Corresponding permissions are can_estimate_query_cost on SQLLab. Make sure you add/replace the necessary permissions on any custom roles you may have. |
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.
nit: Corresponding permissions are can estimate query cost on SQLLab
docs/static/resources/openapi.json
Outdated
@@ -2627,10 +2628,17 @@ | |||
"type": "string" | |||
}, | |||
"last_saved_by": { | |||
<<<<<<< HEAD |
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.
merge conflict here ^^^
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 after the docs CI fail is fixed
…/migrate-estimate_query_cost-to-v1
SUMMARY
Taking over from this PR
#22910
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION