fix: replace adapter.warn_once in duckdb__apply_grants for Fusion compatibility#758
Open
pgoslatara wants to merge 1 commit into
Open
fix: replace adapter.warn_once in duckdb__apply_grants for Fusion compatibility#758pgoslatara wants to merge 1 commit into
pgoslatara wants to merge 1 commit into
Conversation
Fusion's Minijinja-based Jinja runtime does not yet expose @available-decorated adapter methods on the 'adapter' proxy, so 'adapter.warn_once(...)' raises 'unknown method: map has no method named warn_once' and aborts the materialization for any model with a grants config. exceptions.warn is part of the standard dbt Jinja context and works under both the legacy Python Jinja runtime and Fusion's Minijinja runtime. The trade-off is losing the once-per-run dedup that warn_once provided -- the warning will now fire once per affected model -- but it preserves the original intent of duckdb#458 (tolerate grants on DuckDB instead of crashing) under both engines.
Collaborator
|
eh I'm more concerned that the fusion adapter doesn't support "at-available" since we use it all over the place in here /cc @dataders |
Contributor
|
@pgoslatara @jwills I just opened dbt-labs/dbt-core#15211 to discuss next steps with this! likely this solution is what we can adopt, but wanna get the ducks in a row first. I also opened an internal thread about it |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Under dbt-core 2.0.0 / Fusion,
dbt buildfails for any model that has agrantsconfig with:The call site is
duckdb__apply_grantsindbt/include/duckdb/macros/adapters.sql, which invokesadapter.warn_once(...).warn_onceis defined indbt/adapters/duckdb/impl.pyand is decorated with@available, so it works fine under the legacy Python Jinja runtime. Fusion's Minijinja-based runtime, however, does not expose@available-decorated adapter methods on theadapterproxy — it sees the proxy as a generic map and the call fails. The same model build succeeds under dbt-core 1.x.This PR keeps the original intent of #458 (tolerate
grantson DuckDB without crashing the parser) while making the macro work under both runtimes.exceptions.warnis part of the standard dbt Jinja context, so it is bound in Python Jinja and Minijinja alike.{% macro duckdb__apply_grants(relation, grant_config, should_revoke=True) %} {#-- If grant_config is {} or None, this is a no-op --#} {% if grant_config %} - {{ adapter.warn_once('Grants for relations are not supported by DuckDB') }} + {% do exceptions.warn('Grants for relations are not supported by DuckDB') %} {% endif %} {% endmacro %}Trade-off:
exceptions.warndoes not dedup, so a project with N models configured withgrantswill see N warnings per run instead of one. That seems acceptable as the cost of cross-runtime compatibility, but happy to take a different approach (for example deleting the override entirely and falling back to the defaultapply_grants, matchingdbt-clickhouse/dbt-exasol/dbt-salesforce) if maintainers prefer.Refs: #458, #559.