fix default value for new column as autogenerate SQL was missing default 0 #34335
Conversation
|
CC @syun64 |
|
Do we not change generated migrations to make sure it generates right SQL ? |
|
Maybe we can change: airflow/airflow/models/dagrun.py Line 143 in 35818ee to say server_default="0" so that auto-generated migration picks it up. I don't know if it's worth doing at this stage ¯\_(ツ)_/¯. |
| @@ -43,7 +43,7 @@ def upgrade(): | |||
| sa.Column( | |||
| "clear_number", | |||
| sa.Integer, | |||
| default=0, | |||
| server_default="0", | |||
There was a problem hiding this comment.
I think we need to handle MSSQL default in the downgrade if we go with server_default
There was a problem hiding this comment.
The SQL Query generated by this is ALTER TABLE dag_run ADD COLUMN clear_number INTEGER NOT NULL DEFAULT 0; (for postgres, so thinking it would be same for MSSQL as well).
So I think a simple drop should work.
Any resources where I can read more about what you are referring for MSSQL?
There was a problem hiding this comment.
Is support of MSSQL would removed in Airflow 2.8?
There was a problem hiding this comment.
Nevermind, I've found answer in comments
There was a problem hiding this comment.
I've had to spend some time figuring out how to make this work with MSSQL, and I think this is an approach that 'works':
https://github.com/apache/airflow/pull/34344/files - the PR is currently failing because of failing ER diagram and black pre-commit checks, but the Migration CI seems to be working. I don't have access to my laptop right now, but will be able to fix the pre-commit check later this evening... or @abhishekbhakat feel free to adopt the logic into this PR to merge it in, if you are able to get to it faster than I am 👍
Other seemingly more 'correct' ways of handling the migration for MSSQL were failing.
For example, using the following drop_column command still fails for MSSQL:
batch_op.drop_column("clear_number", mssql_drop_default=True)
sqlalchemy.exc.ProgrammingError: (pyodbc.ProgrammingError) ('42000', "[42000] [Microsoft][ODBC Driver 18 for SQL Server][SQL Server]Incorrect syntax near 'constraint'. (102) (SQLExecDirectW)")
[SQL: declare @const_name varchar(256)
select @const_name = QUOTENAME([name]) from sys.default_constraints
where parent_object_id = object_id('dag_run')
and col_name(parent_object_id, parent_column_id) = 'clear_number'
exec('alter table dag_run drop constraint ' + @const_name)]
There was a problem hiding this comment.
Yeah! ER Diagram is failing.
ephraimbuddy
added
the
full tests needed
|
Hi @abhishekbhakat thank you for flagging this. I noticed that my PR was merged with a failing CI on MSSQL, but thought that might have been related to the recent vote from the community to remove MSSQL as a supported backend. Interestingly, the CI tests for airflow db migrate on PostgreSQL passed on the PR, and maybe that's because we are migrating an empty airflow DB in the CI: no records means no null values on the new column. Given that we will be dropping support for MSSQSL (possibly by 2.8.0) I wonder if it would be safe to just disregard the failing tests on MSSQL, with the assumption that it will be removed in 2.8.0? |
I think we should still fix the test since MSSQL support is still there in the CI |
|
I was running over the PR list and other PRs related to this as well and had a hard time until I understood what the field "clear_number" is for - as you have a fix PR for this, can you add some documentation hints into the model description what this is for? When reading the column name at least for me it was not self-explanatory. |
|
Closing this PR as it's completed in #34344. |
closes: #34332
related: #34126