-
Notifications
You must be signed in to change notification settings - Fork 146
Base adapter and postgres support function lifecycle #1332
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
base: qmalcolm--udfs-feature-branch
Are you sure you want to change the base?
Base adapter and postgres support function lifecycle #1332
Conversation
@@ -12,7 +12,7 @@ pre-install-commands = [ | |||
] | |||
dependencies = [ | |||
"dbt-common @ git+https://github.com/dbt-labs/dbt-common.git", | |||
"dbt-core @ git+https://github.com/dbt-labs/dbt-core.git#subdirectory=core", | |||
"dbt-core @ git+https://github.com/dbt-labs/dbt-core.git@qmalcolm--11965-core-handles-lifecycle-of-function-nodes#subdirectory=core", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically I'd say we should change this back before merging. However, we're merging to a feature branch, not main
. As such it is safe to leave as is.
@@ -67,7 +67,7 @@ pre-install-commands = [ | |||
] | |||
dependencies = [ | |||
"dbt-common @ git+https://github.com/dbt-labs/dbt-common.git", | |||
"dbt-core @ git+https://github.com/dbt-labs/dbt-core.git#subdirectory=core", | |||
"dbt-core @ git+https://github.com/dbt-labs/dbt-core.git@qmalcolm--11965-core-handles-lifecycle-of-function-nodes#subdirectory=core", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically I'd say we should change this back before merging. However, we're merging to a feature branch, not main. As such it is safe to leave as is.
@@ -11,7 +11,7 @@ pre-install-commands = [ | |||
] | |||
dependencies = [ | |||
"dbt_common @ git+https://github.com/dbt-labs/dbt-common.git", | |||
"dbt-core @ git+https://github.com/dbt-labs/dbt-core.git#subdirectory=core", | |||
"dbt-core @ git+https://github.com/dbt-labs/dbt-core.git@qmalcolm--11965-core-handles-lifecycle-of-function-nodes#subdirectory=core", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically I'd say we should change this back before merging. However, we're merging to a feature branch, not main. As such it is safe to leave as is.
@@ -0,0 +1,19 @@ | |||
{% materialization function, default %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expect most of this logic to get pushed down a level once we introduced a second function type (UDTFs or UDAFs)
@@ -0,0 +1,19 @@ | |||
{% materialization function, default %} | |||
{% if not adapter.supports(adapter.capability_for_string('UDFs')) %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth noting that we don't do this check for any other kind of materialization. Typically we just proceed optimistically and let the adapter raise an exception if it's not supported.
also, nit: should UDFs
just be functions
since that's the materialization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I only saw two ways forward:
- The base adapter implements a default macro per function type (UDF, UDAF, UDTF) per language (SQL, Python, Javascript, etc) that raises a not implemented exception
- The base adapter implements a base implementation per function type per language, and then the individual adapters can opt in by declaring the capability as supported.
The perk of (2) is that it means the 1st/3rd party adapter has less to implement. It can override simply one of the sub helper macros. For instance, if the arg specification is slightly different, then all they need to override is theget_formatted_udf_args
macro.
Do we want to go the route of (1) or is there another route I'm missing? 🙃
@@ -1915,10 +1915,16 @@ def render_model_constraint(cls, constraint: ModelLevelConstraint) -> Optional[s | |||
def capabilities(cls) -> CapabilityDict: | |||
return cls._capabilities | |||
|
|||
@available |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we don't need to do the supports check then we don't need this. Avoiding adding more to the implicit adapter interface is always nice
resolves #1268
related dbt-labs/dbt-core#12008
Problem
We need to support creating functions!
Solution
Create a function materialization
Checklist