-
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
Use sql
instead of compiled_code
within the default get_limit_sql
macro
#372
Conversation
sql
instead of compiled_code
within the default `get_limit_sq…sql
instead of compiled_code
within the default get_limit_sql
macro
Interesting code change 🤔 Before we merge something like this, can we get some downstream CI builds from concrete adapters that show this is correct? Adapters itself doesn't have the functional tests to give us confidence |
@VersusFacit Assuming something like this will do the trick? |
Assuming so, opened this draft PR to run tests against this branch: dbt-labs/dbt-postgres#178 Does something similar need to be done on adapters other than dbt-postgres? Or is this sufficient? |
@VersusFacit all checks on dbt-postgres passed on dbt-labs/dbt-postgres#178 -- see below for screenshot. Does something similar need to be done on adapters other than dbt-postgres? Or is this sufficient? |
@dbeatty10 are you able to test this out on other adapters too? That would give me more confidence |
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.
all CI passed. Thanks for your patience @dbeatty10 . I stayed on late to squeeze this in for the last pre-holiday release after we left you hanging ❤️
resolves #370
Problem
#249 (comment)
Solution
Just swap out
sql
forcompiled_code
.Checklist