backport(1.11): two silent-correctness fixes for v1.11.1 - #813
Merged
Conversation
…efinition sqlserver__view's skip test compared the stored view definition against the model's compiled SQL with normalized_definition.endswith(normalized_sql). The stored definition is the whole statement (CREATE [OR ALTER] VIEW <name> AS <body>) while the model is only the body, so any edit whose new body is a tail of the old one - most commonly deleting a leading comment or CTE - satisfied endswith() and was silently skipped. dbt run reported PASS, the change never reached the database, and --full-refresh did not fix it; once in that state every subsequent run re-confirmed the skip. Split the header off at its separating ' AS ' (the first one - the quoted relation contains no other) and compare the remainder verbatim. Also drop the | lower and whitespace stripping: both made genuinely different bodies compare equal (where source = 'MAXIMS' vs 'maxims', or any literal containing spaces), turning a missed rebuild into a correctness bug. The comparison follows the asymmetry that a skip failing to fire costs one rebuild while a skip firing wrongly costs correctness - an unparseable definition (no ' AS ') rebuilds rather than guessing. Also removes the dead normalized_relation local, which was computed but never read. Add regression tests: removing a leading comment now lands in the stored definition, and a change confined to the case of a string literal rebuilds. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> (cherry picked from commit 54e7623)
…he build get_column_schema_from_query wants a query's column shape and gets it by executing the query, reading cursor.description, then returning without fetching the rows or closing the cursor. Closing a cursor whose result set the server is still producing makes the driver cancel the request, and the cancel arrives as an attention. Every connection runs SET XACT_ABORT ON (#718), under which SQL Server answers an attention by rolling back the open transaction. None of that raises, so the damage surfaces later and elsewhere: a snapshot loses the staging table it built moments earlier in the same transaction and fails with `Invalid object name '..._dbt_tmp'`, and a contract-enforced model silently loses its in-transaction pre_hook writes. Only queries opening with a CTE were exposed. Anything else is wrapped as `select * from (...) where 1 = 0` by sqlserver__get_empty_subquery_sql and returns nothing, which is why snapshot staging queries (`with snapshot_query as ...`, both check and timestamp strategies) and CTE-headed contract models were the ones that broke, while everything else stayed quiet. The size of the result set is not the trigger; the timing is. The cancel only raises an attention while the request is still in flight, so a few hundred rows at zero client delay is enough, while ~10ms of client work before the close makes even 20MB safe. Existing coverage exercised these paths with tens of narrow rows and so never tripped it -- the snapshot fixtures in test_transactions.py are now sized past the boundary, and the previously untested contract + CTE combination is covered too. _discard_pending_results lives next to _try_drain_nextset rather than introducing a second draining idiom, and is applied to _get_row_count as well, which leaked its cursor on every expand_column_types call. Note: _try_drain_nextset and _discard_pending_results are backported without the ADBC-specific NotSupportedError handling in bc92c53, since v1.11 has no ADBC backend (added later in #783). Reapply that branch if ADBC is ever backported here too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit bc92c53)
axellpadilla
changed the title
axellpadilla
force-pushed
the
backport/1.11.1-critical-fixes
branch
from
August 6, 2026 22:16
022fe34 to
36b9fc1
Compare
sqlserver__get_empty_subquery_sql neuters a probe query as `select * from (...) where 1 = 0`, but a query opening with a CTE cannot be wrapped that way and is passed through untouched, so it ran in full purely to have its column names read. A snapshot executed its whole staging query once for the probe and again to build the staging table; a contract-enforced model ran twice per build. Draining that result set (previous commit) makes it safe but not cheap, and the cost tracks data volume. sp_describe_first_result_set compiles the query and reports its shape without scanning, which is all this method ever wanted. It is already how sqlserver__get_columns_in_query handles CTEs (#698). The macro cannot be the fix point: its output is also embedded in a CREATE VIEW body by dbt-core's unit-test materialization, where only a bare SELECT is legal, and reading cursor.description of an `exec sp_describe...` batch would describe the procedure's own result shape rather than the query's. Reported names and types are unchanged. Reading cursor.description reports Python classes, which collapse whole type families -- every integer width arrives as int, every string type as varchar -- so the describe path is mapped back onto exactly those names and fed through Column.create as before, leaving the dbt_sqlserver_use_native_string_types flag working without the mapping knowing it exists. Rather than guess, the describe path declines and lets the caller execute whenever it cannot guarantee agreement: a type outside the mapping, a query sp_describe_first_result_set refuses to describe such as one reading a #temp table, or a describe that returns nothing. Every gap degrades to slower, never to silently different. The name a type maps back to is also backend-specific, not just SQL Server's: mssql-python decodes uniqueidentifier as uuid.UUID, datetimeoffset as datetime and sql_variant as str, where pyodbc hands back str, bytearray and bytearray for the same three. A fixed table can't serve both, so _executed_name_for_system_type follows the backend in use (folded in from master's 3354428, minus its ADBC Arrow-map portion -- no ADBC backend on this branch). datetimeoffset on pyodbc depends on whether add_query has registered the -155 output converter yet, so it's left unmapped there and executes, agreeing with itself by construction; uniqueidentifier on pyodbc depends on the process-global pyodbc.native_uuid flag, read at probe time. TestCteProbeAvoidsExecution pins both halves: that a CTE-headed probe no longer executes its query, and that the two branches agree across every probed type on the backend under test. Note: dropped the ADBC-backend guard clause from master's version (checking is_adbc_backend before trusting the describe path), since v1.11 has no ADBC backend (added later in #783) to disagree with it. Reapply that guard if ADBC is ever backported here too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 7cee257, with the backend-aware type resolution folded in from 3354428)
…v1.11.1 Both are backported bugfixes with no code changes on this branch beyond the two cherry-picked commits, so they get one section rather than riding under "Unreleased"/"v1.12.0" as on master. Kept as an isolated commit (touching only this new section, no reflow of existing entries) so it cherry-picks cleanly forward onto master's CHANGELOG once v1.12.0 also needs to record that these two shipped early in v1.11.1.
axellpadilla
force-pushed
the
backport/1.11.1-critical-fixes
branch
from
August 6, 2026 23:07
36b9fc1 to
c3c014a
Compare
axellpadilla
changed the title
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
1 participant
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.
Backports two bugfixes from master to
release/v1.11for av1.11.1patch release.What's included
Invalid object name '..._dbt_tmp'on their second run; contract-enforced models silently losepre_hookwrites #809 on this branch), plus the companion perf fix (7cee257, describes CTE-headed probes instead of executing them) — silent-correctness bug: snapshots/contract models could silently lose data mid-transaction. Folded into this same commit: the backend-aware column-type resolution from 3354428 (the non-ADBC portion — see "What's excluded" below), needed because the CTE-describe path must report the same type the backend would have executed with (e.g.mssql-pythondecodesuniqueidentifierasuuid.UUID, pyodbc asstr/varchardepending onnative_uuid), otherwise the probe and the real query disagree.v1.11.1sectionWhat's excluded and why
fix: resolve probe column types per backend and complete the Arrow maps(3354428, also part of PR Fix/undrained probe cursor rollback #810) are not included —adbcdoes not exist on this branch (added later by Add experimental ADBC backend #783, after v1.11 branched). Its non-ADBC portion (backend-aware type resolution, above) is included, folded into the CTE-describe commit.is_adbc_backendguard clause and ADBC-specific exception handling were dropped from the probe-cursor fix — no ADBC backend on this branch to disagree with.deniesconfig feat: adddeniesconfig so object-level DENY survives a rebuild #802, prebuilt Dynamic Data Masking feat: apply data masks on the full_refresh_build=prebuilt path #804) are intentionally left out — this is a bugfix-only patch release.XACT_ABORT ON), not a driver defect — see the closed fix: don't let a spurious mssql-python driver error during trailing drain fail a successful batch #815 and the follow-up on feat: add sqlserver__openquery macro and SQL Server best-practices guide #790.Testing
Full unit suite passes (321 passed).