fix: save columns reference from sqllab save datasets flow #24248

hughhhh · 2023-05-30T13:33:29Z

SUMMARY

With the deprecation of /sqllab_viz we need to update the API for /datasets to manage saving columns when saving a dataset. Also added a refactor for changing column.name to always be column.column_name to match the backend TableColumn model

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

codecov · 2023-05-30T13:43:27Z

Codecov Report

Merging #24248 (c5c3cd2) into master (c3b5d72) will decrease coverage by 10.69%.
The diff coverage is 72.50%.

❗ Current head c5c3cd2 differs from pull request most recent head 2a1f09e. Consider uploading reports for the commit 2a1f09e to get more accurate results

@@             Coverage Diff             @@
##           master   #24248       +/-   ##
===========================================
- Coverage   68.85%   58.16%   -10.69%     
===========================================
  Files        1901     1901               
  Lines       73969    74000       +31     
  Branches     8119     8116        -3     
===========================================
- Hits        50931    43044     -7887     
- Misses      20927    28845     +7918     
  Partials     2111     2111

Flag	Coverage Δ
hive	`53.93% <69.44%> (?)`
javascript	`55.65% <25.00%> (-0.01%)`	⬇️
postgres	`?`
presto	`53.83% <63.88%> (?)`
python	`60.88% <77.77%> (-22.25%)`	⬇️
sqlite	`?`
unit	`54.62% <66.66%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ckages/superset-ui-chart-controls/src/constants.ts	`100.00% <ø> (ø)`
...erset-ui-chart-controls/src/utils/columnChoices.ts	`100.00% <ø> (ø)`
...packages/superset-ui-core/src/query/types/Query.ts	`100.00% <ø> (ø)`
...d/src/SqlLab/components/SaveDatasetModal/index.tsx	`50.00% <0.00%> (ø)`
superset-frontend/src/SqlLab/fixtures.ts	`100.00% <ø> (ø)`
...rset-frontend/src/explore/components/SaveModal.tsx	`35.08% <ø> (+0.30%)`	⬆️
superset/databases/utils.py	`28.88% <ø> (-53.34%)`	⬇️
superset/datasets/api.py	`47.07% <0.00%> (-41.59%)`	⬇️
superset/models/sql_lab.py	`75.10% <ø> (-2.82%)`	⬇️
superset/result_set.py	`83.09% <ø> (-14.79%)`	⬇️
... and 17 more

... and 284 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

betodealmeida · 2023-05-30T20:15:55Z

superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx

@@ -218,7 +220,9 @@ export const SaveDatasetModal = ({
        ...formDataWithDefaults,
        datasource: `${datasetToOverwrite.datasetid}__table`,
        ...(defaultVizType === 'table' && {
-          all_columns: datasource?.columns?.map(column => column.column_name),
+          all_columns: datasource?.columns?.map(
+            column => column.name || column.column_name,


I've asked this before, but I don't remember the answer. Why can't we standardize this? This is definitely going to cause bugs in the future, having two possible options for the attribute name.

Can't we normalize the schema early, if they have to be stored differently, instead of normalizing it late here?

looking into this now @betodealmeida, i think it was just a difference between how sqllab consumes columns vs. explore

eschutho · 2023-05-30T21:56:46Z

@hughhhh can you add some tests on this for the bugs fixed? Thanks!

eschutho

let's add some tests for this specific use case since it's not on the codebase already.

hughhhh · 2023-05-31T21:41:15Z

superset/queries/dao.py

@@ -59,6 +59,8 @@ def update_saved_query_exec_info(query_id: int) -> None:
    def save_metadata(query: Query, payload: Dict[str, Any]) -> None:
        # pull relevant data from payload and store in extra_json
        columns = payload.get("columns", {})
+        for col in columns:
+            col["column_name"] = col.pop("name")


from the query execution the dict comes back column[name], and decided to update the naming here before returning it to the client

eschutho · 2023-06-14T22:00:41Z

@hughhhh does this break any apis? i.e., change the schema for the request or response?

john-bodley · 2023-06-14T22:02:21Z

@hughhhh what's the main reasoning behind renaming the column name to column_name? It seems like the column_ prefix if superfluous and inconsistent, i.e., we now have column_name and type.

eschutho · 2023-06-14T22:35:45Z

/testenv up

github-actions · 2023-06-14T22:36:28Z

@eschutho Container image not yet published for this PR. Please try again when build is complete.

github-actions · 2023-06-14T22:36:29Z

@eschutho Ephemeral environment creation failed. Please check the Actions logs for details.

hughhhh · 2023-06-15T02:30:48Z

@hughhhh what's the main reasoning behind renaming the column name to column_name? It seems like the column_ prefix if superfluous and inconsistent, i.e., we now have column_name and type.

@john-bodley

The main reason is actually keep consistency across the app when it comes to Column objects/dicts, since we we've been incorporated query objects inside explore there has been tons of places where we have conditional logic to manage SqlaTables vs Query (example here)[https://github.com//pull/24248/files#diff-407b1e72d7f4462c85eca33d215cd9981b18db050f649dfaaac5748a262d09b8R503] which has also caused a few bugs whenever a user goes from sqllab -> explore. So wanted to create a consistent convention. I picked .column_name since this it equivalent to our TableColumn model field.

hughhhh · 2023-06-15T02:35:32Z

@hughhhh does this break any apis? i.e., change the schema for the request or response?

@eschutho

Yea the column object now have a different key for name it's now column_name, I can add back the key name for clients that might be relying on the key thats not coming through our UI

eschutho · 2023-06-15T18:41:27Z

Yea the column object now have a different key for name it's now column_name, I can add back the key name for clients that might be relying on the key thats not coming through our UI

That would be great. Yeah, we should always keep the apis backward compatible. Even if the app has a breaking change, the apis have their own versioning and the request/response schemas should be backward compatible until we can bump to the next version of the api.

eschutho

LGTM. I agree with @john-bodley that the column.column_name property sounds redundant over just name but maybe it's something we can revisit for the v3 api?

…ix-save-ds-in-sqllab

fix save columns refernece

41a0312

pull-request-size bot added the size/S label May 30, 2023

hughhhh added 3 commits May 30, 2023 13:45

ok

844fbe9

allow schema to be null

02c01c2

add support for legacy payload

ef6cead

pull-request-size bot added size/M and removed size/S labels May 30, 2023

hughhhh added 4 commits May 30, 2023 11:54

Merge branch 'master' into fix-save-ds-in-sqllab

b257562

set default

8769d6e

fix expression

f0e3734

ok

22223a7

hughhhh requested review from betodealmeida and villebro May 30, 2023 20:09

betodealmeida reviewed May 30, 2023

View reviewed changes

eschutho requested changes May 30, 2023

View reviewed changes

hughhhh added 3 commits May 30, 2023 23:05

remove core change

84e307b

update reference from API for column.column_name to match python column

32920fd

ok

a107cee

hughhhh commented May 31, 2023

View reviewed changes

hughhhh added 2 commits May 31, 2023 23:15

ok

fc741fa

ok

49e10d2

pull-request-size bot added size/L and removed size/M labels May 31, 2023

hughhhh added 2 commits June 1, 2023 17:36

remove unneeded test

29a224f

refactor on removing columns being sent on ds creation

510cd29

pull-request-size bot added size/M and removed size/L labels Jun 1, 2023

revert test

e69c1a0

hughhhh added 6 commits June 14, 2023 16:52

linting

8189c93

oops

8895cff

1 more

4454cf3

fix more presto test

d96fbfd

ok

bb8921a

ok fix

cb6677d

john-bodley requested a review from michael-s-molina June 14, 2023 21:56

omg

6d3d955

reference grandparent

a15cead

hughhhh force-pushed the fix-save-ds-in-sqllab branch 2 times, most recently from 311d07b to 3fc4ea9 Compare June 15, 2023 19:21

eschutho approved these changes Jun 15, 2023

View reviewed changes

hughhhh force-pushed the fix-save-ds-in-sqllab branch from 3fc4ea9 to 3580eb7 Compare June 15, 2023 20:30

make schema backwards compatible

eeec51f

hughhhh force-pushed the fix-save-ds-in-sqllab branch from 3580eb7 to eeec51f Compare June 15, 2023 20:48

hughhhh added 2 commits June 20, 2023 16:46

Merge branch 'master' of https://github.com/preset-io/superset into f…

743746e

…ix-save-ds-in-sqllab

change refernce

2a1f09e

hughhhh merged commit 93e1db4 into apache:master Jun 20, 2023
29 checks passed

hughhhh deleted the fix-save-ds-in-sqllab branch June 20, 2023 17:54

villebro mentioned this pull request Jun 21, 2023

fix(oracle): denormalize column names where applicable #24471

Merged

9 tasks

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.0.0 labels Mar 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: save columns reference from sqllab save datasets flow #24248

fix: save columns reference from sqllab save datasets flow #24248

hughhhh commented May 30, 2023 •

edited

Loading

codecov bot commented May 30, 2023 •

edited

Loading

betodealmeida May 30, 2023

hughhhh May 31, 2023

eschutho commented May 30, 2023

eschutho left a comment

hughhhh May 31, 2023 •

edited

Loading

eschutho commented Jun 14, 2023

john-bodley commented Jun 14, 2023

eschutho commented Jun 14, 2023

github-actions bot commented Jun 14, 2023

github-actions bot commented Jun 14, 2023

hughhhh commented Jun 15, 2023 •

edited

Loading

hughhhh commented Jun 15, 2023 •

edited

Loading

eschutho commented Jun 15, 2023

eschutho left a comment

fix: save columns reference from sqllab save datasets flow #24248

fix: save columns reference from sqllab save datasets flow #24248

Conversation

hughhhh commented May 30, 2023 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented May 30, 2023 • edited Loading

Codecov Report

betodealmeida May 30, 2023

Choose a reason for hiding this comment

hughhhh May 31, 2023

Choose a reason for hiding this comment

eschutho commented May 30, 2023

eschutho left a comment

Choose a reason for hiding this comment

hughhhh May 31, 2023 • edited Loading

Choose a reason for hiding this comment

eschutho commented Jun 14, 2023

john-bodley commented Jun 14, 2023

eschutho commented Jun 14, 2023

github-actions bot commented Jun 14, 2023

github-actions bot commented Jun 14, 2023

hughhhh commented Jun 15, 2023 • edited Loading

hughhhh commented Jun 15, 2023 • edited Loading

eschutho commented Jun 15, 2023

eschutho left a comment

Choose a reason for hiding this comment

hughhhh commented May 30, 2023 •

edited

Loading

codecov bot commented May 30, 2023 •

edited

Loading

hughhhh May 31, 2023 •

edited

Loading

hughhhh commented Jun 15, 2023 •

edited

Loading

hughhhh commented Jun 15, 2023 •

edited

Loading