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.

Sign up for GitHub

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

Jump to bottom

Remove extra semicolon in insert_by_period materialization #439

Merged
merged 2 commits into from
Nov 9, 2021
Merged

Remove extra semicolon in insert_by_period materialization #439

merged 2 commits into from
Nov 9, 2021

Conversation

sean-rose
Copy link
Contributor

@sean-rose sean-rose commented Nov 9, 2021 •
edited
Loading

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

This removes an extra semicolon in the insert_by_period materialization that causes the initial materialization (or rematerialized due to a full refresh) to fail immediately after creating the empty table when running in Snowflake with a "cannot unpack non-iterable NoneType object" error.

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 have "dispatched" any new macro(s) so non-core adapters can also use them (e.g. the star() source)
  • 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

@sean-rose
Remove extra semicolon in insert_by_period materialization.
c832516 
`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.
@sean-rose
Update changelog for PR 439.
e55d888
@joellabes
Copy link
Contributor

Hey @sean-rose, thanks for opening this! You're caught up in Jeremy's consistent-ification of Snowflake semicolons in dbt 0.21.0.

This is in a grey area. The current version of dbt-utils has [">=0.20.0", "<0.22.0"] as its dbt Core version boundary, and the semicolon change happened in the middle of that (0.21). Merging this change would be a breaking change for all Redshift users, and Snowflake users (if any) still on 0.20.x.

Because the insert_by_period materialisation only claims compatibility with Redshift, and we're reviewing how it should play with the wider ecosystem at the moment (see #410 (comment) and dbt-labs/dbt-core#4174), I'm going to close this issue without merging it I'm sorry.

In the meantime, you can apply these changes to your project by making your own copy of the materialisation, or consider using the SF-specific variant in #410

@joellabes joellabes closed this Nov 9, 2021
@sean-rose
Copy link
Contributor Author

sean-rose commented Nov 9, 2021 •
edited
Loading

Hey @sean-rose, thanks for opening this! You're caught up in Jeremy's consistent-ification of Snowflake semicolons in dbt 0.21.0.

This is in a grey area. The current version of dbt-utils has [">=0.20.0", "<0.22.0"] as its dbt Core version boundary, and the semicolon change happened in the middle of that (0.21). Merging this change would be a breaking change for all Redshift users, and Snowflake users (if any) still on 0.20.x.

@joellabes as far as I can tell that is not the case.

In dbt 0.20.0 the create_table_as macro generated SQL statements that already end with a semicolon for both Redshift and Snowflake, and that hasn't changed as of 0.21.latest for both Redshift and Snowflake.

So at least as of the dbt core versions dbt_utils currently supports, the extra semicolon being removed here shouldn't constitute a breaking change for any of those versions for Redshift or Snowflake.

There's even another place in the insert_by_period materialization where create_table_as is called without adding a trailing semicolon, and that's been working fine.

And for Snowflake the issue this is addressing didn't start with dbt 0.21.0, it was already an issue before that.

Because the insert_by_period materialisation only claims compatibility with Redshift, and we're reviewing how it should play with the wider ecosystem at the moment (see #410 (comment) and dbt-labs/dbt-core#4174), I'm going to close this issue without merging it I'm sorry.

I did notice #410, and if that ends up being merged by all means disregard this one. But it seemed like #410 might not make the cut for the next version, and if that's the case then I think having this fix would be very helpful for all Snowflake users trying to make use of insert_by_period. While there are certainly other issues with getting it working on Snowflake, there are workarounds for those. This issue is the one unavoidable hard failure I've run into, and the fix is straightforward.

So if #410 doesn't land for 0.7.4 would you consider reopening this for inclusion in 0.7.4?

@joellabes
Copy link
Contributor

So at least as of the dbt core versions dbt_utils currently supports, the extra semicolon being removed here shouldn't constitute a breaking change for any of those versions for Redshift or Snowflake.

OK! this was over-enthusiastic pattern-matching on my part then; sorry about that! Thanks for doing the detective work to set me straight.

In that case, I'm happy to include this in 0.7.4, which I suspect will be coming out any day now.

@joellabes joellabes reopened this Nov 9, 2021
@joellabes joellabes changed the base branch from main to next/patch November 9, 2021 22:38
@joellabes joellabes merged commit 4457e42 into dbt-labs:next/patch Nov 9, 2021
joellabes added a commit that referenced this pull request Nov 11, 2021
@joellabes @TimoKruth
@fivetran-joemarkiewicz @dataders @sean-rose
dbt 0.7.4 release (#441)
b42e6f9 
* 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>
joellabes added a commit that referenced this pull request Dec 2, 2021
@joellabes @TimoKruth
@fivetran-joemarkiewicz @dataders @sean-rose
dbt v1 compat (#453)
baf4053 
* dbt 0.7.4 release (#441)

* 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>

* Regression: Correctly handle missing relations in get_column_values (#448)

* Create integration test for a dropped relation

* Update get_column_values.sql

* Swap out adapter call for a good old fashioned drop table

* Add missing curlies

* what person wrote this code :/ (it was me)

* wrap values in quotes

* GOOD

* bigquery compat (they don't like except)

* Backport android url changes from #426 (#452)

* Update CHANGELOG.md

* Change require-dbt-version, update dbt_project.yml for integration tests proj

* Upgrade python version in CI, improve drop relation integration test

* Clarify version pinning

* Drop support for release candidates of 1.0.0

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joellabes joellabes joellabes approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sean-rose @joellabes