-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Fix bug #2731 on stripping query comments for snowflake #2974
Conversation
Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA. In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, don't hesitate to ping @drewbanin. CLA has not been signed by users: @rvacaru |
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.
Thanks for the fix @rvacaru, and for the thorough unit testing!
Do you have a strong feeling about adding those in a new file (test_connections.py
), versus adding it to the existing tests in test_snowflake_adapter.py
? I ask because, someday, we'll want to split up these tests by adapter plugin, and it would be great to keep them closer together.
Finally, could you pull dev/margaret-mead
and move your Changelog entry up to the v0.20.0
section (beneath a new ### Fixes
subsection)?
Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA. In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, don't hesitate to ping @drewbanin. CLA has not been signed by users: @rvacaru-atb |
You're welcome @jtcohen6 ! I moved the unit tests and updated the changelog like suggested, it should be ok now. |
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.
Thanks @rvacaru! This is looking good.
I recommended one more unit test, for good measure, to be extra sure we're resolving the initially stated issue.
In testing this out I realized that there's a slight inconsistency arounnd query splitting and query headers, in that the latter only prepend the first split query.
If I have models/a.sql like:
{%- call statement('test') -%}
select 1; -- comment
select 2; -- comment
select 3; -- comment
{%- endcall %}
select 1 as id
In the logs I see:
2021-01-20 08:09:01.951760 (Thread-1): Using snowflake connection "model.testy.a".
2021-01-20 08:09:01.951967 (Thread-1): On model.testy.a: /* {"app": "dbt", "dbt_version": "0.19.0rc2", "profile_name": "jaffle_shop", "target_name": "dev", "node_id": "model.testy.a"} */
select 1; -- comment
2021-01-20 08:09:02.190950 (Thread-1): SQL status: SUCCESS 1 in 0.24 seconds
2021-01-20 08:09:02.191158 (Thread-1): select 2;
2021-01-20 08:09:02.191315 (Thread-1): Using snowflake connection "model.testy.a".
2021-01-20 08:09:02.191677 (Thread-1): On model.testy.a: select 2; -- comment
2021-01-20 08:09:02.554744 (Thread-1): SQL status: SUCCESS 1 in 0.36 seconds
2021-01-20 08:09:02.554959 (Thread-1): select 3;
2021-01-20 08:09:02.555108 (Thread-1): Using snowflake connection "model.testy.a".
2021-01-20 08:09:02.555339 (Thread-1): On model.testy.a: select 3; -- comment
2021-01-20 08:09:02.890287 (Thread-1): SQL status: SUCCESS 1 in 0.33 seconds
...
That's not in scope for this PR, I just wanted to get it down in writing in case someone cares to open an issue later on.
query1 = 'select 1; -- comment' | ||
query2 = 'select 1; /* comment */' | ||
query3 = 'select 1; -- comment\nselect 2; /* comment */ ' | ||
query4 = 'select \n1; -- comment\nselect \n2; /* comment */ ' |
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.
Could you add one more unit test that represents the original error case reported in #2731:
query5 = 'select 1; -- comment \nselect 2; -- comment \nselect 3; -- comment'
expected_query_5 = 'select 1; \nselect 2; \nselect 3;'
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.
I've added the extra test. Let me know if there's anything more
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.
@rvacaru Thanks for the contribution!
resolves #2731
Description
Improved regex which removes EOL comments from snowflake multiline queries.
Checklist
CHANGELOG.md
and added information about my change to the "dbt next" section.