Skip to content
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

dbt v1 compat #453

Merged
merged 15 commits into from
Dec 2, 2021
Merged

dbt v1 compat #453

merged 15 commits into from
Dec 2, 2021

Conversation

joellabes
Copy link
Contributor

This is a:

  • bug fix PR with no breaking changes — please ensure the base branch is master
  • new functionality — please ensure the base branch is the latest dev/ branch
  • a breaking change — please ensure the base branch is the latest dev/ branch

Description & motivation

🥳 ONE POINT OH IS COMING BABYYYYYYYYYY

🧐 New minor version which supports any 1.x.x version of Core.

I'm not pleased that I have to do require-dbt-version: [">=1.0.0-rc", "<2.0.0"]. My original attempt was require-dbt-version: "~=1.0.0" and that didn't work:

(dbt-preprod) joel@Joel-Labes internal-analytics % dbt deps
15:25:45 | [ info  ] | Running with dbt=1.0.0-rc3
15:25:47 | [ error ] | Encountered an error:
Runtime Error
  Could not parse version "~=1.0.0"

Nor did require-dbt-version: [">=1.0.0", "<2.0.0"] because it didn't pick up the release candidates.

Open to a different approach!

Checklist

  • I have verified that these changes work locally on the following warehouses (Note: it's okay if you do not have access to all warehouses, this helps us understand what has been covered)
    • BigQuery
    • Postgres
    • Redshift
    • Snowflake
  • I followed guidelines to ensure that my changes will work on "non-core" adapters by:
    • dispatching any new macro(s) so non-core adapters can also use them (e.g. the star() source)
    • using the limit_zero() macro in place of the literal string: limit 0
    • using dbt_utils.type_* macros instead of explicit datatypes (e.g. dbt_utils.type_timestamp() instead of TIMESTAMP
  • I have updated the README.md (if applicable)
  • I have added tests & descriptions to my models (and macros if applicable)
  • I have added an entry to CHANGELOG.md

joellabes and others added 12 commits November 11, 2021 15:57
* Update require-dbt-version to be 1.0

* Fix SQL 42000 on Exasol (#420)

" SQL-Error [42000]: syntax error, unexpected '*' "
If you specify the * in the unioned with their respectiv names <name>.* you do not receive the SQL Error posted above. This should not inflict any further problems since it is redundant for most DBs.

* Minor readme link fixes (#431)

* minor readme link fixes

* changelog addition

Co-authored-by: Joel Labes <joel.labes@dbtlabs.com>

* 0.7.4 changelog (#432)

* Update CHANGELOG.md

* Note branch name change

* use `limit_zero` macro instead of `limit 0` (#437)

* Utils 0.7.4b1  (#433)

* Update require-dbt-version to be 1.0

* Fix SQL 42000 on Exasol (#420)

" SQL-Error [42000]: syntax error, unexpected '*' "
If you specify the * in the unioned with their respectiv names <name>.* you do not receive the SQL Error posted above. This should not inflict any further problems since it is redundant for most DBs.

* Minor readme link fixes (#431)

* minor readme link fixes

* changelog addition

Co-authored-by: Joel Labes <joel.labes@dbtlabs.com>

* 0.7.4 changelog (#432)

* Update CHANGELOG.md

* Note branch name change

Co-authored-by: Timo Kruth <timo_kruth@gmx.de>
Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>

* standard convention

* Update integration_tests/tests/jinja_helpers/test_slugify.sql

Taking the liberty of committing on your behalf so that the CI job starts again

* Change limit_zero to be a macro

Co-authored-by: Joel Labes <joel.labes@dbtlabs.com>
Co-authored-by: Timo Kruth <timo_kruth@gmx.de>
Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>

* Add col_name alias to else state too (#437)

* Remove extra semicolon in `insert_by_period` materialization (#439)

* Remove extra semicolon in `insert_by_period` materialization.

`create_table_as()` generates a SQL statement that already ends with a semicolon, so the extra semicolon after a `create_table_as()` call in the `insert_by_period` materialization ends up being an empty SQL statement, and at least when using Snowflake this causes the dbt run to fail with a "cannot unpack non-iterable NoneType object" error.

* Update changelog for PR 439.

* Use the relation object passed into get_column_values, instead of making our own (#440)

* Use the relation object passed into get_column_values, instead of making our own

* Rename variables in get column value test to be clearer

* Update CHANGELOG.md

* Update CHANGELOG.md

Co-authored-by: Timo Kruth <timo_kruth@gmx.de>
Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>
Co-authored-by: Anders <swanson.anders@gmail.com>
Co-authored-by: Sean Rose <sean.s.rose@gmail.com>
…448)

* Create integration test for a dropped relation

* Update get_column_values.sql

* Swap out adapter call for a good old fashioned drop table
@joellabes joellabes requested a review from jtcohen6 December 2, 2021 02:48
@joellabes
Copy link
Contributor Author

joellabes commented Dec 2, 2021

@jtcohen6 beyond the code review, I'm not sure where the config is for the version of dbt used in integration tests - can you give me a pointer?

Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's already installing the latest compatible prerelease version of dbt-<adapter>:

pip install --pre "dbt-$1"

The problem is, our Circle containers are running on python 3.6, and dbt-core==1.0.0rc dropped support for python 3.6 (it's reaching end of life on Dec 23). So I think you'll just want to upgrade these container versions from 3.6.13 to something newer, e.g. cimg/python:3.9.9 (docs):

- image: circleci/python:3.6.13-stretch

Comment on lines 59 to 62
data_get_column_values_dropped:
# TODO: Ideally this would use the adapter, but it gives a "Tried to drop relation, but its type is null" error.
#+post-hook: "{% do adapter.drop_relation(this) %}"
+post-hook: "drop table {{ this }}"
Copy link
Contributor

@jtcohen6 jtcohen6 Dec 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you know that this is a table, you could try something like (pseudo-code):

    +post-hook: "{% do adapter.drop_relation(this.incorporate(type='table')) %}"

dbt_project.yml Outdated

require-dbt-version: [">=0.20.0", "<=1.0.0"]
require-dbt-version: [">=1.0.0-rc", "<2.0.0"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about switching this to [">=1.0.0", "<2.0.0"] once we have a final release of dbt-core v1.0.0? In which case, this feels fine for now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I don't really want to have to release 0.8.1 in a week that just drops the release candidates. Since

  • 0.7.5 supports <= 1.0 as its upper bound, which does include the RCs, and
  • 1.0 final is on the cusp of release anyway,

I think I'm happy to say that 0.8 is only going to cover the final released version of 1.0

Copy link
Contributor Author

@joellabes joellabes Dec 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With that said, that means that when I merge this in, Circle CI will give it a ❌ until 1.0 final actually ships. Do we feel OK about that? (cc @jasnonaz)

keeping in mind that there are like a dozen packages that will all need changed, so I really don't wanna do that twice

@joellabes joellabes requested a review from jtcohen6 December 2, 2021 18:45
Copy link
Contributor

@jasnonaz jasnonaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.0!!!

Approving this with failing tests as the tests are failing due to the fact that this version of utils will not be compatible with release candidates of dbt v1.0.0, only with the final version. We have a successful CI run here. The only change made subsequent to this run was bumping the bound to be >1.0.0, which, as expected, caused the tests to fail.

@joellabes joellabes merged commit baf4053 into next/minor Dec 2, 2021
@joellabes joellabes deleted the dbt-v1-compat branch December 2, 2021 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants