[GitHub] [airflow] ashb commented on pull request #19808: Refactor dangling row check to use SQLA queries

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

ashb commented on pull request #19808:
URL: https://github.com/apache/airflow/pull/19808#issuecomment-978210560


   > Honestly I feel this is a massive degrade on readability, but I guess that’s more or less how ORMs work in general to balance between code duplication and hairy string manipulation.
   
   Yeah, I kind of agree, but to add the XCom migration check (which I've already got written, just not in this PR I had to make a change like this:
   
   ```diff
   -        source_to_dag_run_join_cond = and_(
   -            source_table.c.dag_id == dagrun_table.c.dag_id,
   -            source_table.c.execution_date == dagrun_table.c.execution_date,
   -        )
   -        invalid_rows_query = (
   -            session.query(source_table.c.dag_id, source_table.c.task_id, source_table.c.execution_date)
   -            .select_from(outerjoin(source_table, dagrun_table, source_to_dag_run_join_cond))
   -            .filter(dagrun_table.c.dag_id.is_(None))
   +        if "task_id" in fk_target.columns:
   +            join_condition &= to_migrate.c.task_id == fk_target.c.task_id
   +            query = session.query(
   +                to_migrate.c.dag_id, to_migrate.c.task_id, to_migrate.c.execution_date, to_migrate.c.task_id
   +            )
   +        else:
   +            query = session.query(to_migrate.c.dag_id, to_migrate.c.task_id, to_migrate.c.execution_date)
   +
   +        if "execution_date" in fk_target.columns:
   +            join_target = fk_target
   +            join_condition &= to_migrate.c.execution_date == fk_target.c.execution_date
   +        else:
   +            # Target Table doesn't have execution_date column (anymore?) i.e. TaskInstance after 2.2.0
   +            join_target = fk_target.join(
   +                dagrun_table,
   +                and_(
   +                    dagrun_table.c.dag_id == fk_target.c.dag_id, dagrun_table.c.run_id == fk_target.c.run_id
   +                ),
   +                isouter=True,
   +            )
   +            join_condition &= join_target.c.dag_run_execution_date == to_migrate.c.execution_date
   ```
   
   Which makes it _even less readable_, but the other option is that we have to have a lot of if/else and cases for Xcom pre 2.2, Xcom post 2.2, TI, etc.
   
   


-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org