You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "hughhhh (via GitHub)" <gi...@apache.org> on 2023/04/18 22:13:04 UTC
[GitHub] [superset] hughhhh opened a new pull request, #23729: chore: migrate `sqllab_viz` endpoint to api v1
hughhhh opened a new pull request, #23729:
URL: https://github.com/apache/superset/pull/23729
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually 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:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] jfrag1 commented on a diff in pull request #23729: chore: migrate `sqllab_viz` endpoint to api v1
Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on code in PR #23729:
URL: https://github.com/apache/superset/pull/23729#discussion_r1175648103
##########
superset/datasets/api.py:
##########
@@ -95,6 +95,7 @@ class DatasetRestApi(BaseSupersetModelRestApi):
"related_objects",
"duplicate",
"get_or_create_dataset",
+ "sqllab_viz",
Review Comment:
this can be removed
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] hughhhh merged pull request #23729: chore: migrate `sqllab_viz` endpoint to api v1
Posted by "hughhhh (via GitHub)" <gi...@apache.org>.
hughhhh merged PR #23729:
URL: https://github.com/apache/superset/pull/23729
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] betodealmeida commented on a diff in pull request #23729: chore: migrate `sqllab_viz` endpoint to api v1
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #23729:
URL: https://github.com/apache/superset/pull/23729#discussion_r1175827419
##########
superset/datasets/schemas.py:
##########
@@ -49,14 +49,14 @@ class DatasetColumnsPutSchema(Schema):
column_name = fields.String(required=True, validate=Length(1, 255))
type = fields.String(allow_none=True)
advanced_data_type = fields.String(allow_none=True, validate=Length(1, 255))
- verbose_name = fields.String(allow_none=True, Length=(1, 1024))
+ verbose_name = fields.String(allow_none=True, metadata={Length: (1, 1024)})
Review Comment:
I assume you're doing this because Marshmallow deprecated passing arbitrary keys as metadata, and now recommends explicitly using the `metadata` argument, ie:
```python
fields.String(allow_none=True, description="foo") # DEPRECATED
fields.String(allow_none=True, metadata={"description": "foo"}) # RECOMMENDED
```
The reason why the latter is recommended is because the former will silently swallow keywords arguments, which is exactly what's happening here; `Length()` is supposed to be a validator, not metadata. So you want:
```suggestion
verbose_name = fields.String(allow_none=True, validate=Length(1, 1024))
```
##########
superset/datasets/schemas.py:
##########
@@ -71,7 +71,7 @@ class DatasetMetricsPutSchema(Schema):
metric_name = fields.String(required=True, validate=Length(1, 255))
metric_type = fields.String(allow_none=True, validate=Length(1, 32))
d3format = fields.String(allow_none=True, validate=Length(1, 128))
- verbose_name = fields.String(allow_none=True, Length=(1, 1024))
+ verbose_name = fields.String(allow_none=True, metadata={Length: (1, 1024)})
Review Comment:
Same here, the original code is broken and you want:
```suggestion
verbose_name = fields.String(allow_none=True, validate=Length(1, 1024))
```
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] jfrag1 commented on a diff in pull request #23729: chore: migrate `sqllab_viz` endpoint to api v1
Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on code in PR #23729:
URL: https://github.com/apache/superset/pull/23729#discussion_r1173975410
##########
superset-frontend/src/explore/actions/datasourcesActions.ts:
##########
@@ -49,7 +49,7 @@ export function saveDataset({
const {
json: { data },
} = await SupersetClient.post({
- endpoint: '/superset/sqllab_viz/',
+ endpoint: '/api/v1/dataset/sqllab_viz/',
Review Comment:
This one will need be updated as well
##########
tests/integration_tests/sqllab_tests.py:
##########
@@ -485,102 +485,6 @@ def test_pa_conversion_dict(self):
self.assertEqual(len(data), results.size)
self.assertEqual(len(cols), len(results.columns))
- def test_sqllab_viz(self):
Review Comment:
nit: I think it's fine to leave in tests for deprecated endpoints, we can just delete them when they are eventually removed for good
##########
superset-frontend/src/explore/actions/datasourcesActions.ts:
##########
@@ -49,7 +49,7 @@ export function saveDataset({
const {
json: { data },
} = await SupersetClient.post({
- endpoint: '/superset/sqllab_viz/',
+ endpoint: '/api/v1/dataset/sqllab_viz/',
Review Comment:
Also since this is explore, the response payload from `POST /api/v1/dataset/` maybe be missing keys that explore expects to be there in order to work properly. After this post we may need to make a request to `GET /api/v1/dataset/{id}`, which will return all needed keys. See here in a recent PR of mine where I had to do something similar: https://github.com/apache/superset/pull/23678/files#diff-ccfa293ffd1a07eab82f8646fcb2732ff5d001fec273656b18064db5115ac9fbR165
##########
superset-frontend/src/SqlLab/actions/sqlLab.js:
##########
@@ -1511,9 +1511,19 @@ export function createDatasourceFailed(err) {
export function createDatasource(vizOptions) {
return dispatch => {
dispatch(createDatasourceStarted());
+ const { dbId, schema, datasourceName, sql } = vizOptions;
return SupersetClient.post({
- endpoint: '/superset/sqllab_viz/',
- postPayload: { data: vizOptions },
+ endpoint: '/api/v1/dataset/',
+ headers: { 'Content-Type': 'application/json' },
+ body: JSON.stringify({
+ database: dbId,
+ schema: schema,
+ table_name: datasourceName,
+ sql: sql,
+ owners: [1],
Review Comment:
the `owners` key can probably just be omitted from the payload here, then the backend should default to making the current user owner I believe
##########
superset/datasets/schemas.py:
##########
@@ -239,6 +239,21 @@ class GetOrCreateDatasetSchema(Schema):
template_params = fields.String(description="Template params for the table")
+class SqllabVizColumnSchema(Schema):
+ name = fields.String(required=True, description="Name of table")
+ type = fields.String(required=True, description="Name of table")
+ is_dttm = fields.Boolean(required=True, description="Name of table")
+
+
+class SqllabVizSchema(Schema):
Review Comment:
these can be removed
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] jfrag1 commented on a diff in pull request #23729: chore: migrate `sqllab_viz` endpoint to api v1
Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on code in PR #23729:
URL: https://github.com/apache/superset/pull/23729#discussion_r1172826084
##########
superset/datasets/api.py:
##########
@@ -989,3 +1001,120 @@ def get_or_create_dataset(self) -> Response:
exc_info=True,
)
return self.response_422(message=ex.message)
+
+ @expose("/sqllab_viz/", methods=["POST"])
Review Comment:
Instead of creating a new endpoint, could we instead re-use the `POST /api/v1/dataset/` endpoint? It seems to serve the same purpose and has the validation being done in this endpoint automatically built-in
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] codecov[bot] commented on pull request #23729: chore: migrate `sqllab_viz` endpoint to api v1
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #23729:
URL: https://github.com/apache/superset/pull/23729#issuecomment-1513954028
## [Codecov](https://codecov.io/gh/apache/superset/pull/23729?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#23729](https://codecov.io/gh/apache/superset/pull/23729?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f0c64d3) into [master](https://codecov.io/gh/apache/superset/commit/8bd432274a0eed58187473f8c0845fb34a10353e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8bd4322) will **decrease** coverage by `11.39%`.
> The diff coverage is `48.38%`.
> :exclamation: Current head f0c64d3 differs from pull request most recent head 21a4833. Consider uploading reports for the commit 21a4833 to get more accurate results
```diff
@@ Coverage Diff @@
## master #23729 +/- ##
===========================================
- Coverage 68.05% 56.67% -11.39%
===========================================
Files 1922 1922
Lines 74057 74116 +59
Branches 8104 8100 -4
===========================================
- Hits 50403 42006 -8397
- Misses 21579 30033 +8454
- Partials 2075 2077 +2
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `?` | |
| presto | `53.07% <40.42%> (-0.03%)` | :arrow_down: |
| python | `59.39% <40.42%> (-23.68%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `53.01% <40.42%> (-0.03%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/23729?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset-frontend/src/SqlLab/reducers/sqlLab.js](https://codecov.io/gh/apache/superset/pull/23729?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9yZWR1Y2Vycy9zcWxMYWIuanM=) | `35.86% <0.00%> (-1.21%)` | :arrow_down: |
| [...t-frontend/src/explore/actions/saveModalActions.js](https://codecov.io/gh/apache/superset/pull/23729?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvYWN0aW9ucy9zYXZlTW9kYWxBY3Rpb25zLmpz) | `97.01% <ø> (-1.74%)` | :arrow_down: |
| [...rset-frontend/src/explore/components/SaveModal.tsx](https://codecov.io/gh/apache/superset/pull/23729?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9TYXZlTW9kYWwudHN4) | `35.08% <33.33%> (-2.88%)` | :arrow_down: |
| [superset/datasets/api.py](https://codecov.io/gh/apache/superset/pull/23729?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvYXBpLnB5) | `44.76% <39.13%> (-43.24%)` | :arrow_down: |
| [.../src/components/Chart/DrillBy/DrillByMenuItems.tsx](https://codecov.io/gh/apache/superset/pull/23729?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ2hhcnQvRHJpbGxCeS9EcmlsbEJ5TWVudUl0ZW1zLnRzeA==) | `91.30% <50.00%> (-1.24%)` | :arrow_down: |
| [...tend/src/components/Chart/DrillBy/DrillByModal.tsx](https://codecov.io/gh/apache/superset/pull/23729?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ2hhcnQvRHJpbGxCeS9EcmlsbEJ5TW9kYWwudHN4) | `81.11% <80.00%> (+0.38%)` | :arrow_up: |
| [...d/src/SqlLab/components/QueryAutoRefresh/index.tsx](https://codecov.io/gh/apache/superset/pull/23729?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1F1ZXJ5QXV0b1JlZnJlc2gvaW5kZXgudHN4) | `94.59% <94.11%> (+35.97%)` | :arrow_up: |
| [superset-frontend/src/SqlLab/actions/sqlLab.js](https://codecov.io/gh/apache/superset/pull/23729?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9hY3Rpb25zL3NxbExhYi5qcw==) | `64.67% <100.00%> (+0.17%)` | :arrow_up: |
| [...erset-frontend/src/SqlLab/components/App/index.jsx](https://codecov.io/gh/apache/superset/pull/23729?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL0FwcC9pbmRleC5qc3g=) | `83.33% <100.00%> (ø)` | |
| [superset/views/core.py](https://codecov.io/gh/apache/superset/pull/23729?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `36.25% <100.00%> (-39.07%)` | :arrow_down: |
... and [299 files with indirect coverage changes](https://codecov.io/gh/apache/superset/pull/23729/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] villebro commented on pull request #23729: chore: migrate `sqllab_viz` endpoint to api v1
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on PR #23729:
URL: https://github.com/apache/superset/pull/23729#issuecomment-1598231464
@hughhhh @betodealmeida @jfrag1 @Antonio-RiveroMartnez this PR appears to have caused a nasty regression, namely it's now impossible to save a chart from a query that hasn't been saved as a dataset first: saving triggers an "Unknown field." error:
<img width="971" alt="image" src="https://github.com/apache/superset/assets/33317356/d64314ca-4e78-49a3-8065-3e7f5865f4cf">
Interestingly the "Save as dataset" flow works:
<img width="411" alt="image" src="https://github.com/apache/superset/assets/33317356/b2c580a1-5638-4afc-8cd3-d432f19675f3">
This is happening, because in the main "Save" workflow we're sending a POST to `/api/v1/dataset/` with the same type of payload that we were sending previously to `/superset/sqllab_viz/`:
<img width="612" alt="image" src="https://github.com/apache/superset/assets/33317356/f347f3dd-6a33-4949-bfed-3a0e6364506d">
As can be seen here, the schema doesn't support `columns`, nor `template_params`: https://github.com/apache/superset/blob/3e76736874f5a7b1ca35a9f3b13cbb4bc3791f69/superset/datasets/schemas.py#L79-L87
I think we need to DRY up these flows, as it feels unnecessary to have a different code path for saving the dataset in "Save as dataset" vs the main "Save" button..
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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