-
Notifications
You must be signed in to change notification settings - Fork 40
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
Add sql directory parser and dag generator #836
Conversation
This is a very early draft of how it could look like using multiple cc @dimberman |
dabb6a6
to
28e63ec
Compare
Failed checks are unrelated. I'll add unit tests once we agree to a specific strategy. I tested it by installing the following deps:
You can then run the following examples as below:
|
@dimberman @tatiana PTAL Note that we can add meta data to the sql files and use
EDIT: Please see decision doc for pros and cons |
Markdown link check should be fixed by #842 |
Codecov ReportBase: 94.46% // Head: 94.46% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## main #836 +/- ##
=======================================
Coverage 94.46% 94.46%
=======================================
Files 47 47
Lines 2113 2113
Branches 229 229
=======================================
Hits 1996 1996
Misses 83 83
Partials 34 34 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
@feluelle I like how simple and how explicit this approach is. Let's wait for other's feedback 🤞 |
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.
@feluelle given the pros and cons we described, are we only going with the "generate transform_file function" option? Or would the idea be that the user can decide whether to generate or simply render at runtime?
I think given the speeds we've seen I don't think that "it renders faster" is a huge concern here. We were able to render 1000 tasks in pretty much the same amount of time.
The debugging argument also seems a bit iffy to me. Ultimately the error either way is going to be "could not parse at ..../foo.sql.py", I don't think it really matters which line in the airflow DAG starts the task. Either way the user is going to need to debug the SQL file itself.
Also with the aql.transform_file
approach the DAG would be essentially unreadable when you get too 100 or 1000 tasks. It would generate a monstrously large DAG that would be really painful to edit.
I think it makes sense to offer both options:
This is correct. I have edited my comment in GitHub to link to the notion doc.
Same here. Please let us have this discussion in this comment of the docs.
The point is that you should not edit it as it gets overwritten every time we regenerate the DAG file. This is indeed one of the "Cons" of this approach, see this block in the docs. With the new dataset support in Airflow and astro-sdk, we can also easily split it into multiple DAGs 👍 (we should probably do that anyway, even if not meant to be edited.) |
c1544e9
to
741f8bb
Compare
@dimberman @tatiana I changed the code to use
Another note: |
@dimberman @tatiana please see this slack thread in |
169006b
to
c6902aa
Compare
- move exceptions to its own file - document functions in main
The failing checks are unrelated. I will try to address them separately. |
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.
This looks fantastic @feluelle. well-documented, clean, love it.
@@ -0,0 +1,46 @@ | |||
from __future__ import annotations |
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.
In future, we may be able to rename this module to something jinja related - so we avoid generic utils blowing up. I'm happy for us to do this in a separate PR as well :)
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.
@feluelle minor comments, looking forward to seeing the feedback from our end-users on 0.1! 🎉
- add generated tests dag and sql to .gitignore - fix dependencies - improve tests - regenerate examples - refactor code
- add test for symlinks
# Description ## What is the current behavior? Currently, the tasks template creates `transform_sql` tasks, but we only have `transform_file`. related: #836 ## What is the new behavior? Fix references to `transform_sql` by replacing it with `transform_file` ## Does this introduce a breaking change? No. ### Checklist - [ ] Created tests which fail without the change (if possible) - [ ] Extended the README / documentation, if necessary
Description
What is the current behavior?
Currently, we do not have a way of parsing a directory of sql files and generating a DAG out of it.
closes: #923
related: #897
What is the new behavior?
This PR adds a SQL Directory Parser as well as a DAG Generator with examples.
The current implementation adds minimal configurability. More can be handled in a separate PR.
The following assumptions have been made:
datetime.now()
Does this introduce a breaking change?
No.
Checklist