ephraimbuddy commented on code in PR #26457:
URL: https://github.com/apache/airflow/pull/26457#discussion_r1016512108
##########
airflow/models/taskinstance.py:
##########
@@ -452,6 +452,7 @@ class TaskInstance(Base, LoggingMixin):
queued_by_job_id = Column(Integer)
pid = Column(Integer)
executor_config = Column(ExecutorConfigType(pickler=dill))
+ notes = Column(String(1000))
Review Comment:
About with_variant:
https://docs.sqlalchemy.org/en/14/core/type_api.html#sqlalchemy.types.TypeEngine.with_variant
Let me explain why we need this change.
In your migration file, you have an `else` statement to use `Text` or
`String` depending on whether the DB is MySQL or not. However, the ORM is just
`String`. You can find out the column type created in MySQL when the DB is
created from ORM by running this command `airflow db init` on a clean DB and
compare the column type created when you create the DB from migration file by
running `airflow db upgrade -r heads` on a clean DB as well.
Both ORM column types and migration files should be the same to avoid issues
with column change in the future.
The test is passing because we ignored String columns in type compare see:
https://github.com/apache/airflow/blob/8f152a102baf6851a719e4ad10fbb15e928fa72b/airflow/utils/db.py#L1796-L1802
Another place we have done a similar thing is
https://github.com/apache/airflow/blob/8f152a102baf6851a719e4ad10fbb15e928fa72b/airflow/models/connection.py#L92.
I believe there are other places too.
People creating DB from ORM should have the same column types as those
creating from migration file so we don't have issue when changing the type in
the future
--
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]
