Allow not specifying parameters names when using placeholder templater #5101
Conversation
shyaginuma
changed the title
| "Failure in placeholder templating: {}. Have you configured your " | ||
| "variables?".format(err) | ||
| ) | ||
| replacement = str(context.get(param_name, "")) |
There was a problem hiding this comment.
It might be better to set custom default value instead of blank string, but I'd like to hear your opinion.
There was a problem hiding this comment.
@shyaginuma - I think it would be safer to replace with the name of the variable as a string. That way it's transparent and most likely to still parse. This is similar to what we've done with the dbt ref() macro.
i.e. SELECT :foo, 1 would get parsed as SELECT "foo", 1 rather than SELECT , 1 (the latter would fail to parse, but the earlier one would pass).
How does that sound to you? I think most SQL dialects allow quoted literals in most cases that would accept strings, dates or other datatypes.
@barrywhart - do you have any views as you put up one of the original issues?
There was a problem hiding this comment.
IIRC, when I implemented the similar behavior for the Jinja and dbt templaters, I had it simply return the unquoted string. Either behavior may be reasonable, but I think we should be consistent. Can someone confirm the dbt templater's behavior?
There was a problem hiding this comment.
Our current jinja templater renders this:
select * from {{ ref("foo") }}as
select * from fooso that means it would be more consistent to render it unquoted. thanks @barrywhart !
@shyaginuma - could you update your code to that? I think then this is good to go.
There was a problem hiding this comment.
@alanmcruickshank Thanks for your suggestion! Fixed in 31bd5f1, could you take a look again?
|
n.b., to close two issues, next time put "fixes #4139, fixes #2108" ("fixes" twice) in the PR description https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword |
|
thanks for the suggestion. let me fix 🙇 |
|
now, I think it's ready for review. Could someone take a look? |
There was a problem hiding this comment.
Hi @shyaginuma - could you resolve the thread above about how to resolve missing variables? To be consistent with other places where we do something similar, I think we should resolve any missing variables as the name of the variable rather than an empty string, I think that gives us the best chance of the SQL still parsing afterward.
Otherwise I think this is good to go. Thanks for putting this together! 🏆
|
Thanks for the review!! 😄 |
Brief summary of the change made
fixes #4139, fixes #2108 (actually enhancement)
Are there any other side effects of this change that we should be aware of?
From my perspective, nothing. But this change was different from the intent of
placeholdertemplater, I'd like to discuss.Pull Request checklist
Please confirm you have completed any of the necessary steps below.
Included test cases to demonstrate any code changes, which may be one or more of the following:
.ymlrule test cases intest/fixtures/rules/std_rule_cases..sql/.ymlparser test cases intest/fixtures/dialects(note YML files can be auto generated withtox -e generate-fixture-yml).test/fixtures/linter/autofix.Added appropriate documentation for the change.
Created GitHub issues for any relevant followup/future enhancements if appropriate.