Skip to content
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

Separate infer_data_interval for data interval timetables #17755

Merged

Conversation

uranusjr
Copy link
Member

CronDataIntervalTimetable and DeltaDataIntervalTimetable needs different infer_data_interval implementations because since the 'align' method aligns the time forward, CronDataIntervalTimetable needs to call get_prev one more time than DeltaDataIntervalTimetable to get the correct interval.

CronDataIntervalTimetable and DeltaDataIntervalTimetable needs
different infer_data_interval implementations because since the 'align'
method aligns the time *forward*, CronDataIntervalTimetable needs to
call get_prev one more time than DeltaDataIntervalTimetable to get the
correct interval.
@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Aug 20, 2021
@github-actions
Copy link

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

# run at 1am 25th is between 0am 24th and 0am 25th.
end = self._schedule.get_prev(self._schedule.align(run_after))
return DataInterval(start=self._schedule.get_prev(end), end=end)


class DeltaDataIntervalTimetable(_DataIntervalTimetable):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(not related to this PR)

Should we change the name to DataIntervalWithDeltaTimetable ? no strong opinion

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No strong opinion from me either, this name is not intended to be used directly by users anyway (they can just use schedule_interval). The only “complaint” (a weird term considering I named these classes) I have to this (and CronDataIntervalTimetable) is they are so terribly long.

@uranusjr uranusjr marked this pull request as ready for review August 20, 2021 16:32
@uranusjr uranusjr changed the title Separate infer_data_interval implementations for interval timetables Separate infer_data_interval for data interval timetables Aug 20, 2021
@uranusjr uranusjr merged commit 22e87b3 into apache:main Aug 20, 2021
@uranusjr uranusjr deleted the aip-39-logical-date-data-interval-shift branch August 20, 2021 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full tests needed We need to run full set of tests for this PR to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants