-
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, Make It Nullable #46295
AIP-83: Restore Uniqueness Constraint on Logical Date, Make It Nullable #46295
Conversation
…stronomer/airflow into restore-unique-constraint-logical-date
…que-constraint-logical-date
…stronomer/airflow into restore-unique-constraint-logical-date
Need to look at failures. |
…que-constraint-logical-date
I added a couple of changes to fix tests in ways I think make more sense. This looks good to me except the parts involving backfill; deferring those to Daniel. |
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.
Apart from the above comments I think we should add more test for logical_dates as null especially in models.
Let's take the backfill part of this out; we can just do minimal changes to backfill such that tests are passing. Backfill can be broken if necessary as a result of this and fixed by sneha's PR. But we need to get the constraint change in there. |
Let’s take out the changes in backfill.py from this PR so we can merge the model changes first. Everything else sort of depends on them. This means backfilling would break if you hit a duplicate logical date, but I think it’s fine as long as we fix it before the final release. I’ll do that tomorrow if nobody beats me to it. |
@uranusjr @dstandish I have removed backfill related changes from this PR. We should be good to merge this after CI is green |
ok cool |
airflow/migrations/versions/0032_3_0_0_rename_execution_date_to_logical_date_and_nullable.py
Show resolved
Hide resolved
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.
LGTM. @uranusjr if it also looks good to me, I think we could probably just merge it?
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.
Made a change in tests, I’ll merge this once everything is green.
It's curious/a massive oversight in tests that this is broken but it's only a provider test that picks it up! |
Yep. I would really be curious to see what's the solution - maybe it's just a test case, maybe that's something that it's missing in the unit tests. Or maybe we should bring back all provider tests to be run on core changes - they are disabled now but seems to be breaking things more often. |
The apache#46295 removed the unique constraints but the case when logical date is None is not handled. This one fixes the problem but likely needs some unit tests and fixes in the core
…le (apache#46295) Co-authored-by: Tzu-ping Chung <[email protected]>
closes:#46187
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rst
or{issue_number}.significant.rst
, in newsfragments.