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 incremental materializations #226

Merged
merged 5 commits into from
Jul 28, 2022

Conversation

stu-k
Copy link
Contributor

@stu-k stu-k commented Jul 20, 2022

Description

Implements the incremental materialization for bigquery. Nearly directly copied from Ian's work in spark

@stu-k stu-k requested a review from jtcohen6 July 20, 2022 15:55
@cla-bot cla-bot bot added the cla:yes label Jul 20, 2022
@stu-k stu-k removed the request for review from jtcohen6 July 20, 2022 15:55
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.

@stu-k Thanks for pushing up the initial PR!

As discussed during our chat yesterday, there are (I believe) two cases in which we do not create a "temp" table before merging:

  • merge strategy with on_schema_change: ignore
  • insert_overwrite strategy with "static" partitions config

In those cases, dbt templates the model {{ sql }} directly into the merge statement. That won't work for Python, so we really do need to submit the Python job and save the results to a table before the merge takes place. We also need to make sure that table is cleaned up after the merge, either by altering it to set it to expire within X hours (our standard approach is 12), or drop it explicitly at the end of the materialization.

@stu-k
Copy link
Contributor Author

stu-k commented Jul 20, 2022

@jtcohen6 Apologies I didn't mean to add you as a reviewer, this is still in progress, I'm just using the pr to run the CI.

@jtcohen6
Copy link
Contributor

@stu-k Oh whoops, sorry! Keep on keepin' on!

@stu-k stu-k force-pushed the feature/python-model-incremental-mats branch from 4a070ff to b2763d9 Compare July 27, 2022 16:48
@stu-k stu-k force-pushed the feature/python-model-incremental-mats branch from b2763d9 to 1f4799c Compare July 28, 2022 18:04
@stu-k stu-k requested review from jtcohen6 and ChenyuLInx July 28, 2022 18:04
Copy link
Contributor

@ChenyuLInx ChenyuLInx left a comment

Choose a reason for hiding this comment

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

Looks great to me!

{%- call statement('main') -%}
{{ build_sql }}
{% endcall %}
Also, why does not either drop_relation or adapter.drop_relation work here?!
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove this comment?

@stu-k stu-k merged commit 257f994 into feature/python-model-v1 Jul 28, 2022
@stu-k stu-k deleted the feature/python-model-incremental-mats branch July 28, 2022 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants