fix: guard Fusion's None target.threads and empty failures in all adapter overrides - #569
Open
mbiyashev3 wants to merge 2 commits into
Open
Conversation
dbt Fusion (dbt_version 2.0.0) leaves target.threads as None. brooklyn-data#549 added a guard, but only to default__get_invocations_dml_sql. Because adapter.dispatch prefers an adapter-specific override, every warehouse with its own macro still rendered the bare Python literal `None` into the invocations INSERT, which the warehouse rejects: BigQuery: Unrecognized name: None at [42:9] invocations is 8th in the upload order, so the failure also aborts the sources/tests/models uploads and orphans the already-inserted model_executions / test_executions rows on command_invocation_id. Guards bigquery__, postgres__, trino__ and sqlserver__. Null literal chosen per branch by surrounding construct: - bigquery/postgres/trino emit a bare tuple consumed as `insert into <rel> (<cols>) values (<tuple>)`, so the target column type is known and an untyped `null` is unambiguous. - sqlserver wraps its tuple in `select ... from (values (...)) v (...)`, where the derived column's type comes from the row rather than the insert target, so a typed null is required. Uses dbt.type_int() to match how target_threads is declared in models/sources/invocations.sql. Values-only change: no columns added, removed or reordered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ides brooklyn-data#549 replaced the `failures` expression in default__get_test_executions_dml_sql with a guard that handles both None and the empty string, but left the five adapter overrides on the older form: {{ 'null' if test.failures is none else test.failures }} That form handles None but not ''. When test.failures is an empty string the expression renders to nothing at all, so the VALUES tuple collapses to `..., null, , 'message', ...` — an empty positional value, which is a syntax error on every warehouse. Aligns bigquery__, postgres__, snowflake__, sqlserver__ and trino__ with the default__ form from brooklyn-data#549. Note snowflake__ was previously fixed for invocations only by falling through to default__; test_executions has its own override and so was still on the old form. A literal 0 continues to render as 0 -- the guard tests `is not none` and `!= ''` rather than truthiness, so a legitimate zero-failure count is not collapsed to null. Values-only change: no columns added, removed or reordered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Author
|
@mtcarlone — tagging you since you reviewed and merged #549; this PR finishes that fix for the adapter overrides Two things that would help when you get a chance:
The second commit ( |
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.
Overview
#549 guarded
default__only. Becauseadapter.dispatchprefers an adapter-specific override, every warehouse that ships its own macro still renders the bare Python literalNoneinto theinvocationsINSERT — so BigQuery, Postgres, Trino and SQL Server remain broken under dbt Fusion. This applies the same guard to those four overrides, and (second commit) aligns the fivetest_executionsoverrides with thefailuresguard #549 applied todefault__.Update type - breaking / non-breaking
What does this solve?
Under dbt Fusion (
dbt_version2.0.0, e.g. the dbt Cloud Fusion runtime)target.threadsisNone. Inon-run-end,dbt_artifacts.upload_results()renders thatNonestraight into theinvocationsINSERT as a bare Python literal, and the warehouse rejects the statement.Observed on BigQuery with dbt_artifacts 2.11.0 — which already contains #549:
The emitted SQL:
All 19 values in that tuple are valid warehouse literals except position 10.
Noneis the sole defect.Impact is worse than a failed statement.
invocationsis 8th in the upload order, so the failure kills the tail of the sequence —sources,testsandmodelsnever upload (0 rows) — and every already-insertedmodel_executions/test_executionsrow is left orphaned oncommand_invocation_id.stg_dbt__invocationsandfct_dbt__invocationscome out empty, so every invocation-joined mart is empty too. That is silent bad data, not just staleness.#549 was an incomplete fix
Merge commit c8c0b59 is a 3-line diff touching two files, and in both cases it patched only the
default__macro. The four adapter overrides were left with a bare{{ target.threads }}, and this is still unfixed onmainas of the 2.11.0 release merge (3d27645) — so no released version of the package fixes BigQuery:Snowflake is unaffected for
invocations(it falls through todefault__), but not fortest_executions, which does have its own override.Commit 1 —
target.threadsGuards
bigquery__,postgres__,trino__,sqlserver__. The null literal is chosen per branch based on the surrounding construct rather than copied verbatim fromdefault__:bigquery__,postgres__,trino__insert into <rel> (<cols>) values (<tuple>)nullsqlserver__select "1"…"19" from (values ( ... )) v (...)cast(null as {{ dbt.type_int() }})default__.For the typed case I used
dbt.type_int()rather than hardcoding, to match how the column is declared inmodels/sources/invocations.sql:19(cast(null as {{ type_int() }}) as target_threads) and how sibling macros in this package reference dbt's type helpers.dbt.type_int()is dispatched and resolves tointeverywhere except BigQuery, where it resolves toint64— so it stays correct if a future branch needs a typed null.Note: this package has no
type_inthelper of its own inmacros/database_specific_helpers/;type_intis dbt-core's global macro. I deliberately did not add a dispatched helper for this, to keep a values-only fix from growing intodatabase_specific_helpers/churn.Commit 2 —
failures(independent; drop it if you'd rather ship thetarget.threadsfix alone)#549 also replaced the
failuresexpression indefault__get_test_executions_dml_sqlwith a guard covering bothNoneand'', but left the five overrides on the older form:{{ 'null' if test.failures is none else test.failures }}That handles
Nonebut not''. Whentest.failuresis an empty string the expression renders to nothing at all, and the tuple collapses to an empty positional value — a syntax error on every warehouse:This aligns
bigquery__,postgres__,snowflake__,sqlserver__andtrino__with thedefault__form. A literal0still renders as0— the guard testsis not noneand!= ''rather than truthiness, so a legitimate zero-failure count is not collapsed to null.Commit 2 is independent of commit 1 and can be dropped without touching the
target.threadsfix.Not changed
Values only — no columns added, removed or reordered, so the column-order invariant across the upload macros,
models/sources/*andget_column_name_lists.sql(CONTRIBUTING.md) is untouched. No version bump beyond a patch is implied.Outstanding questions
./scripts/ci/setup.shfails before completing becausepyodbccannot build without the MS ODBC Driver 18. Please treat Tier 1 CI on this PR as the first real execution of these paths. What I did verify is described under "What databases have you tested with?" below.target.threadsis neverNoneunder dbt-core —profiles.ymlsets it and dbt supplies a default — so a green integration run exercises only the non-None branch and would pass identically with or without this patch. I could not find a way to forcetarget.threads = Nonethrough the existing harness without a Fusion runtime in CI. Reproducing this in CI likely needs a Fusion leg, which feels like its own piece of work; happy to take direction if you'd like it attempted here.macros/upload_individual_datasets/upload_model_executions.sqlrenders{{ model.execution_time }}bare with no guard at all (6 branches), and rendersconfig_full_refreshunquoted in thebigquery__andtrino__branches. These are plausibly the next thing Fusion breaks.config_full_refreshis the less exposed of the two — it already falls back toflags.FULL_REFRESHwhenNone, so it only breaks if that flag is itselfNone— whereasmodel.execution_timehas no guard whatsoever. I left both alone to keep this PR to the reported bug; happy to open a follow-up../scripts/ci/lint.shruns SQLFluff againstmodels/only, and this PR touchesmacros/exclusively — so these changes fall outside sqlfluff's scope entirely, rather than being unlinted for want of Snowflake credentials.What databases have you tested with?
No warehouse was executed against — I want to be explicit about that. No container runtime was available for the Postgres / Trino / SQL Server legs (see Outstanding questions), and fork PRs cannot reach the Snowflake or BigQuery secrets in any case.
What I verified instead, offline:
macros/upload_individual_datasets/upload_invocations.sqlandupload_test_executions.sqlthrough a Jinja2 environment withjinja2.ext.doenabled (matching dbt's) — both parse cleanly, confirming the nested{{ dbt.type_int() }}inside an{% if %}/{% else %}output branch is valid.target_threadssites:threads=8→8;threads=None→null(bigquery/postgres/trino) andcast(null as int)(default__, sqlserver). For all sixfailuressites:3→3,0→0,None→null,''→null.failuresbug reproduces. Running the same render against the pre-patch overrides withfailures=''produces an empty value, which is what commit 2 fixes.This is template-level verification, not warehouse verification — it proves the Jinja is correct and emits the literals I claim, and does not prove those literals are accepted by each engine. The per-branch reasoning for
nullvscast(null as int)is set out in the table above and is the part most worth a reviewer's eye. Postgres / Trino / SQL Server should be covered by Tier 1 CI on this PR — though the Tier 1 run is currently sitting ataction_required, so it needs a maintainer to approve workflows for this fork before it will execute. Snowflake, BigQuery and Databricks will need a maintainer post-merge.Related
I searched open and closed issues and PRs for
target.threads,Unrecognized name: Noneandfusion. There is no existing issue or PR for this bug and no duplicate of this change. The Fusion-tagged issues that do exist are unrelated: #523 (+column_storeconfig), #515 (+as_columnstorewarning), #555 (;indocs.mdbreaking dollar-quoted comments on Postgres), #552 (state-aware orchestration). #549 is the PR this one completes.