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

Add materialized view configuration for BigQuery #4578

Merged
merged 12 commits into from
Dec 13, 2023

Conversation

mikealfare
Copy link
Contributor

@mikealfare mikealfare commented Dec 4, 2023

What are you changing in this pull request and why?

Add configuration options for materialized views in BigQuery. This corresponds to new functionality that was added to dbt-bigquery in v1.7.

Checklist

@mikealfare mikealfare self-assigned this Dec 4, 2023
@mikealfare mikealfare requested review from dataders and a team as code owners December 4, 2023 19:56
Copy link

vercel bot commented Dec 4, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs-getdbt-com ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 13, 2023 5:42pm

@github-actions github-actions bot added content Improvements or additions to content size: medium This change will take up to a week to address labels Dec 4, 2023
Copy link
Contributor

@dbeatty10 dbeatty10 left a comment

Choose a reason for hiding this comment

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

Things look good in the preview 👍

I didn't verify the code examples or the key technical details.

My main feedback is that:

  • it would be nice to make the table less wide
  • consider lowercase or sentence case for YES / NO, and potentially others

These are nits, and I'm checking off the "Review by DX" box.

p.s. One thing I'd recommend is changing Jinja booleans to lowercase (true / false). I've seen it be a problem in the past, but maybe it's okay now 🤷

Note:
The special constants true, false, and none are indeed lowercase. Because that caused confusion in the past, (True used to expand to an undefined variable that was considered false), all three can now also be written in title case (True, False, and None). However, for consistency, (all Jinja identifiers are lowercase) you should use the lowercase versions.

Copy link
Contributor

@dbeatty10 dbeatty10 left a comment

Choose a reason for hiding this comment

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

Approving with the assumption that this comment will be resolved somehow.

@matthewshaver matthewshaver merged commit 4d6bc32 into current Dec 13, 2023
7 checks passed
@matthewshaver matthewshaver deleted the content/materialized-views/dbt-bigquery branch December 13, 2023 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content Improvements or additions to content size: medium This change will take up to a week to address
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants