Refactor Jinja environment variable handling #353
Refactor Jinja environment variable handling #353MACKAT05 wants to merge 5 commits intoSnowflake-Labs:masterfrom
Conversation
|
I have yet to try to deploy the new example... i have run the render command and inspected the results... the main focus was to show the macro pattern that would inflate structured data instead of hard coded SQL and that the structured data was loading without throwing fits about not being able to find files. |
README.md
Outdated
|
|
||
| ##### from_csv, from_json, from_yaml | ||
|
|
||
| These functions provide access to local data files for use in Jinja templates. For detailed documentation and examples, see [LocalDataInjection.md](docs/LocalDataInjection.md). |
There was a problem hiding this comment.
The reference to LocalDataInjection.md is invalid. We can remove .md if the file is still valid.
There was a problem hiding this comment.
i missed this one. I used AI tooling to help create examples ( without posting sensitive customer/ client data) it unfortunately was very aggressive about littering the repository with .md files and not using the existing markdown file entries.
I think this should at this point be revised to:
For examples, see citibike_demo_jinja_data_injection.
How are you finding this feature branch otherwise?
There was a problem hiding this comment.
I have not completed my review. I generally understand what you are doing.
Curious - Was there a scenario you faced that lead to developing this solution? Does it relate to an open PR?
sahil-walia
left a comment
There was a problem hiding this comment.
Thank you so much for contributing.
README.md
Outdated
|
|
||
| ##### from_csv, from_json, from_yaml | ||
|
|
||
| These functions provide access to local data files for use in Jinja templates. For detailed documentation and examples, see [LocalDataInjection.md](docs/LocalDataInjection.md). |
There was a problem hiding this comment.
I have not completed my review. I generally understand what you are doing.
Curious - Was there a scenario you faced that lead to developing this solution? Does it relate to an open PR?
schemachange/JinjaEnvVar.py
Outdated
There was a problem hiding this comment.
Should we not rename this file from schemachange/JinjaEnvVar.py to JinjaTemplateDataProvider and code from your localdatainjection.py since we are extending the capability?
I think we should not name the file with Injection as it is generally perceived with negative connotation.
There was a problem hiding this comment.
I have plans to add additional modules that invoke snowflake connections and connections to other databases. to allow database introspection. I'm not opposed to renaming them. however with that in mind i also had thought it best to keep them as seperate modules to allow the default option to not load them to employ some level surface reduction from the security stand point hence local data injection and remote data injection.
| expected = "John is 30 years old\nJane is 25 years old\n" | ||
| assert result == expected | ||
| finally: | ||
| os.unlink(csv_path) |
There was a problem hiding this comment.
nit: add one blank line at the end
| with open(yaml_path, encoding=encoding) as yamlfile: | ||
| data = yaml.safe_load(yamlfile) | ||
|
|
||
| return data |
| return data | ||
|
|
||
| @staticmethod | ||
| def from_json( |
There was a problem hiding this comment.
nit: formatting inconsistencies .
| def from_json( | |
| def from_json( | |
| file_path: str, | |
| encoding: str = "utf-8" | |
| ) -> dict[str, Any] | list[Any]: |
| return data | ||
|
|
||
| @staticmethod | ||
| def from_yaml( |
There was a problem hiding this comment.
nit: formatting for consistency similar to from_csv
| def from_yaml( | |
| def from_yaml( | |
| file_path: str, | |
| encoding: str = "utf-8" | |
| ) -> dict[str, Any] | list[Any]: |
| csv_path = Path(file_path) | ||
| if not csv_path.exists(): | ||
| raise FileNotFoundError(f"CSV file not found: {file_path}") | ||
|
|
There was a problem hiding this comment.
validation for input delimiter. .
if not isinstance(delimiter, str) or len(delimiter) != 1:
raise ValueError("Delimiter must be a single character")
There was a problem hiding this comment.
One more to check file is actually a csv
if not csv_path.suffix.lower() == '.csv':
raise ValueError(f"File {file_path} is not a CSV file")
There was a problem hiding this comment.
what about tsv? i added some additional functionality to check if the file looks like the expected csv and to throw warnings instead of adding a requirement that a file be suffixed with .csv
|
Hi @MACKAT05, Thank you for this comprehensive enhancement to Jinja data injection! The We've just released 4.3.0, and this PR is targeted for 4.4.0. Action needed: This PR has merge conflicts with the master branch. Could you please rebase against Once rebased, we'll proceed with final review and merge. Looking forward to including this in 4.4.0! |
…Injection class. Added functions to load data from CSV, JSON, and YAML files directly into Jinja templates.
…ks added as guardrails
8085e72 to
89c741c
Compare
|
rebased and split commits. i added a couple of updates along the lines of the provided commentary. |
This pull request introduces a major refactor and enhancement to how local data files are injected into Jinja templates, replacing the legacy
JinjaEnvVarapproach with the newLocalDataInjectionclass. It adds new functions for loading CSV, JSON, and YAML files directly into templates, updates documentation, and provides comprehensive demos comparing the new and legacy methods. The changes are backward compatible and include deprecation notices for legacy features.Local Data Injection Enhancements
JinjaEnvVarclass into the newLocalDataInjectionclass, which now provides theenv_var()function and introducesfrom_csv(),from_json(), andfrom_yaml()functions for loading local data files in Jinja templates. This enables direct access to structured data from CSV, JSON, and YAML files within templates. (CHANGELOG.md[1]README.md[2] [3]LocalDataInjectionand its new functions, including detailed usage examples and a dedicated documentation file. (README.md[1] [2]docs/LocalDataInjection.md[3]Demo and Documentation
demo/citibike_demo_jinja_data_injection) showcasing both the newLocalDataInjectionapproach and the legacy method, including SQL scripts and configuration files that demonstrate loading and processing data from external files. (demo/citibike_demo_jinja_data_injection/1_setup/A__setup.sql[1]2_test/V1.1.0__initial_database_objects.sql[2]2_test/V1.1.0__initial_database_objects_legacy.sql[3] and related files)Deprecation and Backward Compatibility
JinjaEnvVarclass in favor ofLocalDataInjection, while maintaining backward compatibility for existing templates usingenv_var(). (CHANGELOG.mdCHANGELOG.mdR6-R22)Configuration Files for Demo
file_formats.json,stages.yaml) and their legacy Jinja equivalents to support the demo and illustrate the benefits of loading data from external sources. (file_formats.json[1]stages.yaml[2]file_formats_legacy.j2[3]These changes make it much easier and more maintainable to inject local structured data into Jinja templates, improving both developer experience and template flexibility.