Skip to content
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.

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

Bug fix on clear_number migration script to properly set the default value #34344

Merged
merged 14 commits into from
Sep 20, 2023

Conversation

sungwy
Copy link
Contributor

@sungwy sungwy commented Sep 13, 2023

closes: #34332
related: #34126

As @abhishekbhakat reported, the merged method of enabling 'default=0' produces an autogenerated sql that causes issues on the db, with nullable=False. 'server_default=0' should be used instead here, but server_default=0 results in the creation of a default DB constraint on MSSQL, which cannot be named deterministically, and hence cannot be removed in the downgrade.

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)]

@jscheffl
Copy link
Contributor

Can you add a description what this PR is for? Any relation to a bug or feature?
Otherwise if you experiment-around, maybe mark the PR as DRAFT?

@sungwy sungwy changed the title Bug fix on clear number migration script [DRAFT] Bug fix on clear number migration script Sep 13, 2023
@sungwy sungwy marked this pull request as draft September 13, 2023 19:31
@sungwy
Copy link
Contributor Author

sungwy commented Sep 13, 2023

Can you add a description what this PR is for? Any relation to a bug or feature? Otherwise if you experiment-around, maybe mark the PR as DRAFT?

Sorry about that @jens-scheffler-bosch ! made the requested change.

@sungwy sungwy changed the title [DRAFT] Bug fix on clear number migration script [DRAFT] Bug fix on clear_number migration script to properly set the default value Sep 13, 2023
@sungwy sungwy marked this pull request as ready for review September 14, 2023 00:38
@sungwy sungwy changed the title [DRAFT] Bug fix on clear_number migration script to properly set the default value Bug fix on clear_number migration script to properly set the default value Sep 14, 2023
@sungwy
Copy link
Contributor Author

sungwy commented Sep 14, 2023

tagging @ephraimbuddy @abhishekbhakat and @uranusjr for review

Copy link
Contributor

@phanikumv phanikumv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you fix the conflicts please?

@sungwy
Copy link
Contributor Author

sungwy commented Sep 20, 2023

@phanikumv I've resolved the merge conflict, and tests were passing but 'Mergeable' check was stuck 'In Progress' - triggered another CI run by updating the branch

@hussein-awala hussein-awala merged commit 840c0d7 into apache:main Sep 20, 2023
43 checks passed
@ephraimbuddy ephraimbuddy added this to the Airflow 2.8.0 milestone Oct 3, 2023
@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Oct 3, 2023
if conn.dialect.name == "mssql":
with op.batch_alter_table("dag_run") as batch_op:
batch_op.add_column(sa.Column("clear_number", sa.Integer, default=0))
batch_op.alter_column("clear_number", existing_type=sa.Integer, nullable=False)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing the issue but this doesn't fixed mssql. Is there a reason why you forked the code to support this?
alter_column can take a 'server_default' value as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default value for clear_number column not applied via sqlalchemy
8 participants