Visitar URL original
Separate infer_data_interval for data interval timetables by uranusjr · Pull Request #17755 · apache/airflow · GitHub
Skip to content

Separate infer_data_interval for data interval timetables - #17755

Merged
uranusjr merged 1 commit into
apache:mainfrom
uranusjr:aip-39-logical-date-data-interval-shift
Aug 20, 2021
Merged

uranusjr merged 1 commit into
apache:mainfrom
uranusjr:aip-39-logical-date-data-interval-shift

Conversation

@uranusjr

Copy link
Copy Markdown
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
Copy Markdown
Contributor

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.

return DataInterval(start=self._schedule.get_prev(end), end=end)


class DeltaDataIntervalTimetable(_DataIntervalTimetable):

Copy link
Copy Markdown
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
Copy Markdown
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.

3 participants