Add force on cluster config option #328
Conversation
|
|
There was a problem hiding this comment.
Great! Imo this is a good solution that makes this macro usable in many different contexts, including in pre/post-hooks.
Big question: What should happen if force is set but there is no cluster in config? Error or should can_on_cluster be set to None as it would be currently.
Nit: calling the parameter force instead of force_on_cluster is just as clear and a bit shorter.
Nice cleanup up the logic in the if X then X else '' statements. I cannot see any side effects.
Not sure about the extent of required tests. Great to have them. Wonder if you need to test the behavior of the various if scenarios within get_on_cluster with and without the force argument, to catch any future breakages. (Setting force=False shouldn't affect can_on_cluster if it can on cluster according to the base logic in get_on_cluster)
Note I am just a community member.
| table and incremental materializations with non-replicated engine will not be affected by `cluster` setting (model would be created on the connected node only). | ||
| By default, tables and incremental materializations with non-replicated engines will not be affected by the `cluster` setting (model would be created on the connected node only). | ||
|
|
||
| To force relations to be created on a cluster regardless of their engine or materialization, use the `force_on_cluster` argument: |
There was a problem hiding this comment.
nit:
| To force relations to be created on a cluster regardless of their engine or materialization, use the `force_on_cluster` argument: | |
| To force relations to be created on a cluster regardless of their engine or materialization, use the `force` argument: |
The macro is called on_cluster, so a standard force argument makes sense IMO. no need to call it force_on_cluster. Follows typical pattern of adding a force or -f flag.
tests/integration/adapter/clickhouse/test_clickhouse_table_materializations.py
Show resolved
Hide resolved
| @@ -118,14 +118,13 @@ def create_from( | |||
| if schema == relation_config.source_name and relation_config.database: | |||
| schema = relation_config.database | |||
|
|
|||
| if relation_config.config.get("force_on_cluster") in ("True", "true"): | |||
| can_on_cluster = True | |||
There was a problem hiding this comment.
What if cluster is not set in config but force is enabled? What should happen? Should we error or set can_on_cluster to False?
There was a problem hiding this comment.
IMO it's unnecessary to error out if the cluster isn't set, we can just keep the original behavior by letting the can_on_cluster variable default to None, which should have no impact downstream.
I've added an additional gate to prevent strange behaviour where there is no cluster var, but force_on_cluster is somehow enabled
benjamin-awd
force-pushed
the
add-force-on-cluster
branch
from
d280fb0
to
7bef2d1
Compare
|
@emirkmo thanks for the review 😄
Addressed here #328 (comment)
If the force flag isn't set, the else condition should trigger and the default behavior should occur -- IMO it's not necessary to have a test for this
In this case, my preference is to be more explicit with the naming. As a user that knows nothing about the adapter, if I see {{ config(
engine='Null',
materialized='materialized_view',
force='true'in the config, it will not be apparent that this in fact refers to the ON CLUSTER clause. Otherwise, if |
Summary
Closes #327
This PR allows users to create relations on a cluster, regardless of their engine or materialization type.
Checklist
Delete items not relevant to your PR: