Copilot commented on code in PR #64838:
URL: https://github.com/apache/airflow/pull/64838#discussion_r3045460478
##########
airflow-core/src/airflow/migrations/versions/0082_3_1_0_make_bundle_name_not_nullable.py:
##########
@@ -61,61 +63,50 @@ def upgrade():
('dags-folder');
""")
)
- if dialect_name == "sqlite":
- op.execute(text("PRAGMA foreign_keys=OFF"))
- op.execute(
- text("""
- INSERT OR IGNORE INTO dag_bundle (name) VALUES
- ('example_dags'),
- ('dags-folder');
- """)
- )
- conn = op.get_bind()
- with ignore_sqlite_value_error(), op.batch_alter_table("dag", schema=None)
as batch_op:
- conn.execute(
- text(
- """
- UPDATE dag
- SET bundle_name =
- CASE
- WHEN fileloc LIKE '%/airflow/example_dags/%' THEN
'example_dags'
- ELSE 'dags-folder'
- END
- WHERE bundle_name IS NULL
- """
+ sqlite_context = disable_sqlite_fkeys(op) if dialect_name == "sqlite" else
contextlib.nullcontext()
+
+ with sqlite_context:
+ if dialect_name == "sqlite":
+ op.execute(
Review Comment:
`disable_sqlite_fkeys(op)` is already a no-op for non-SQLite, so the
`sqlite_context = ... if ... else contextlib.nullcontext()` indirection is
unnecessary. Consider using `with disable_sqlite_fkeys(op):` directly and
dropping the `contextlib` import/variable to keep the migration consistent with
other usages and reduce branching.
##########
airflow-core/src/airflow/migrations/versions/0084_3_1_0_add_last_parse_duration_to_dag_model.py:
##########
@@ -47,15 +48,11 @@ def upgrade():
def downgrade():
"""Unapply add last_parse_duration to dag model."""
- conn = op.get_bind()
- dialect_name = conn.dialect.name
-
- if dialect_name == "sqlite":
+ if op.get_bind().dialect.name == "sqlite":
# SQLite requires foreign key constraints to be disabled during batch
operations
- conn.execute(text("PRAGMA foreign_keys=OFF"))
- with op.batch_alter_table("dag", schema=None) as batch_op:
- batch_op.drop_column("last_parse_duration")
- conn.execute(text("PRAGMA foreign_keys=ON"))
+ with disable_sqlite_fkeys(op):
+ with op.batch_alter_table("dag", schema=None) as batch_op:
+ batch_op.drop_column("last_parse_duration")
else:
with op.batch_alter_table("dag", schema=None) as batch_op:
batch_op.drop_column("last_parse_duration")
Review Comment:
The explicit `if op.get_bind().dialect.name == "sqlite"` branching is
redundant because `disable_sqlite_fkeys(op)` is already a no-op on non-SQLite.
You can simplify this downgrade to a single `with disable_sqlite_fkeys(op):`
block to avoid duplicated batch-alter logic and extra bind/dialect lookups.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
