[schema] Updating the base column schema

[john-bodley]

We've noticed a number of anomalies in our database caused by ill-defined forms and/or table schema definitions. This PR resolves a number of issues related to the columns and table_columns table including:

• Adds the missing legacy constraint to the table_columns table which was added in PR Adding YAML Import-Export for Datasources to CLI #3978 but didn't contain the corresponding migration. Note this requires reducing the size of the column_name column (which has already been resized in the model) for viability purposes.
• Ensures that the column_name column is non-NULL.
• Deletes any orphaned records, i.e., those which are not associated with a datasource/table.

The uniqueness constraint was added to the columns table in this migration.

Note this migration will fail if the table_columns.column_name column is NULL. One must manually fix these records as programmatically trying to remedy these invalid records is difficult as the intent is unclear and the columns may function (from a query standpoint) if the underlying column expression is defined. The following query determines which records are problematic:

SELECT 
    *
FROM 
    table_columns
WHERE 
    column_name IS NULL 

Additionally this migration may truncate the table_columns.column_name column from 256 to 255 characters. Note certain legacy installations have the 256 character size whereas newer installations have the correct 255 character size. The following query determines which records are problematic:

SELECT
    *
FROM 
    table_columns
WHERE
    CHAR_LENGTH(column_name) > 255

Finally this migration will fail if the table_columns table is corrupt in terms of the uniqueness constraint. One must manually consolidate duplicate or non-valid records given there's no way of programmatically removing invalid records. The following query determines whether there are duplicates:

SELECT 
    table_id, 
    column_name,
    COUNT(1)
FROM 
    table_columns
GROUP BY 
    1, 2
HAVING 
    COUNT(1) > 1
ORDER BY 
    COUNT(1) DESC

Note this PR is gated by #5445 and #7084 which ensure that empty strings associated with form-data wont persist in the database and is necessary for ensuring that the relevant entries are non-NULL.

to: @fabianmenges @graceguo-supercat @michellethomas @mistercrunch @timifasubaa

[codecov-io]

Codecov Report

Merging #5452 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master    #5452   +/-   ##
=======================================
  Coverage   64.91%   64.91%           
=======================================
  Files         428      428           
  Lines       20951    20951           
  Branches     2330     2330           
=======================================
  Hits        13601    13601           
  Misses       7226     7226           
  Partials      124      124

Impacted Files	Coverage Δ
superset/connectors/druid/models.py	82.49% <100%> (ø)	⬆️
superset/connectors/sqla/models.py	81.86% <100%> (ø)	⬆️
superset/connectors/base/models.py	89.89% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 48b2af4...a6f2222. Read the comment docs.

[fabianmenges]

Thanks

[mrshu]

I came here to quickly mention that the following sentence added to UPDATING.md has turned out to be a bit of an understatement for us, as we've updated from v29 to v34 over this past weekend:

Depending on the integrity of the data, manual intervention may be required

Only later, when presented with exceptions of the following sort

sqlalchemy.exc.IntegrityError: (MySQLdb._exceptions.IntegrityError) (1062, "Duplicate entry 'week-21' for key 'uq_table_columns_column_name'")
[SQL: ALTER TABLE table_columns ADD CONSTRAINT uq_table_columns_column_name UNIQUE (column_name, table_id)]
(Background on this error at: http://sqlalche.me/e/gkpj)

did we find out that the actual problem was that we had schema columns and so called calculated columns whose lowercase representation turned out to be the same (i.e. week as a schema column and WEEK as a calculated column). Since the constraint added in this PR only considers their lowercased forms, many queries finished with integrity errors. The fix was quite straightforward (ensure there are no overlapping lowercase names for schema and calculated columns) but was quite hard to find.

I was therefore wondering if something like this would be worth adding to UPDATING.md, so that we may prevent someone else's surprise when their new version of Superset does not work after upgrade.

Thanks!

[john-bodley]

Hi Marek, I created #8465 which updates the UPDATING.md to mention that the addition of uniqueness constraints may be case insensitive depending on the database configuration.

…

-John

On Mon, Oct 28, 2019 at 1:55 PM Marek Šuppa ***@***.***> wrote: I came here to quickly mention that the following sentence added to UPDATING.md has turned out to be a bit of an understatement for us, as we've updated from v29 to v34 over this past weekend: Depending on the integrity of the data, manual intervention may be required Only later, when presented with exceptions of the following sort sqlalchemy.exc.IntegrityError: (MySQLdb._exceptions.IntegrityError) (1062, "Duplicate entry 'week-21' for key 'uq_table_columns_column_name'") [SQL: ALTER TABLE table_columns ADD CONSTRAINT uq_table_columns_column_name UNIQUE (column_name, table_id)] (Background on this error at: http://sqlalche.me/e/gkpj) did we find out that the actual problem was that we had schema columns and so called *calculated columns* whose lowercase representation turned out to be the same (i.e. week as a schema column and WEEK as a calculated column). Since the constraint added in this PR only considers their lowercased forms, many queries finished with integrity errors. The fix was quite straightforward (ensure there are no overlapping lowercase names for schema and calculated columns) but was quite hard to find. I was therefore wondering if something like this would be worth adding to UPDATING.md, so that we may prevent someone else's surprise when their new version of Superset does not work after upgrade. Thanks! — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#5452>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABC3BTJZKXSSAN6GSKJEJ3LQQ5GTTANCNFSM4FLFRQKQ> .

[mrshu]

Thanks a lot @john-bodley -- I really appreciate that!