[GitHub] agrawaldevesh commented on a change in pull request #5449: [schema] Updating the tables schema

[GitBox <gi...@apache.org>]

agrawaldevesh commented on a change in pull request #5449: [schema] Updating the tables schema
URL: https://github.com/apache/incubator-superset/pull/5449#discussion_r250889034
 
 

 ##########
 File path: superset/migrations/versions/1d73b15530e7_update_tables.py
 ##########
 @@ -0,0 +1,93 @@
+"""update tables
+
+Revision ID: 1d73b15530e7
+Revises: 1d9e835a84f9
+Create Date: 2018-07-20 11:36:04.535859
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = '1d73b15530e7'
+down_revision = '1d9e835a84f9'
+
+from alembic import op
+import sqlalchemy as sa
+
+from superset.utils import generic_find_uq_constraint_name
+
+
+conv = {
+    'uq': 'uq_%(table_name)s_%(column_0_name)s',
+}
+
+tables = sa.Table(
+    'tables',
+    sa.MetaData(),
+    sa.Column('id', sa.Integer, primary_key=True),
+    sa.Column('database_id', sa.Integer),
+    sa.Column('schema', sa.String(127)),
+    sa.Column('table_name', sa.String(127), nullable=False),
+)
+
+
+def upgrade():
+    bind = op.get_bind()
+    insp = sa.engine.reflection.Inspector.from_engine(bind)
+
+    # Reduce the size of the schema and table_name columns for constraint
+    # viability. Note most engines have a 64 character limit for these fields.
+    with op.batch_alter_table('tables') as batch_op:
+        batch_op.alter_column(
+            'schema',
+            existing_nullable=True,
+            existing_type=sa.String(255),
+            type_=sa.String(127),
+        )
+
+        # Additionally enforce that the table_name column be non-nullable.
+        batch_op.alter_column(
+            'table_name',
+            existing_type=sa.String(250),
+            nullable=False,
+            type_=sa.String(127),
+        )
+
+    # Add the missing uniqueness constraint.
+    with op.batch_alter_table('tables', naming_convention=conv) as batch_op:
+        batch_op.create_unique_constraint(
 
 Review comment:
   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: 15b67b2c6c3c2982f6620fce5d30bd05951458f7 (from the days of caravel) that already actually adds the schema you are shooting for. 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org