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

Allow multiple materialized views to write to same target (#280) #364

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

the4thamigo-uk
Copy link
Contributor

@the4thamigo-uk the4thamigo-uk commented Oct 8, 2024

Summary

Proposed mechanism to support having multiple materialized views pointing to the same target table.

In brief, we add tags to mark blocks of the sql which will form each of the materialized views to be created.

i.e. https://github.com/ClickHouse/dbt-clickhouse/pull/364/files#diff-b1cddf2e5c9b3d08ea2276e5cef4944b06b9744a4076c5970ff5a5edb0140f08R62.

The entire union of all such blocks is used to backfill the target table.

Checklist

Delete items not relevant to your PR:

  • Unit and integration tests covering the common scenarios were added
  • A human-readable description of the changes was provided to include in CHANGELOG
  • For significant changes, documentation in https://github.com/ClickHouse/clickhouse-docs was updated with further explanations or tutorials

@the4thamigo-uk
Copy link
Contributor Author

issue #280

@the4thamigo-uk the4thamigo-uk force-pushed the multiple-materialized-views_to_same_target branch from e4e9b6d to f932efb Compare October 8, 2024 13:14
@BentsiLeviav
Copy link
Contributor

Hi @the4thamigo-uk

Thank you for your contribution!
Before reviewing your PR, it is required to add a short description with a PR link to the changelog (please keep the current format we have in the Changelog file).
With this kind of feature, I think it is also important to add a note in the documentation (readme file).

looking forward to reviewing this

@the4thamigo-uk the4thamigo-uk force-pushed the multiple-materialized-views_to_same_target branch from f932efb to 798aca9 Compare November 13, 2024 09:25
the4thamigo-uk added a commit to the4thamigo-uk/dbt-clickhouse that referenced this pull request Nov 13, 2024
the4thamigo-uk added a commit to the4thamigo-uk/dbt-clickhouse that referenced this pull request Nov 13, 2024
@the4thamigo-uk the4thamigo-uk force-pushed the multiple-materialized-views_to_same_target branch from 3485793 to e661bfa Compare November 13, 2024 09:45
the4thamigo-uk added a commit to the4thamigo-uk/dbt-clickhouse that referenced this pull request Nov 13, 2024
@the4thamigo-uk the4thamigo-uk force-pushed the multiple-materialized-views_to_same_target branch from e661bfa to 277fa45 Compare November 13, 2024 09:46
@the4thamigo-uk the4thamigo-uk force-pushed the multiple-materialized-views_to_same_target branch from 277fa45 to 064ed53 Compare November 13, 2024 09:47
@the4thamigo-uk
Copy link
Contributor Author

@BentsiLeviav Ok, Ive done this.

"hackers.sql": MV_MODEL,
}

def test_create(self, project):
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you also test for updates and full refresh?

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 think if you change the model to add or rename MVs, then old ones will be left running, which is incorrect.

@BentsiLeviav
Copy link
Contributor

Thanks @the4thamigo-uk
We appreciate this contribution!
The code LGTM. I left a small comment regarding the tests.

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.

2 participants