Append query comment #2199

ilkinulas · 2020-03-12T13:01:01Z

resolves #2138

Description

By default comment is inserted at the beginning of the query.

Snowflake removes leading comments from the query. https://docs.snowflake.net/manuals/release-notes/2017-04.html#queries-leading-comments-removed-during-execution

With this PR, dbt users will be able to insert the comment to the end query.

Comment location can be set in the dbt_project.yml file.

# comment will be appended 
query-comment:
    comment: 'executed by dbt'
    append: True

# default comment will be appended 
query-comment:
    append: True

Checklist

have signed the CLA
I have run this code in development and it appears to resolve the stated issue
This PR includes tests, or tests are not required/relevant for this PR
I have updated the CHANGELOG.md and added information about my change to the "dbt next" section.

cla-bot · 2020-03-12T13:01:04Z

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: @ilkinulas

cla-bot · 2020-03-12T13:26:19Z

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: @ilkinulas

cla-bot · 2020-03-12T13:35:05Z

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: @ilkinulas

cla-bot · 2020-03-12T13:46:26Z

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: @ilkinulas

cla-bot · 2020-03-12T14:00:04Z

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: @ilkinulas

cla-bot · 2020-03-12T14:20:09Z

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: @ilkinulas

beckjake · 2020-03-13T16:16:04Z

@cla-bot check

cla-bot · 2020-03-13T16:16:07Z

The cla-bot has been summoned, and re-checked this pull request!

ilkinulas · 2020-03-16T12:21:10Z

Hi ✋
Remaining 7 checks are stuck for 3 days. What should I need to do to get this PR reviewed?

beckjake · 2020-03-16T13:41:14Z

core/dbt/config/project.py

@@ -244,7 +250,7 @@ class Project:
    snapshots: Dict[str, Any]
    dbt_version: List[VersionSpecifier]
    packages: Dict[str, Any]
-    query_comment: Optional[Union[str, NoValue]]
+    query_comment: Dict[str, Any]


Instead of a Dict[str, Any], let's make this a new optional JsonSchemaMixin dataclass with two fields:

@dataclass class QueryComment(JsonsSchemaMixin): comment: Optional[str] = DEFAULT_QUERY_COMMENT append: bool = False

Then on the config/partial project objects, we'd change this to something like:

query_comment: Optional[QueryComment] = field(default_factory=QueryComment)

