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

Support params argument in aql.render to declare values to SQL Jinja template #125 #254

Merged
merged 3 commits into from
Mar 29, 2022

Conversation

dimberman
Copy link
Collaborator

@dimberman dimberman commented Mar 29, 2022

Context
At the moment, Astro 0.5.1 does not use the params keyword argument. This argument would allow us to overwrite SQL Jinja template values.

Example:

SELECT
   t.table_schema,
   t.table_name,
   CONCAT(t.table_catalog, '.', t.table_schema, '.', t.table_name) table_path
FROM dwh_legacy.information_schema.tables t
WHERE
       t.table_schema IN ({{ params.schemas }})
   AND t.table_type = 'BASE TABLE';
aql.render(path='/usr/local/airflow/include/sql/schemas',
                                      params={'schemas': dwh_schemas})

Currently raises the error:

  File "/sr/local/lib/python3.9/site-packages/jinja2/runtime.py", line 903, in _fail_with_undefined_error
      raise self._undefined_exception(self._undefined_message)
jinja2.exceptions.UndefinedError: 'dict object' has no attribute 'schemas'

Acceptance criteria

  • Users of aql.render can override Jinja template values by using the params argument
  • Have an example_dag which uses params
  • Add at least one unit test which validate this behaviour
  • Update the docstring so it details this feature
  • Ask @mag3141592 (bug reporter) to review the PR

This PR addresses one part of the previous PR that was not correctly implemented, namely the adding params at a task level as well as at the DAG level

addresses #125

@codecov
Copy link

codecov bot commented Mar 29, 2022

Codecov Report

Merging #254 (2ee62e3) into main (23cab8b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #254   +/-   ##
=======================================
  Coverage   90.82%   90.83%           
=======================================
  Files          36       36           
  Lines        1526     1527    +1     
  Branches      256      257    +1     
=======================================
+ Hits         1386     1387    +1     
  Misses        114      114           
  Partials       26       26           
Impacted Files Coverage Δ
src/astro/sql/parsers/sql_directory_parser.py 92.03% <100.00%> (+0.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 23cab8b...2ee62e3. Read the comment docs.

Comment on lines 123 to 125
aql.render(
"single_task_dag", input_table=input_table, params={"col": "rooms"}
)
Copy link
Collaborator

@kaxil kaxil Mar 29, 2022

Choose a reason for hiding this comment

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

Please add a separate unit test (or add to an existing test) to check that using {{ params.col }} produces room

@kaxil
Copy link
Collaborator

kaxil commented Mar 29, 2022

Cool, only 1 item remaining - #254 (comment)

@kaxil kaxil merged commit 4528162 into main Mar 29, 2022
@kaxil kaxil deleted the add-params-to-render branch March 29, 2022 20:37
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.

None yet

2 participants