[schema] Updating the tables schema #5449

john-bodley · 2018-07-20T22:19:59Z

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 tables table including:

Modifies the uniqueness logic defined by PR Adding YAML Import-Export for Datasources to CLI #3978 (which was never created in a migration) to include the schema name (previously it was only based on the database/table). This is now handled by a SQLAlchemy event listener as many dialects permit multiple NULL values for columns than can contain NULL.
Removes the pre-add for checking for uniqueness. This is no longer required as the uniqueness constraint defines the same logic. This also only worked when adding a table and thus it was possible to have duplicates names if someone edited the record.
Adds form validation to ensure that the schema name is not part of the table name.
Ensures the table_name column is non-nullable.

Note this migration will fail if the table_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 tables may function (from a query standpoint) if SQL is provided. The following query determines which records are problematic:

SELECT 
    *
FROM 
   tables
WHERE 
   table_name IS NULL

Finally this migration will fail if the tables table is corrupt in terms of the uniqueness. 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 
    database_id,
    `schema`, 
    table_name,
    COUNT(1)
FROM 
    tables
GROUP BY 
    1, 2, 3
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 · 2018-07-21T06:16:48Z

Codecov Report

Merging #5449 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #5449      +/-   ##
==========================================
+ Coverage   59.11%   59.13%   +0.01%     
==========================================
  Files         372      372              
  Lines       23756    23754       -2     
  Branches     2758     2758              
==========================================
+ Hits        14044    14046       +2     
+ Misses       9697     9693       -4     
  Partials       15       15

Impacted Files	Coverage Δ
superset/connectors/sqla/models.py	`78.01% <100%> (ø)`	⬆️
superset/connectors/sqla/views.py	`73.78% <100%> (+3.31%)`	⬆️

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 0d5443e...08b8821. Read the comment docs.

fabianmenges · 2018-07-26T19:19:29Z

superset/connectors/sqla/models.py


-    table_name = Column(String(250))
+    table_name = Column(String(127))


I think this will cause problems. When you create new "Tables" in SQL Lab it will autogenerate very long table names.

@fabianmenges unless the database or tab names are significantly long I suspect that this shouldn't generate very long table names.

fabianmenges · 2018-07-26T19:20:10Z

superset/connectors/sqla/views.py

@@ -243,17 +244,13 @@ class TableModelView(DatasourceModelView, DeleteMixin, YamlExportMixin):  # noqa
        'is_sqllab_view': _('SQL Lab View'),
        'template_params': _('Template parameters'),
    }
+    validators_columns = {


What about SQL Lab tables?

@fabianmenges could you be more specific? It potentially seems wrong to use a period in a datasource name. Based on the logic for auto-generating the SQL Lab table name it should only contain a period if the username, database name, or tab name contain a period.

youngyjd · 2018-09-27T20:58:32Z

quick question: when will this PR be merged? Saw this PR has been open for more than 2 months.

john-bodley · 2018-09-27T21:38:30Z

@youngyjd there's still responses I'm waiting on and it's also gated by #5445.

mistercrunch · 2019-01-25T06:53:01Z

I made the point in #6718 that we may want to allow multiple datasources that point to the same physical table in the database.

The concept of ownership effectively allows a user to "landgrab" a datasource and control it. It's also possible that an employe that has left the company is the owner of a datasource, then you have to ask an admin to grant you ownership of that table... Or if I want to add a metric to a table, I have to ask the owner to make me an owner as well. Also if a table has many owners, it's likely that one person will break someone else's chart or dashboard.

On the other hand, having many datasources around the rides or booking physical table is also confusing. Tradeoffs. I think I could be convinced either way...

agrawaldevesh

Thanks for looking at my PR #6718 which made me look at yours :-)

agrawaldevesh · 2019-01-25T07:38:45Z

superset/migrations/versions/1d73b15530e7_update_tables.py

+
+    # Add the missing uniqueness constraint.
+    with op.batch_alter_table('tables', naming_convention=conv) as batch_op:
+        batch_op.create_unique_constraint(


I noticed that when I did this op.batch_alter_table alone: It didn't really remove the existing unique constraint. Second, the '.schema' on the table showed that a new unique constraint wasn't really created. Usually alembic creates a temp table and copies the old table to it but it didn't do so in this case. So I am curious if you actually did confirm that this unique constraint is effectively getting created for sqlite ?

Also, are you aware of this very old diff: 15b67b2 (from the days of caravel) that already actually adds the schema you are shooting for.

@agrawaldevesh this may because that in some deployments there may have been a uniqueness constraint on the table_name. I'll update the migration logic.

agrawaldevesh · 2019-01-25T07:39:48Z

superset/migrations/versions/1d73b15530e7_update_tables.py

+        batch_op.drop_constraint(
+            generic_find_uq_constraint_name(
+                'tables',
+                {'database_id', 'schema', 'table_name'},


One of the issues with this schema uniqueness is the pesky 'sql' column on the table: Effectively we would ideally like to have two different views of the same table, both with different sql but with the same <database, schema, table_name>.

I think the issue here is that if there were two different views of the same table how would one differentiate between them as datasources. Note in a database construct view names need to be unique.

I didn't mean real 'SQL views'. I meant basically having the same table added twice, but with different 'sql' like so:

Table foo with sql: 'select foo.a, foo.b, count(1) from foo group by a, b'.
And another with Table foo with sql: 'select foo.*, some_function() from foo'

Personally they feel like different views and thus should be represented as different entities in the database table. If they're both called foo it's not apparent to the user which one the datasource is referring to and thus this would lead to confusion.

stale · 2019-05-24T18:29:20Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue .pinned to prevent stale bot from closing the issue.

john-bodley · 2021-01-31T18:20:05Z

Sadly yes. I think we never resolved how to implement the ideal solution for the various Superset metadata databases.

github-actions · 2021-03-12T21:38:26Z

⚠️ @john-bodley Your base branch master has just also updated superset/migrations.