Skip to content

[b/390590500] Airflow dates params #766

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

Merged
merged 11 commits into from
Mar 25, 2025

Conversation

vladislav-sidorovich
Copy link
Collaborator

Add date range support to airflow connector.

Usage:

  1. --lookback-days=x -> date range: [now-x, now)
  2. --start-date=x --end-date=y -> date range: [x, y)
  3. -start-date=x --lookback-days=y -> date range: [x-y, x)

If date range is provided, sql queries will be generate for each day in the interval.

@misolt misolt removed their request for review March 3, 2025 13:21
@kpiotrowski
Copy link
Collaborator

-start-date=x --lookback-days=y -> date range: [x-y, x)

This usage is not intuitive, at least for me. Usually when I provide start date I want all the data to be later than this. I think end-date works better with a lookback-days because lookback refers to searching N days in the past.

@vladislav-sidorovich
Copy link
Collaborator Author

-start-date=x --lookback-days=y -> date range: [x-y, x)

This usage is not intuitive, at least for me. Usually when I provide start date I want all the data to be later than this. I think end-date works better with a lookback-days because lookback refers to searching N days in the past.

I do the calculation and log the actual time range to show the date range explicitly. I agree that start-date might be confusing. However the logic behind it is the following: I chose X date in the calendar and go back for Y days, as the result it will be a date range. In other words, there is an ability to move now.
With such logic go back from end is confusing as well.

I'm open to ideas.

There is a good example: https://grafana.com/docs/grafana/latest/dashboards/use-dashboards/#set-dashboard-time-range
For example we can rename start -> from and end - to. So, it will be: from date X look back for Y days.

does it look better?

Copy link
Collaborator

@shevek-google shevek-google left a comment

Choose a reason for hiding this comment

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

My thoughts from discussion:

  • Start should temporally be before end.
  • Can we make the names sufficiently generic that we can have one set of arguments for query logs, airflow logs, etc, in the future?
  • lookback implies with respect to end, so it doesn't "feel" right to say "start" and lookback (because it would be forwards from start). Can we use interval, period, something like that which is agnostic about which endpoint we anchor?

Overall, I appreciate the philosophy that is driving these clarifications, so thank you for asking.

@vladislav-sidorovich
Copy link
Collaborator Author

I will keep only start and end date parameters for now. lookback-days or similar parameter will be introduced in a separate PR.
Eventually these params should be applied to all connectors (metadata and query logs).

@vladislav-sidorovich vladislav-sidorovich merged commit 3dce62d into main Mar 25, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants