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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


Reply via email to