-
Notifications
You must be signed in to change notification settings - Fork 56
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
Upgrade to support dbt core v141 #214
Conversation
Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide. |
0092ca3
to
1b3e09f
Compare
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.
New tests introduced in 1.4.0 are missing.
f2ca6c2
to
1789204
Compare
@hovaesco |
@@ -3,7 +3,7 @@ | |||
from typing import ClassVar, Dict | |||
|
|||
from dbt.adapters.base.column import Column | |||
from dbt.exceptions import RuntimeException | |||
from dbt.exceptions import DbtRuntimeError |
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.
My IDE says (while checking only this commit):
Cannot find reference to DbtRuntimeError
in exceptions.py
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.
that is a breaking change in dbt 1.4.0 dbt-labs/dbt-core#6460
@@ -13,7 +13,7 @@ class TrinoQuotePolicy(Policy): | |||
|
|||
@dataclass(frozen=True, eq=False, repr=False) | |||
class TrinoRelation(BaseRelation): | |||
quote_policy: TrinoQuotePolicy = TrinoQuotePolicy() | |||
quote_policy: TrinoQuotePolicy = field(default_factory=lambda: TrinoQuotePolicy()) |
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.
Is this change related to the upgrade?
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.
yes, because of this change
@@ -86,9 +86,9 @@ | |||
{% endif %} | |||
|
|||
{#-- Get the incremental_strategy, the macro to use for the strategy, and build the sql --#} | |||
{% set incremental_predicates = config.get('incremental_predicates', none) %} | |||
{% set incremental_predicates = config.get('predicates', none) or config.get('incremental_predicates', none) %} |
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.
Note that this change is rather characteristic for dbt-core 1.4.1 and not dbt-core 1.4.0
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.
Actually this PR is about to adjust to 1.4.1
| jwt_token | JWT token for authentication | Optional (required if `method` is `jwt`) | `none` or `abc123` | | ||
| client_certificate | Path to client certificate to be used for certificate based authentication | Optional (required if `method` is `certificate`) | `/tmp/tls.crt` | | ||
| client_private_key | Path to client private key to be used for certificate based authentication | Optional (required if `method` is `certificate`) | `/tmp/tls.key` | | ||
| http_headers | HTTP Headers to send alongside requests to Trino, specified as a yaml dictionary of (header, value) pairs. | Optional | `X-Trino-Client-Info: dbt-trino` | |
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.
Pls add a separate commit for the spacing changes in the README.md
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.
Extracted to separate commit
| threads | How many threads dbt should use | Optional (default is `1`) | `8` | | ||
| prepared_statements_enabled | Enable usage of Trino prepared statements (used in `dbt seed` commands) | Optional (default is `true`) | `true` or `false` | | ||
| retries | Configure how many times a database operation is retried when connection issues arise | Optional (default is `3`) | `10` | | ||
| Option | Description | Required? | Example | |
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.
btw, you could extract this change to a separate PR which is much easier to grasp and to merge
@@ -0,0 +1,7 @@ | |||
kind: Under the Hood | |||
body: Adjustment to dbt-core 1.4.0 |
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.
Why not 1.4.1 ?
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.
Actually this PR is about to adjust to 1.4.1, I changed this here
b610008
to
a93be8a
Compare
"models": { | ||
"+incremental_predicates": ["dbt_internal_dest.id != 2"], | ||
"+incremental_strategy": "merge", | ||
"+on_table_exists": "drop", |
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.
on_table_exists
probably not needed here.
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.
removed
…0. Add tests for incremental_predicates.
a93be8a
to
8290c5a
Compare
@@ -86,7 +86,7 @@ def _get_dbt_core_version(): | |||
}, | |||
install_requires=[ | |||
"dbt-core~={}".format(dbt_version), | |||
"trino==0.319.0", | |||
"trino==0.321.0", |
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.
Add to changie.
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.
added
@@ -429,6 +435,7 @@ def open(cls, connection): | |||
isolation_level=IsolationLevel.AUTOCOMMIT, | |||
source="dbt-trino", | |||
verify=credentials.cert, | |||
timezone=credentials.timezone, |
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.
Add to changie.
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.
added
6147594
to
e6517fb
Compare
e6517fb
to
5ee545d
Compare
@@ -0,0 +1,7 @@ | |||
kind: Features | |||
body: Added new connection property `timezone` |
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.
@damian3031 could you please document timezone
in README.md
?
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.
Related PR trinodb/trino-python-client#252
Closes #213