[GitHub] [airflow] dstandish commented on a change in pull request #6370: AIRFLOW-5701: Don't clear xcom explicitly before execution

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

dstandish commented on a change in pull request #6370: AIRFLOW-5701: Don't clear xcom explicitly before execution
URL: https://github.com/apache/airflow/pull/6370#discussion_r339287344
 
 

 ##########
 File path: airflow/models/xcom.py
 ##########
 @@ -43,16 +43,14 @@ class XCom(Base, LoggingMixin):
     """
     __tablename__ = "xcom"
 
-    id = Column(Integer, primary_key=True)
-    key = Column(String(512))
+    key = Column(String(512), primary_key=True, nullable=False)
     value = Column(LargeBinary)
-    timestamp = Column(
-        UtcDateTime, default=timezone.utcnow, nullable=False)
-    execution_date = Column(UtcDateTime, nullable=False)
+    timestamp = Column(UtcDateTime, default=timezone.utcnow, nullable=False)
+    execution_date = Column(UtcDateTime, primary_key=True, nullable=False)
 
     # source information
-    task_id = Column(String(ID_LEN), nullable=False)
-    dag_id = Column(String(ID_LEN), nullable=False)
+    task_id = Column(String(ID_LEN), primary_key=True, nullable=False)
+    dag_id = Column(String(ID_LEN), primary_key=True, nullable=False)
 
     __table_args__ = (
         Index('idx_xcom_dag_task_date', dag_id, task_id, execution_date, unique=False),
 
 Review comment:
   @Fokko 2 things
   
   1. it seems we don't need this index anymore, now that there is a primary key with the same columns
   2. i could be wrong (i am new to alembic) but aren't you missing a migration script for users who are upgrading? 
   3. should we add `PrimaryKeyConstraint('dag_id', 'task_id', 'execution_date', 'key')` here to clarify order of index columns?  Although it seems that table structure is entirely managed by alembic, and these table args have no effect, we should probably be consistent (and if i understand correctly, if not specified, primary key cols are ordered as they appear in table def).  

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services