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.

Sign up for GitHub

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

Jump to bottom

feat(datasource): remove deleted columns and update column type on metadata refresh #10619

Merged

villebro merged 6 commits into apache:master from preset-io:villebro/remove-missing-cols-on-meta-refresh

Aug 18, 2020

Member

villebro commented Aug 17, 2020 •

edited

Loading

SUMMARY

Currently refreshing table metadata doesn't remove columns that aren't available on the table any longer. In addition, the React Table view doesn't update column types if they've changed. This PR:

removes columns that are not present on table schema (both legacy and React).
updates column type if changed (React; already working on legacy view).
adds toasts for added, removed and update columns to (both legacy and React).
Updates unit tests on React Jest specs to correspond to the new logic (skipped legacy view as I assume it's going to be deprecated).
fix broken style on type pills on the table metadata column on React views (see screenshots).

AFTER

BEFORE

The type pills on the column tab were using an incorrect style, causing white foreground on white background.

TEST PLAN

Local testing + new tests

ADDITIONAL INFORMATION

villebro added 2 commits

August 17, 2020 13:13


          fix: remove missing columns on metadata refresh

fec14d5


          add tests

34d4020

superset-github-bot bot added the preset-io label

pull-request-size bot added the size/L label

villebro commented

View reviewed changes

superset-frontend/spec/javascripts/datasource/DatasourceEditor_spec.jsx

Comment on lines +78 to +100

+                  const columns = [
+                    {
+                      name: 'ds',
+                      type: 'DATETIME',
+                      nullable: true,
+                      default: '',
+                      primary_key: false,
+                    },
+                    {
+                      name: 'gender',
+                      type: 'VARCHAR(32)',
+                      nullable: true,
+                      default: '',
+                      primary_key: false,
+                    },
+                    {
+                      name: 'new_column',
+                      type: 'VARCHAR(10)',
+                      nullable: true,
+                      default: '',
+                      primary_key: false,
+                    },
+                  ];

Member Author

villebro Aug 17, 2020

This is the correct schema coming back from the API (compare with extraColumn above).

villebro commented

View reviewed changes

superset-frontend/spec/javascripts/datasource/DatasourceEditor_spec.jsx

Comment on lines +107 to +116

+                      {
+                        type: 'DATETIME',
+                        description: null,
+                        filterable: false,
+                        verbose_name: null,
+                        is_dttm: true,
+                        expression: '',
+                        groupby: false,
+                        column_name: 'ds',
+                      },

Member Author

villebro Aug 17, 2020

This column is unchanged.

villebro commented

View reviewed changes

superset-frontend/spec/javascripts/datasource/DatasourceEditor_spec.jsx

Comment on lines +117 to +126

+                      {
+                        type: 'VARCHAR(32)',
+                        description: null,
+                        filterable: true,
+                        verbose_name: null,
+                        is_dttm: false,
+                        expression: '',
+                        groupby: true,
+                        column_name: 'gender',
+                      },

Member Author

villebro Aug 17, 2020

This type is updated from VARCHAR(16) to VARCHAR(32)

villebro commented

View reviewed changes

superset-frontend/spec/javascripts/datasource/DatasourceEditor_spec.jsx

Comment on lines +127 to +130

+                      expect.objectContaining({
+                        column_name: 'new_column',
+                        type: 'VARCHAR(10)',
+                      }),

Member Author

villebro Aug 17, 2020

This is a new column.

villebro commented

View reviewed changes

superset-frontend/spec/javascripts/datasource/DatasourceEditor_spec.jsx

Comment on lines +133 to +135

+                  expect(inst.state.databaseColumns).not.toEqual(
+                    expect.arrayContaining([expect.objectContaining({ name: 'name' })]),
+                  );

Member Author

villebro Aug 17, 2020

This is one of the removed columns in the original metadata.

villebro commented

View reviewed changes

superset-frontend/src/datasource/DatasourceEditor.jsx

-                  let { databaseColumns } = this.state;
-                  let hasChanged;
-                  const currentColNames = databaseColumns.map(col => col.column_name);
+                updateColumns(cols) {

Member Author

villebro Aug 17, 2020 •

edited

Loading

Method was renamed to reflect that metadata is updated, not only merged.

villebro commented

View reviewed changes

superset/connectors/sqla/models.py

-                      dbcols = (
-                          db.session.query(TableColumn)
-                          .filter(TableColumn.table == self)
-                          .filter(or_(TableColumn.column_name == col.name for col in table_.columns))

Member Author

villebro Aug 17, 2020

I'm not sure what the intent here was, but I don't see any scenario where we want to bring in columns from another table with the same column names.

villebro changed the title ~~feat(datasource): remove deleted columns and update type metadata on metadata refresh~~ feat(datasource): remove deleted columns and column type on metadata refresh

villebro changed the title ~~feat(datasource): remove deleted columns and column type on metadata refresh~~ feat(datasource): remove deleted columns and update column type on metadata refresh

villebro added 4 commits

August 17, 2020 16:03


          lint and simplify

0af3627


          lint


          reduce locals

e876ec5


          fix label style

0387ca9

willbarrett approved these changes

View reviewed changes

Member

willbarrett left a comment

This looks like a good improvement/fix - I'd like to tag @lilykuang as well who's touched this code recently for a review.

lilykuang approved these changes

View reviewed changes

Member

lilykuang left a comment

LGTM 👍 thank you for the improvement

villebro requested a review from nytai

August 18, 2020 06:03

nytai approved these changes

View reviewed changes

Member

nytai left a comment

lgtm

villebro merged commit ca5dc42 into apache:master

villebro deleted the villebro/remove-missing-cols-on-meta-refresh branch

August 18, 2020 18:58

amitmiran137 pushed a commit to amitmiran137/incubator-superset that referenced this pull request


          Merge branch 'master' into dynamic-plugin-import

cf13c0a

* master: (43 commits)
  feat: Getting fancier with Storybook (apache#10647)
  fix: dedup groupby in viz.py while preserving order (apache#10633)
  feat: bump superset-ui for certified tag (apache#10650)
  feat: setup react page with submenu for datasources listview  (apache#10642)
  feat: add certification to metrics (apache#10630)
  feat(viz-plugins): add date formatting to pivot-table (apache#10637)
  fix: controls scroll issue (apache#10644)
  feat: Allow tests files in  /src (plus Label component tests) (apache#10634)
  fix: remove duplicated params and cache_timeout from list_columns; add viz_type to list_columns (apache#10643)
  chore: splitting button stories into separate stories (apache#10631)
  refactor: remove slice level label_colors from dashboard init load (apache#10603)
  feat: card view bulk select (apache#10607)
  style: Label styling/storybook touchups (apache#10627)
  fix: removing unsupported modal sizes (apache#10625)
  feat(datasource): remove deleted columns and update column type on metadata refresh (apache#10619)
  improve documentation for country maps (apache#10621)
  chore: npm audit fix as of 2020-08-15 (apache#10613)
  feat: dataset REST API for distinct values (apache#10595)
  chore: bump react-redux to 5.1.2, whittling console noise (apache#10602)
  fixing console error about bad html attribute (apache#10604)
  ...

# Conflicts:
#	superset-frontend/src/explore/components/ExploreViewContainer.jsx
#	superset-frontend/src/views/App.tsx
#	superset/config.py

Ofeknielsen pushed a commit to ofekisr/incubator-superset that referenced this pull request


          feat(datasource): remove deleted columns and update column type on me…

f3a4cc2

…tadata refresh (apache#10619)

* fix: remove missing columns on metadata refresh

* add tests

* lint and simplify

* lint

* reduce locals

* fix label style

auxten pushed a commit to auxten/incubator-superset that referenced this pull request


          feat(datasource): remove deleted columns and update column type on me…

1585c3d

…tadata refresh (apache#10619)

* fix: remove missing columns on metadata refresh

* add tests

* lint and simplify

* lint

* reduce locals

* fix label style

mistercrunch added 🏷️ bot 🚢 0.38.0 labels

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🏷️ bot size/L 🚢 0.38.0