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

Feature/incremental predicates #161

Merged
merged 25 commits into from Dec 15, 2022

Conversation

dave-connors-3
Copy link
Contributor

resolves #160

Description

Updates the incremental materialization to include user supplied incremental_predicates and passes that to the dbt_spark_get_incremental_sql macro.

The changes to dbt_spark_get_incremental_sql are updated in dbt-spark PR #436

Checklist

  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md and added information about my change to the "dbt-databricks next" section.

Copy link
Collaborator

@ueshin ueshin left a comment

Choose a reason for hiding this comment

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

Otherwise, LGTM.
I'll test with this change once the upstreams are merged.

@zhengruifeng zhengruifeng removed their request for review November 25, 2022 09:21
@dave-connors-3 dave-connors-3 requested review from ueshin and removed request for allisonwang-db November 28, 2022 19:36
@dave-connors-3
Copy link
Contributor Author

@ueshin thank you for the review! I believe i've addressed the outstanding issues!

@ueshin
Copy link
Collaborator

ueshin commented Dec 15, 2022

Hi @dave-connors-3, now that the PRs for dbt-core and dbt-spark were merged.
Could you revert dev-requirements.txt and fix the linter errors?
Also, could you add an entry in CHANGELOG.md?

from dbt.tests.adapter.incremental.test_incremental_predicates import BaseIncrementalPredicates


models__spark_incremental_predicates_sql = """
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to re-define this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i believe I needed to redefine this in my dbt-spark PR, and I assumed I needed to do the same here.

i did just run the SQL in databricks without the cast function and it seemed to execute. I can remove this if we don't think this is necessary!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Shall we just rename it to models__databricks_incremental_predicates_sql, then?

@ueshin
Copy link
Collaborator

ueshin commented Dec 15, 2022

Why was tests/integration/incremental_strategies/seeds/expected_exclude_upsert.csv removed?
Could you add it back?

@dave-connors-3
Copy link
Contributor Author

Why was tests/integration/incremental_strategies/seeds/expected_exclude_upsert.csv removed? Could you add it back?

Totally by mistake! restored the file

Copy link
Collaborator

@ueshin ueshin left a comment

Choose a reason for hiding this comment

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

LGTM, pending linter.
@dave-connors-3 Could you run black again?

@dave-connors-3
Copy link
Contributor Author

@ueshin i think black is passing, but flake8 line length checks are failing

@ueshin
Copy link
Collaborator

ueshin commented Dec 15, 2022

@dave-connors-3 Yeah, sometimes black and flake8 conflict..

@ueshin
Copy link
Collaborator

ueshin commented Dec 15, 2022

Thanks! merging.

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.

support user incremental predicates on merge
2 participants