You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Prompted most recently by Ct-581 grants as configs #5230, which adds grants as a config under NodeConfig, where it gets inherited by node types that won't actually use it.
Problem
We need to better organize our tightly bundled NodeConfig. Many of these configs are only relevant to "executable" nodes (models, snapshots, seeds, and tests IFF store_failures is enabled). Some of them are only relevant to incremental models in particular. Analyses have no business with database/schema/alias.
Existing proposals
Incremental models. Many many configs are really just relevant to the incremental model materialization. As discussed in #5089: It feels a little icky that configs like unique_key and on_schema_change appear in the config for every node, when they're really just relevant to incremental models in particular. Could we isolate these to appear on incremental models only? This could be:
an IncrementalModelConfig subclass, which would supply unique_key, on_schema_change, incremental_strategy, and (new) incremental_predicates if and only if materialized == 'incremental'
a dictionary named incremental that contains strategy + predicates, and the old names can be passthroughs to that dictionary for backwards compatibility
Metadata interface. Every time we add a new attribute to NodeConfig, it changes the manifest.json artifact schema. As discussed in #4617: Given that users can supply whatever configs they want, to any node type that accepts config, should we even consider this a contracted metadata interface? I think the external contract for node.config should really be Dict[str, Any], and we should always be in the habit of "promoting" configs that are relevant and reliable to downstream metadata consumers as top-level node keys. Similar to what we did with node_info on logging events, which "promotes" config.materialized into node_info.materialized.
Adapter-specific configs (sort of discussed in #4622): I'm actually not sure if AdapterConfig works at all today. My sense is that their keys don't show up in the manifest unless set, their types aren't really validated until used (at runtime), and their default values don't get passed through. The code here is kinda jank, to put it nicely.
The text was updated successfully, but these errors were encountered:
@emmyoop@jtcohen6 could you work together to flesh this out some more? It can be a spike if we need some digging in and designing on what we want to do here
Context
grants
as a config underNodeConfig
, where it gets inherited by node types that won't actually use it.Problem
We need to better organize our tightly bundled
NodeConfig
. Many of these configs are only relevant to "executable" nodes (models, snapshots, seeds, and tests IFFstore_failures
is enabled). Some of them are only relevant to incremental models in particular. Analyses have no business withdatabase
/schema
/alias
.Existing proposals
Incremental models. Many many configs are really just relevant to the
incremental
model materialization. As discussed in #5089: It feels a little icky that configs likeunique_key
andon_schema_change
appear in the config for every node, when they're really just relevant to incremental models in particular. Could we isolate these to appear on incremental models only? This could be:IncrementalModelConfig
subclass, which would supplyunique_key
,on_schema_change
,incremental_strategy
, and (new)incremental_predicates
if and only ifmaterialized == 'incremental'
incremental
that containsstrategy
+predicates
, and the old names can be passthroughs to that dictionary for backwards compatibilityMetadata interface. Every time we add a new attribute to
NodeConfig
, it changes themanifest.json
artifact schema. As discussed in #4617: Given that users can supply whatever configs they want, to any node type that acceptsconfig
, should we even consider this a contracted metadata interface? I think the external contract fornode.config
should really beDict[str, Any]
, and we should always be in the habit of "promoting" configs that are relevant and reliable to downstream metadata consumers as top-level node keys. Similar to what we did withnode_info
on logging events, which "promotes"config.materialized
intonode_info.materialized
.Adapter-specific configs (sort of discussed in #4622): I'm actually not sure if
AdapterConfig
works at all today. My sense is that their keys don't show up in the manifest unless set, their types aren't really validated until used (at runtime), and their default values don't get passed through. The code here is kinda jank, to put it nicely.The text was updated successfully, but these errors were encountered: