inject query comments (#1643) #1864

beckjake · 2019-10-24T21:47:39Z

In a user's dbt_project.yml, a new string field is accepted: query-comment. The string is used as the body of a macro that is evaluated with the following context:

dbt_version
env_var
modules
run_started_at
invocation_id
return
fromjson
tojson
log
var
target
all the macros that would be available in a model (even ones that will not work with the given context)!

It will be provided with two arguments:

connection_name, a string
node, which will either be null or a compiled/parsed node object.

The parsed macro is not available in any other context, and has no access to any macros.

Cleaned up contexts so things have a sensible inheritance pattern

it's probably not perfect but at least it's defined

query generator is an attribute on the connection manager

has a thread-local comment str
when acquiring a connection, set the comment str appropriately
new 'connection_for' context manager: like connection_named, except also use the node to set the query string
connection_named sets the query string using the connection name only.
has a special context

drewbanin

This is great! The core query comment feature works great, and I feel so good about the changes you made to how the context is built up. Super nice work!

I have a couple of comments here, and the big one is around moving this config from profiles.yml to dbt_project.yml. My instinct is that this isn't a huge change, but definitely let me know if it's more involved than I'm picturing.

drewbanin · 2019-10-28T21:30:54Z

core/dbt/adapters/base/query_headers.py

+from dbt.contracts.graph.compiled import CompileResultNode
+
+
+default_query_comment = '''


can we push this into the global project? Or is that a bad idea?

Also, let's make the default super simple. Can we just include:

app='dbt'

dbt_version

node_id=node.unique_id

target_name=target.name

These other fields are going to be really helpful in specific use-cases, but I don't think they need to be provided in the default implementation.

can we push this into the global project? Or is that a bad idea?

I think we can realistically do this, or we can move it to dbt_project.yml, but not really both.

drewbanin · 2019-10-28T21:33:30Z

core/dbt/adapters/base/query_headers.py

+        self.query_comment: str = initial
+
+    def add(self, sql: str) -> str:
+        # Make sure there are no trailing newlines.


I think we could equivalently place this text inside of a block comment (/* ... */) to remove this logic. I'm not super concerned that someone would provide a closing block comment in one of these comment attributes. I think a block comment makes more sense here because the resulting json block in the SQL query will still be machine-readable.

drewbanin · 2019-10-28T21:35:04Z

core/dbt/adapters/base/query_headers.py

+
+class QueryStringSetter:
+    def __init__(self, config: HasCredentials):
+        if config.config.query_comment is not None:


Can we grab the query comment string from a config called query_comment defined in the dbt_project.yml file instead of profiles.yml? My thinking here is twofold:

I imagine this is something users will want to version control for all dbt users (both human and machine).

If a user has multiple profiles defined in their dbt_project.yml file, then they'd be unable to use different query_comment configs for each profile.

drewbanin · 2019-10-28T21:37:26Z

core/dbt/config/profile.py

@@ -255,7 +255,10 @@ def from_raw_profile_info(cls, raw_profile, profile_name, cli_vars,
            target could not be found
        :returns Profile: The new Profile object.
        """
-        # user_cfg is not rendered since it only contains booleans.
+        # user_cfg is not rendered.


I don't totally follow what's going on here.

Two things:

user_cfg doesn't just contain booleans, so I fixed the comment

this method is called directly by some unit tests, so it needs to try to extract the config from the raw_profile to get valid results.

should this have changed when we moved the query comment from the profile into the project? If this is fine, then it does not offend me, just want to make sure

No, it was wrong before! user_cfg still contains non-booleans (the printer width, for example).

drewbanin · 2019-10-28T21:39:24Z

core/dbt/context/base.py

+        run_started_at = None
+        invocation_id = None
+
+        if dbt.tracking.active_user is not None:


I could have sworn we made a change a while back to include invocation_id and run_started_at even if users have opted out of anonymous event tracking. Is it a substantial change to make that the case here? I imagine invocation_id and run_started_at will both be super useful in the context of query comments

We did, and we do. active_user is only None early on during initialization.

drewbanin · 2019-10-28T21:42:56Z

core/dbt/context/base.py

+
+    def get_target(self) -> Dict[str, Any]:
+        target = dict(
+            self.config.credentials.connection_info(with_aliases=True)


super smart to use connection_info here - nice call

drewbanin · 2019-10-28T21:49:29Z

plugins/bigquery/dbt/adapters/bigquery/connections.py

@@ -45,7 +45,8 @@ def type(self):
        return 'bigquery'

    def _connection_keys(self):
-        return ('method', 'database', 'schema', 'location')
+        return ('method', 'database', 'schema', 'location', 'priority',


drewbanin · 2019-10-29T15:37:26Z

I definitely don’t want to do both - this config should only be provided in the dbt_project.yml file

On Tue, Oct 29, 2019 at 11:35 AM Jacob Beck ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In core/dbt/adapters/base/query_headers.py <#1864 (comment)> : > @@ -0,0 +1,101 @@ +from threading import local +from typing import Optional, Callable + +from dbt.clients.jinja import QueryStringGenerator + +from dbt.contracts.connection import HasCredentials +# this generates an import cycle, as usual +from dbt.context.base import QueryHeaderContext +from dbt.contracts.graph.compiled import CompileResultNode + + +default_query_comment = ''' can we push this into the global project? Or is that a bad idea? I think we can realistically do this, *or* we can move it to dbt_project.yml, but not really both. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1864>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALYIEZY7AH32QNYZQZVTNTQRBJ5HANCNFSM4JE36O5Q> .

-- Drew Banin Fishtown Analytics

Make a fake "macro" that we parse specially with a single global context Macro takes an argument (the node, may be none) Users supply the text of the macro in their 'user_config' under a new 'query_comment' No macros available query generator is an attribute on the connection manager - has a thread-local comment str - when acquiring a connection, set the comment str new 'connection_for' context manager: like connection_named, except also use the node to set the query string Updated unit tests to account for query comments Added a hacky, brittle integration test - log to a custom stream and read that Trim down the "target" context value to use the opt-in connection_info - Make sure it contains a superset of the documented stuff - Make sure it does not contain any blacklisted items Change some asserts to raise InternalExceptions because assert error messages in threads are useless

Add special handling to 'dbt debug' for this behavior Rework the dependencies/requirements for adapters since they now require more of a config object tests...

Fix the tests

drewbanin

ship it!

cla-bot bot added the cla:yes label Oct 24, 2019

beckjake force-pushed the feature/query-comments branch 4 times, most recently from 82bd794 to fdef2f3 Compare October 25, 2019 16:10

drewbanin reviewed Oct 28, 2019

View reviewed changes

beckjake force-pushed the feature/query-comments branch from fdef2f3 to ae58199 Compare October 29, 2019 18:17

beckjake requested a review from drewbanin October 29, 2019 18:47

beckjake force-pushed the feature/query-comments branch 2 times, most recently from 3c377b3 to e6daba3 Compare October 30, 2019 17:18

Jacob Beck added 7 commits November 4, 2019 09:01

Move query comments into the project config

5b6586d

Add special handling to 'dbt debug' for this behavior Rework the dependencies/requirements for adapters since they now require more of a config object tests...

less query commenting

15ff08d

use block-style comments per PR feedback

57f4221

Fix the tests

move macro stuff around

1873f40

macro support, tests, add yet another mypy env for development

84d585c

pin urllib3 to a version that snowflake supports

ab9fcb4

beckjake force-pushed the feature/query-comments branch from df68f85 to ab9fcb4 Compare November 4, 2019 16:01

if the comment macro is null/empty, no comments

b56d93b

drewbanin approved these changes Nov 4, 2019

View reviewed changes

beckjake merged commit c4cd4fc into dev/louisa-may-alcott Nov 4, 2019

beckjake deleted the feature/query-comments branch November 4, 2019 19:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

inject query comments (#1643) #1864

inject query comments (#1643) #1864

beckjake commented Oct 24, 2019 •

edited

Loading

drewbanin left a comment

drewbanin Oct 28, 2019

beckjake Oct 29, 2019 •

edited

Loading

drewbanin Oct 28, 2019

drewbanin Oct 28, 2019

drewbanin Oct 28, 2019

beckjake Oct 29, 2019

drewbanin Oct 31, 2019

beckjake Oct 31, 2019

drewbanin Oct 28, 2019

beckjake Oct 29, 2019

drewbanin Oct 28, 2019

drewbanin Oct 28, 2019

drewbanin commented Oct 29, 2019 via email

drewbanin left a comment

		from dbt.contracts.graph.compiled import CompileResultNode


		default_query_comment = '''

inject query comments (#1643) #1864

inject query comments (#1643) #1864

Conversation

beckjake commented Oct 24, 2019 • edited Loading

drewbanin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beckjake Oct 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewbanin commented Oct 29, 2019 via email

drewbanin left a comment

Choose a reason for hiding this comment

beckjake commented Oct 24, 2019 •

edited

Loading

beckjake Oct 29, 2019 •

edited

Loading