We can use the object itself being None to indicate the old None behavior (don't include any comment) and the comment field being None to indicate the old NoValue behavior (default comment).

Should we support the following config ( for backward compatibility) ?

query-comment: "executed by dbt"

The following definition will not allow string values for query-comment:

query_comment: Optional[QueryComment] = field(default_factory=QueryComment)

Hmm, you're right - we'll want to make it a Union[QueryComment, str] and do an isinstance check at runtime!

Do you have any idea why jsonschema validation fails for this config:

query-comment: comment: "executed by dbt" append: true

core/dbt/contracts/project.py

@dataclass class Project(HyphenatedJsonSchemaMixin, Replaceable): ... query_comment: Optional[Union[QueryComment, str]] = None

@dataclass class QueryComment(JsonSchemaMixin): comment: str = DEFAULT_QUERY_COMMENT append: bool = False

Here is the error message :

2020-03-17 21:04:42.892154 (MainThread): Running with dbt=0.17.0-a1 2020-03-17 21:04:43.124426 (MainThread): Encountered an error while reading the project: 2020-03-17 21:04:43.124548 (MainThread): ERROR: Runtime Error at path []: QueryComment(comment='executed by dbt', append=True) is not of type 'object' 2020-03-17 21:04:43.124647 (MainThread): Sending event: {'category': 'dbt', 'action': 'invocation', 'label': 'invalid', 'context': [<snowplow_tracker.self_describing_json.SelfDescribingJson object at 0x7fd85acdb370>, <snowplow_tracker.self_describing_json.SelfDescribingJson object at 0x7fd85acdb940>, <snowplow_tracker.self_describing_json.SelfDescribingJson object at 0x7fd85acdb2e0>]} 2020-03-17 21:04:43.124892 (MainThread): Encountered an error: 2020-03-17 21:04:43.124948 (MainThread): Runtime Error Could not run dbt 2020-03-17 21:04:43.126486 (MainThread): jsonschema.exceptions.ValidationError: QueryComment(comment='executed by dbt', append=True) is not of type 'object' Failed validating 'type' in schema[0]: {'additionalProperties': False, 'description': 'QueryComment(comment: str = "\\n{%- set comment_dict ' '= {} -%}\\n{%- do comment_dict.update(\\n ' "app='dbt',\\n dbt_version=dbt_version,\\n " "profile_name=target.get('profile_name'),\\n " "target_name=target.get('target_name'),\\n) -%}\\n{%- " 'if node is not none -%}\\n {%- do ' 'comment_dict.update(\\n ' 'node_id=node.unique_id,\\n ) -%}\\n{% else %}\\n {# ' 'in the node context, the connection name is the ' 'node_id #}\\n {%- do ' 'comment_dict.update(connection_name=connection_name) ' '-%}\\n{%- endif -%}\\n{{ return(tojson(comment_dict)) ' '}}\\n", append: bool = False)', 'properties': {'append': {'default': False, 'type': 'boolean'}, 'comment': {'default': '\n' '{%- set comment_dict = {} -%}\n' '{%- do comment_dict.update(\n' " app='dbt',\n" ' dbt_version=dbt_version,\n' ' ' "profile_name=target.get('profile_name'),\n" ' ' "target_name=target.get('target_name'),\n" ') -%}\n' '{%- if node is not none -%}\n' ' {%- do comment_dict.update(\n' ' node_id=node.unique_id,\n' ' ) -%}\n' '{% else %}\n' ' {# in the node context, the ' 'connection name is the node_id ' '#}\n' ' {%- do ' 'comment_dict.update(connection_name=connection_name) ' '-%}\n' '{%- endif -%}\n' '{{ ' 'return(tojson(comment_dict)) ' '}}\n', 'type': 'string'}}, 'required': [], 'type': 'object'} On instance: QueryComment(comment='executed by dbt', append=True)

I'd prefer asking before digging into details of jsonschema .

Thank you

I usually see that when I put the actual object in a place where we expected the dict form.

ilkinulas · 2020-03-19T11:19:46Z

Hi @beckjake, I have pushed your requested changes. Thanks a lot for your help.

core/dbt/adapters/base/query_headers.py

- isinstance check is needed to pass mypy checks

beckjake

Three last tiny things and I'll kick off tests, and then this should be good to merge! Thanks for your contribution @ilkinulas, this is great!

beckjake · 2020-03-19T16:54:40Z

CHANGELOG.md

@@ -3,6 +3,9 @@
 ### Fixes
 - When a jinja value is undefined, give a helpful error instead of failing with cryptic "cannot pickle ParserMacroCapture" errors ([#2110](https://github.com/fishtown-analytics/dbt/issues/2110), [#2184](https://github.com/fishtown-analytics/dbt/pull/2184))

+### Features
+- Support for appending query comments to SQL queries. ([#2199](https://github.com/fishtown-analytics/dbt/pull/2199))


Oops, sorry, one last thing! Can you add the issue to this changelog entry as well? [#2138](https://github.com/fishtown-analytics/dbt/issues/2138) should be it

beckjake · 2020-03-19T16:56:01Z

core/dbt/config/project.py

+        cfg_query_comment: Union[QueryComment, str]
+) -> QueryComment:
+    if isinstance(cfg_query_comment, str):
+        if cfg_query_comment in ('None', ''):


if not cfg_query_comment is probably sufficient here, as well, right?

if not cfg_query_comment check does not catch below config.

query-comment: None

because cfg_query_comment's type is str and its value is 'None'

I don't think we should catch that case! In yaml, the appropriate thing to do to get None out is either

query-comment:

or

query-comment: null

The docs says that

You can disable query comments by setting the query-comment config to None

What is the preferred way of disabling query comments?

The docs are wrong! I'll open a PR there to fix that, thank you for pointing that out.

The two methods I listed above should both work.

Now that I think about it more, this problem is why I had the NoValue construct, to differentiate "not set, so leave it at the default" and "don't supply a query comment".

So you are suggesting something like this:

query_comment: Optional[Union[NoValue, Union[QueryComment, str]]

Is my understanding correct?

Yeah, though unions get collapsed by typing, so that's equivalent to Optional[Union[NoValue, QueryComment, str]]

beckjake · 2020-03-19T16:58:19Z

CHANGELOG.md

@@ -3,6 +3,9 @@
 ### Fixes
 - When a jinja value is undefined, give a helpful error instead of failing with cryptic "cannot pickle ParserMacroCapture" errors ([#2110](https://github.com/fishtown-analytics/dbt/issues/2110), [#2184](https://github.com/fishtown-analytics/dbt/pull/2184))

+### Features
+- Support for appending query comments to SQL queries. ([#2199](https://github.com/fishtown-analytics/dbt/pull/2199))
+


Please add a contributors section, and add yourself!

Suggested change

Contributors:

- [@ilkinulas](https://github.com/ilkinulas) [#2199](https://github.com/fishtown-analytics/dbt/pull/2199)

- replaced pr link with issue link

- added ilkinulas to contributers list

- hologram version updated to 0.0.6 to be able to use QueryComment, NoValue and str within the same Union type.

- to disable query comments use : `'query-comment': ''`

beckjake

Thanks for your work on this marathon of a PR @ilkinulas! I think this is working as we want now, I've kicked off the rest of the integration tests and once those pass I think this is ready to merge for Octavius Catto - which should become 0.17.0.

@drewbanin do you have anything to add?

ilkinulas · 2020-03-23T14:40:26Z

Thank you @beckjake for your support in this PR.
Is it possible to include this in 0.16.0, so we can use it from the upstream ?

beckjake · 2020-03-23T14:47:22Z

I don't think so, unfortunately. We just released 0.16.0 an hour or so ago and we were well into the RC period/feature freeze last week.

Edit: If/when we do a 0.16.1 bugfix release though, I will try to backport this in.

drewbanin · 2020-03-24T00:35:56Z

Nice work on this @ilkinulas!!

@beckjake let's definitely get this merged as-is, and if you're easily able to backport it to 0.16.1 as well, i think some folks out there would really appreciate it :)

🚢

ilkinulas added 3 commits March 12, 2020 10:28

added query comment append feature (wip)

62b19b5

pass unit tests

6910847

added unit test for query_headers.py

65f1472

fixed mypy error

f18f6af

return default comment if config.query_comment is None

be80009

handle unset query comment

5fa59d8

fixed unittest: default value for query_comment is None

fd8629a

fix integration test.

1cf9220

cla-bot bot added the cla:yes label Mar 13, 2020

updated CHANGELOG

e3c53ee

beckjake suggested changes Mar 16, 2020

View reviewed changes

ilkinulas added 3 commits March 19, 2020 10:25

added QueryComment dataclass

ab19135

refactored types (pass mypy checks)

3b90bfc

pass unit test

36bf479

beckjake suggested changes Mar 19, 2020

View reviewed changes

core/dbt/adapters/base/query_headers.py Outdated Show resolved Hide resolved

core/dbt/adapters/base/query_headers.py Outdated Show resolved Hide resolved

core/dbt/adapters/base/query_headers.py Outdated Show resolved Hide resolved

refactored _get_comment_macro method

7c34ff1

- isinstance check is needed to pass mypy checks

beckjake reviewed Mar 19, 2020

View reviewed changes

ilkinulas added 3 commits March 19, 2020 23:47

updated change log.

75e1c1d

- replaced pr link with issue link

updated changelog

65d9c18

- added ilkinulas to contributers list

NoValue type definition updated.

ac336b2

- hologram version updated to 0.0.6 to be able to use QueryComment, NoValue and str within the same Union type.

ilkinulas added 2 commits March 21, 2020 17:31

Merge branch 'dev/octavius-catto' into query_comment_append

574d65a

pass integration test

0eed51d

- to disable query comments use : `'query-comment': ''`

beckjake approved these changes Mar 23, 2020

View reviewed changes

beckjake merged commit efcf78b into dbt-labs:dev/octavius-catto Mar 24, 2020

beckjake mentioned this pull request Mar 31, 2020

append query comment backport (#2138) #2282

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Append query comment #2199

Append query comment #2199

ilkinulas commented Mar 12, 2020 •

edited

Loading

cla-bot bot commented Mar 12, 2020

cla-bot bot commented Mar 12, 2020

cla-bot bot commented Mar 12, 2020

cla-bot bot commented Mar 12, 2020

cla-bot bot commented Mar 12, 2020

cla-bot bot commented Mar 12, 2020

beckjake commented Mar 13, 2020

cla-bot bot commented Mar 13, 2020

ilkinulas commented Mar 16, 2020

beckjake Mar 16, 2020

ilkinulas Mar 17, 2020

beckjake Mar 17, 2020

ilkinulas Mar 17, 2020

beckjake Mar 17, 2020

ilkinulas commented Mar 19, 2020

beckjake left a comment •

edited

Loading

beckjake Mar 19, 2020

beckjake Mar 19, 2020

ilkinulas Mar 19, 2020

beckjake Mar 19, 2020

ilkinulas Mar 19, 2020

beckjake Mar 19, 2020

beckjake Mar 19, 2020

ilkinulas Mar 19, 2020

beckjake Mar 19, 2020

beckjake Mar 19, 2020

beckjake left a comment

ilkinulas commented Mar 23, 2020

beckjake commented Mar 23, 2020 •

edited

Loading

drewbanin commented Mar 24, 2020



	Contributors:
	- [@ilkinulas](https://github.com/ilkinulas) [#2199](https://github.com/fishtown-analytics/dbt/pull/2199)

Append query comment #2199

Append query comment #2199

Conversation

ilkinulas commented Mar 12, 2020 • edited Loading

Description

Checklist

cla-bot bot commented Mar 12, 2020

cla-bot bot commented Mar 12, 2020

cla-bot bot commented Mar 12, 2020

cla-bot bot commented Mar 12, 2020

cla-bot bot commented Mar 12, 2020

cla-bot bot commented Mar 12, 2020

beckjake commented Mar 13, 2020

cla-bot bot commented Mar 13, 2020

ilkinulas commented Mar 16, 2020

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

ilkinulas commented Mar 19, 2020

beckjake left a comment • 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

beckjake left a comment

Choose a reason for hiding this comment

ilkinulas commented Mar 23, 2020

beckjake commented Mar 23, 2020 • edited Loading

drewbanin commented Mar 24, 2020

ilkinulas commented Mar 12, 2020 •

edited

Loading

beckjake left a comment •

edited

Loading

beckjake commented Mar 23, 2020 •

edited

Loading