-
Notifications
You must be signed in to change notification settings - Fork 14.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AIP-83 Restore uniqueness constraint on logical date and make logical date nullable #46232
Changes from 12 commits
d0e22ad
2b11aed
237360d
6f423c1
18efafb
8f9e076
1c47c75
d3bfe20
0fa2477
75a684a
53d329b
c3e29c7
0830d58
2b4251d
3ec1ecd
5680a76
16cd717
7d6c9f4
02a82a0
83b62bb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -17,9 +17,9 @@ | |||||
# under the License. | ||||||
|
||||||
""" | ||||||
Drop ``execution_date`` unique constraint on DagRun. | ||||||
Make logical_date nullable. | ||||||
|
||||||
The column has also been renamed to logical_date, although the Python model is | ||||||
The column has been renamed to logical_date, although the Python model is | ||||||
not changed. This allows us to not need to fix all the Python code at once, but | ||||||
still do the two changes in one migration instead of two. | ||||||
|
||||||
|
@@ -49,10 +49,15 @@ def upgrade(): | |||||
"execution_date", | ||||||
new_column_name="logical_date", | ||||||
existing_type=TIMESTAMP(timezone=True), | ||||||
existing_nullable=False, | ||||||
existing_nullable=True, | ||||||
) | ||||||
|
||||||
with op.batch_alter_table("dag_run", schema=None) as batch_op: | ||||||
batch_op.drop_constraint("dag_run_dag_id_execution_date_key", type_="unique") | ||||||
batch_op.create_unique_constraint( | ||||||
"dag_run_dag_id_logical_date_key", | ||||||
columns=["dag_id", "logical_date"], | ||||||
) | ||||||
|
||||||
|
||||||
def downgrade(): | ||||||
|
@@ -63,7 +68,9 @@ def downgrade(): | |||||
existing_type=TIMESTAMP(timezone=True), | ||||||
existing_nullable=False, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, I Implemented this change. |
||||||
) | ||||||
|
||||||
with op.batch_alter_table("dag_run", schema=None) as batch_op: | ||||||
batch_op.drop_constraint("dag_run_dag_id_logical_date_key", type_="unique") | ||||||
batch_op.create_unique_constraint( | ||||||
"dag_run_dag_id_execution_date_key", | ||||||
columns=["dag_id", "execution_date"], | ||||||
|
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -1719,7 +1719,7 @@ def create_dagrun( | |||||||
self, | ||||||||
*, | ||||||||
run_id: str, | ||||||||
logical_date: datetime, | ||||||||
logical_date: datetime | None, | ||||||||
data_interval: tuple[datetime, datetime], | ||||||||
conf: dict | None = None, | ||||||||
run_type: DagRunType, | ||||||||
|
@@ -1743,7 +1743,8 @@ def create_dagrun( | |||||||
|
||||||||
:meta private: | ||||||||
""" | ||||||||
logical_date = timezone.coerce_datetime(logical_date) | ||||||||
if logical_date is not None: | ||||||||
logical_date = timezone.coerce_datetime(logical_date) | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
This function handles None on its own There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, I Implemented this change. |
||||||||
|
||||||||
if data_interval and not isinstance(data_interval, DataInterval): | ||||||||
data_interval = DataInterval(*map(timezone.coerce_datetime, data_interval)) | ||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -130,7 +130,7 @@ class DagRun(Base, LoggingMixin): | |||||||||||||||||||||||||||||
id = Column(Integer, primary_key=True) | ||||||||||||||||||||||||||||||
dag_id = Column(StringID(), nullable=False) | ||||||||||||||||||||||||||||||
queued_at = Column(UtcDateTime) | ||||||||||||||||||||||||||||||
logical_date = Column(UtcDateTime, default=timezone.utcnow, nullable=False) | ||||||||||||||||||||||||||||||
logical_date = Column(UtcDateTime, nullable=True) | ||||||||||||||||||||||||||||||
start_date = Column(UtcDateTime) | ||||||||||||||||||||||||||||||
end_date = Column(UtcDateTime) | ||||||||||||||||||||||||||||||
_state = Column("state", String(50), default=DagRunState.QUEUED) | ||||||||||||||||||||||||||||||
|
@@ -179,6 +179,7 @@ class DagRun(Base, LoggingMixin): | |||||||||||||||||||||||||||||
__table_args__ = ( | ||||||||||||||||||||||||||||||
Index("dag_id_state", dag_id, _state), | ||||||||||||||||||||||||||||||
UniqueConstraint("dag_id", "run_id", name="dag_run_dag_id_run_id_key"), | ||||||||||||||||||||||||||||||
UniqueConstraint("dag_id", "logical_date", name="dag_run_dag_id_logical_date_key"), | ||||||||||||||||||||||||||||||
Index("idx_dag_run_dag_id", dag_id), | ||||||||||||||||||||||||||||||
Index( | ||||||||||||||||||||||||||||||
"idx_dag_run_running_dags", | ||||||||||||||||||||||||||||||
|
@@ -1304,8 +1305,14 @@ def verify_integrity(self, *, session: Session = NEW_SESSION) -> None: | |||||||||||||||||||||||||||||
def task_filter(task: Operator) -> bool: | ||||||||||||||||||||||||||||||
return task.task_id not in task_ids and ( | ||||||||||||||||||||||||||||||
self.run_type == DagRunType.BACKFILL_JOB | ||||||||||||||||||||||||||||||
or (task.start_date is None or task.start_date <= self.logical_date) | ||||||||||||||||||||||||||||||
and (task.end_date is None or self.logical_date <= task.end_date) | ||||||||||||||||||||||||||||||
or ( | ||||||||||||||||||||||||||||||
task.start_date is None | ||||||||||||||||||||||||||||||
or (self.logical_date is not None and task.start_date <= self.logical_date) | ||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||
and ( | ||||||||||||||||||||||||||||||
task.end_date is None | ||||||||||||||||||||||||||||||
or (self.logical_date is not None and self.logical_date <= task.end_date) | ||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suspect this "might" be the correct condition. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes this one is correct. Thanks @Lee-W |
||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
created_counts: dict[str, int] = defaultdict(int) | ||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
0f041321ecbb5d059f7a9c85fd5cfba745732eea205dc2e4f5e3b6dd76a9468d | ||
051d91640c4b2c3daa69ed48eb523b42d8149f43c07c8b6e6f36e0a97c56651b |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -355,10 +355,11 @@ def test_dagrun_no_deadlock_with_depends_on_past(self, dag_maker, session): | |
run_id="test_dagrun_no_deadlock_1", | ||
start_date=DEFAULT_DATE, | ||
) | ||
dr2 = dag_maker.create_dagrun_after( | ||
dr, | ||
next_date = DEFAULT_DATE + datetime.timedelta(days=1) | ||
dr2 = dag_maker.create_dagrun( | ||
run_id="test_dagrun_no_deadlock_2", | ||
start_date=DEFAULT_DATE + datetime.timedelta(days=1), | ||
logical_date=next_date, | ||
) | ||
Comment on lines
-354
to
359
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, this shouldn’t need to be changed. How does this fail originally? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
ti1_op1 = dr.get_task_instance(task_id="dop") | ||
dr2.get_task_instance(task_id="dop") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
existing_nullable
is for specifying the nullable property we don’t want to change. Here we want to change nullability.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I Implemented this change.