Skip to content

[codex] Improve Jinja macro guardrails - #405

Draft
dataders wants to merge 4 commits into
mainfrom
codex/jinja-code-improvements
Draft

[codex] Improve Jinja macro guardrails#405
dataders wants to merge 4 commits into
mainfrom
codex/jinja-code-improvements

Conversation

@dataders

Copy link
Copy Markdown
Collaborator

Summary

  • avoid mutating BigQuery external table options while rendering connection_name
  • guard Redshift partition logging for empty partition batches
  • read Fabric nullity tests from the current column instead of the column collection
  • remove unused Snowflake refresh variables
  • add lightweight macro contract tests for these cases

Validation

  • /opt/homebrew/bin/uv run python3 -m unittest tests/test_jinja_macro_contracts.py
  • SNOWFLAKE_ACCOUNT=dummy SNOWFLAKE_USER=dummy DBT_ENV_SECRET_SNOWFLAKE_PASS=dummy SNOWFLAKE_ROLE=dummy SNOWFLAKE_DATABASE=dummy SNOWFLAKE_WAREHOUSE=dummy SNOWFLAKE_SCHEMA=dummy /opt/homebrew/bin/uv run dbt parse --profiles-dir . --target snowflake

@dataders dataders left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

tell me more!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

what does this file do?!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

again, why?

Comment on lines +12 to +13
{%- set column_tests = column.tests if 'tests' in column else [] -%}
{%- set nullity = 'NOT NULL' if 'not_null' in column_tests else 'NULL'-%}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

i think i like this but say more

Comment on lines -4 to -8
{% set snowpipe = source_node.external.get('snowpipe', none) %}

{% set auto_refresh = external.get('auto_refresh', false) %}
{% set partitions = external.get('partitions', none) %}
{% set delta_format = (external.table_format | lower == "delta") %}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

these do exist in the snowpipe tables which also get refreshed with refresh_external_table btw

Comment on lines +32 to +37
def test_snowflake_refresh_has_no_unused_branch_variables(self):
macro = read_macro("macros/plugins/snowflake/refresh_external_table.sql")

self.assertNotIn("set snowpipe", macro)
self.assertNotIn("set partitions", macro)
self.assertNotIn("set delta_format", macro)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

is there even a pytest harness in this repo??

@dataders dataders left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Found one small cleanup before merge:

  • macros/plugins/redshift/helpers/add_partitions.sql:27 has trailing whitespace, so git diff --check origin/main..HEAD fails. Remove the space after the opening log string line.

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.

1 participant