Repository navigation
fix: consolidate shared behavior and correct snapshot histories - #811
Merged
Merged
Conversation
Add compiler.graph transitive_closure_many (explicit include_starts) and replace the hand-rolled dependency-closure copies in check, lineage, compile diff, planner scopes/source loads/warehouse snapshot/audit scheduling, Python node selectors, virtual Python closure, build blocking, and Dagster selection. Behaviour is unchanged. Add characterization tests for build blocking and planner scope resolution, and unit tests for the new closure entrypoint. (cherry picked from commit 5e61a2d)
The depth-limited transitive closure walked depth-first and marked nodes visited on first discovery, so a node first reached by a long route was never re-expanded from a shorter one and its descendants within the depth limit were dropped. Walk breadth-first so every node is expanded at its minimum depth, add max_depth to transitive_closure_many, and move the lineage depth-limited walk onto the shared owner. No production caller passed max_depth to the shared owner before; lineage already used its own breadth-first walk with identical semantics. (cherry picked from commit 0bf33f3)
Build the planner model-name closure and the virtual source-freshness incomplete-model set from transitive_closure_many, keeping the resource-type projection at each call site, and resolve lineage path-between selectors through the shared path_nodes owner. Add lineage path-selector tests covering path nodes, endpoint expansion, and the unreachable-endpoint error. run_despite_unchanged._upstream_source_names is left alone: it stops at the nearest source instead of walking through it, which is a pruned traversal rather than a projected closure. (cherry picked from commit 9ab50a1)
Move the optional and required authored value validators out of the private discovery YAML primitives into compiler.authored_values, exposed as compiler-internal main entries, and replace the copies in audit argument parsing, model header parsing, schema column parsing, and dlt loader parsing. Error messages and defaults are preserved: the named boolean takes an explicit default so audit always_run keeps False while column nullable keeps None, and every caller keeps its own error class. Project config helpers keep their distinct path-free message contract and are unchanged. (cherry picked from commit b824326)
Compile output, the manifest, and the model version identity each copied the same SqlHookEntry/PythonHookEntry serializer. Replace them with compiler.discovery serialize_hook_entries, which takes an explicit SQL hook field allowlist and per-name Python hook fields. Every caller still uses the full field list, so output and identities are unchanged. (cherry picked from commit 9b21e5f)
The model version identity hashed every serialized SQL hook field, including the hook resource description and its file location, so editing a hook description or moving its file created new model versions and rebuilds without any execution change. The identity now uses an explicit SQL hook field allowlist (statement, name, definition_sql, kwargs); Python hooks keep name, kwargs, and version hash. Compile output and the manifest still include description and relative_path. Upgrading changes the version identity of every model that has a SQL hook with a description or a hook resource file, so the first plan or build after upgrading produces one round of new model versions for those models. Models without SQL hooks are unaffected. (cherry picked from commit 08472bb)
Replace discovery's parse_audit_instances primitive, the model header audit parser, and the schema column audit parser with one auditing entry, parse_audit_instances. Messages are unchanged. null_as_empty keeps the existing split: MODEL headers and column metadata treat a null audits value as no audits, while YAML model, source, and column entries keep rejecting an explicit null with "audits must be a list". (cherry picked from commit f413e44)
…stgres Cover function version upsert/get, physical relation lookup and listing order, virtual environment listing order, and active versus expired lock listings against both state backends before consolidating their SQL. (cherry picked from commit 1fae390)
Add SqlStateBackend between StateBackend and the DuckDB and Postgres state backends. It owns the read-only getters whose SQL, parameters, and row mapping were identical in both backends (model, function, seed and Python node versions, physical relations and ancestry, virtual environments, node/model/function/seed/Python refs, source freshness, checkpoints and checkpoint refs, and state operations). Backends supply only the placeholder style and the fetch calls: DuckDB runs on the connection, Postgres on a short-lived cursor, as before. Table layout, stored formats, and query results are unchanged. The Postgres file is now under the line limit, so its FFR601 exception is removed. (cherry picked from commit 85c8934)
Move the model, function, seed, Python node, physical relation, ancestry, state operation, and virtual environment upserts into SqlStateBackend on one created_at-preserving replace helper. Each upsert still runs the same lookup, DELETE, and INSERT for the same key and columns inside one BEGIN/COMMIT with ROLLBACK on failure; backends supply the transaction executor (the DuckDB connection or a Postgres cursor) and statement calls. Stored rows are unchanged: the same columns and values, created_at kept from the existing row, updated_at set to CURRENT_TIMESTAMP. (cherry picked from commit b68614c)
Move the virtual environment listing, active and expired lock listings, and lock renewal into SqlStateBackend. Their SQL, ordering, and row mapping were identical in both backends. acquire_lock, release_lock, and delete_lock stay per backend: DuckDB tolerates a failed ROLLBACK after a conflicting insert, and Postgres commits explicitly after deleting a lock. (cherry picked from commit a5f25a8)
The Dagster selection resolver expanded downstream from the set it had already expanded upstream, so +x+ also selected every descendant of x's ancestors (for example siblings of x). Expand both directions from the originally matched nodes, matching the planner, Python node, lineage, and diff selectors. Existing +x+ Dagster selections may now select fewer assets: nodes that are descendants of x's ancestors but not of x itself are no longer included. (cherry picked from commit bfc4ddf)
The Dagster resolver kept only one breadth-first shortest path between
a and b, and which branch won a tie depended on set iteration order.
Resolve path selectors through the shared path_nodes helper so a~b keeps
every node on every directed path between a and b, deterministically,
matching the planner and lineage selectors.
Behaviour change: as in the planner, x~x and a~b where b is not
downstream of a now raise DagsterDagInputError ("no SQLBuild DAG path
exists between ..."). Previously x~x selected only x.
Add a waffle shop e2e test that checks Dagster selection against
sqb plan for +x+ and path selectors.
(cherry picked from commit 0c8ec48)
…e backends Cover single-type node ref replacement, function ref replacement across udf and table_fn, grouped ref replacement, and environment publish with refs on both backends before consolidating the remaining shared state SQL. (cherry picked from commit b5b6493)
Move the node, model, seed, function, and grouped ref replacement methods, the combined environment-and-refs publish, the Python node ref upsert, and ref validation into SqlStateBackend. They were identical in both backends apart from the executor passed to the node-ref group helper. _replace_virtual_environment_node_ref_groups stays per backend behind an abstract hook: DuckDB stages refs in a temporary table and deletes missing rows before an upsert, while Postgres deletes the environment's rows for the node type and reinserts them with ON CONFLICT. (cherry picked from commit 142f22f)
Move checkpoint creation and deletion, checkpoint row inserts, and the lease-fenced conditional environment publication into SqlStateBackend, and delete the DuckDB conditional-publish mixin that held the DuckDB copies. Statement order, BEGIN/COMMIT/ROLLBACK placement (including the ROLLBACK and False return when a lease is not held), and the inserted rows are unchanged. The lease fence stays per backend behind _lease_is_owned: DuckDB bumps updated_at with UPDATE ... RETURNING, while Postgres takes a row lock with SELECT ... FOR UPDATE. (cherry picked from commit 656e120)
Move node result insert and read, the node result row mapping, the optional-equality and materialized storage helpers, and the state operation and reconcile event inserts into SqlStateBackend. SQL, parameters, ordering, and decoding were identical in both backends; the backends now only supply how a single autocommit statement runs. executor/node_results/main/_read.py keeps its own row mapping: it reads the direct-mode node results table, whose row layout (target columns, column positions, ISO timestamp fallback) differs from the virtual state node results query. (cherry picked from commit fe2de36)
Move initialize, create_backup, rollback, reset, table and index creation, migration event recording, backup listing and lookup, schema existence, column type mapping, and backup schema naming into SqlStateBackend. Each backend ran the same statements in the same order inside one BEGIN/COMMIT with ROLLBACK on failure. inspect_schema and type matching stay per backend because they read driver-specific catalog type names; delete_state_backup and delete_lock stay per backend because Postgres commits explicitly afterwards. (cherry picked from commit 22d0a94)
…Postgres state backends Extend the shared ref contract to delete one environment and check that its record and refs are removed while another environment's refs survive. (cherry picked from commit 350254f)
…ount Move close, source freshness record validation, virtual environment deletion, and the unreferenced Python node version count into SqlStateBackend. Both backends ran the same statements in the same order and transaction; only placeholders and the executor differed. prune_unreferenced_python_node_versions stays per backend: the DELETE aliases the version table as "versions" on DuckDB and "AS versions" on Postgres, so the statement text is not identical. (cherry picked from commit 7c79874)
Run the warehouse test, local test and capture scenario pipelines through one generic per-scenario loop and one target-connection context manager. Per-mode execution and failure-result construction stay in each entry point. Behaviour is unchanged: only the capture path observes the target connection with an operation lifecycle. (cherry picked from commit 53db84c)
The capture pipeline wrapped adapter.connect in the scenario_target_connection operation lifecycle, but the warehouse scenario test pipeline connected to the same target without it, so observability consumers saw no connection operation for sqb test scenarios. Both warehouse pipelines now publish it. CLI text output is unchanged because the operation has no visible progress label. (cherry picked from commit e2e4e3e)
Route the SQL function, source, scenario, audit, SQL hook, enum and constant file discoverers through one fault-isolating parse loop, and the loader, Python node, provider, sink and materialization module loaders through one import helper. Per-kind rules are kept: SQL hooks skip underscore-prefixed files, sources scan top-level YAML only, and error types and messages are unchanged. Characterisation tests pin fault isolation and import failure messages per kind. (cherry picked from commit ca2cfb6)
Loader, task, asset and check nodes now share one builder for the kind-independent fields, provider usages and identity, with a documented variant for kinds that carry tags, group, description and meta. Each kind still supplies exactly the same identity decorator_config fields as before; a characterisation test pins the per-kind field sets so version identities do not change. (cherry picked from commit b750432)
…ec table The macro, UDF and table_fn CTE classifiers shared the same loop and checks and differed only in CTE names and foreign-CTE labels. They now run through one classifier driven by a per-mode spec. Every message is unchanged, including table_fn's generic direct-logic wording; a characterisation test pins the exact message per mode. (cherry picked from commit dca429a)
…h one helper The four invocation resolvers repeated the same discovery, adapter, connection, colour and progress-stream resolution. They now call a shared runtime helper; audit, check and test also build the shared connection and planning progress reporters, while seed keeps omitting them because its planning and execution phases build their own. An integration test pins the resolved context per command against a real DuckDB project. (cherry picked from commit bf3e65a)
The capture path stringified a failed step's error_message directly, so a fixture or seed failure without a message reported "None" (or an empty message). It now uses the capture fallback message, matching the warehouse and local scenario runners. (cherry picked from commit df6a0ba)
The warehouse, local and capture scenario runners each carried their own _first_error, _first_error_code and _first_error_help copies. They now share first_failure_details, keeping each runner's fallback message and fallback codes. Semantics are unchanged: the message comes from the first failed step, while code and help come from the first failed step that has them; a unit test pins that behaviour. (cherry picked from commit 535dd6e)
SQL tests and SQL scenarios each carried an identical cached scanner for the leading WITH clause. Scenarios now call the SQL-test scanner with their own context label and CTE model. Messages are unchanged. (cherry picked from commit 72df5a3)
Scenario files reuse the SQL-test CTE scanner, so a missing WITH clause, an invalid CTE name, a missing AS keyword, a missing ceremonial SELECT 1 or an empty fixture or expected target reported "SQL test '<file>'" wording, and the missing-WITH message asked for mock CTEs. The scanner and its helpers now take the caller's context label, so scenario errors say "SQL scenario '<file>'" and ask for fixture plus expected or assertion CTEs. SQL-test messages are unchanged and pinned by tests. (cherry picked from commit c9927fc)
The rules normalisers and numeric-decision compaction treated only single
and double quotes as quoted text and never honoured backslash escapes.
In Snowflake or BigQuery a literal such as 'a\'(' or a backticked
identifier containing ) could desynchronise the scanner, so table-function
calls were left unnormalised and the model failed to parse for rules, or
Snowflake EXCLUDE/trailing-comma rewrites were skipped. The rules quote
policy now follows the sqlparser dialect used for the model: backticks
where the dialect delimits identifiers with them, and backslash escapes
where it supports them (double-quoted text only where it is a string).
(cherry picked from commit 4dc640d)
(cherry picked from commit e86b30a)
…ion iterator The SQL-test set-operation splitter, select-list end finder, top-level comma splitter, AS-alias extractor and SELECT * detector each hand-rolled the same comment-, quote- and depth-aware loop over sql_analysis primitives. They now consume one sql_analysis iter_code_positions entrypoint. Results are unchanged and pinned by characterisation tests. (cherry picked from commit 9a0ab5d)
The native and Python expected-CTE branch splitters only consumed an optional ALL after UNION, so an __expected__ CTE using UNION DISTINCT left "DISTINCT SELECT ..." as the next branch and sqb test failed with a false "must define each __expected__<model> set-operation branch as a SELECT query" error. Both splitters now accept ALL or DISTINCT. An end-to-end sqb test case covers a passing UNION DISTINCT expectation. (cherry picked from commit 08014ba)
Expected-projection validation hard-coded __expected__<model> in its alias, empty-projection, set-operation branch and branch-mismatch errors, so macro, UDF and table_fn tests were told to fix an __expected__<model> CTE they do not have. The native extractor, the Python classifier and the Polyglot path now report the validated CTE (for example __macro_expected__); model-mode wording is unchanged. (cherry picked from commit 88c0890)
…rDuck, Postgres and SQL Server Historical check snapshots with invalidate_hard_deletes now open a new active version when a hard-deleted key reappears in a later observation with unchanged check columns, matching the documented behaviour and the BigQuery, Databricks and Snowflake adapters. Previously these four adapters left the key without an active version until a check column changed. Behaviour change: existing history rows are not rewritten. A key that is currently closed by a hard delete and is present in a later observation gets a new active version, starting at that observation, on the next build. - Initial build: a row also starts a version when the key was absent from the immediately preceding observation group. - Incremental build: the first observation after a hard-deleted latest version is added to the new changes. The latest version counts as hard-deleted only when its key is absent from the observation at its valid_to, so a version closed by a change in the same run is not reopened. Rows are deduplicated by unique key and observed_at, with no QUALIFY or whole-row UNION DISTINCT, so the SQL works on Postgres and SQL Server. (cherry picked from commit c9ef3ed)
Render historical and current-state snapshot SQL for the check and timestamp strategies, with and without hard deletes, through the initial and incremental paths, and execute it on DuckDB for the DuckDB, MotherDuck, Postgres, BigQuery, Snowflake and Databricks adapters. Every adapter must produce the same expected history and keep the snapshot invariants: no overlapping windows and at most one active version per key. The only shims are BigQuery identifier quoting and a pinned execution clock for current-state histories. SQL Server renders T-SQL that DuckDB cannot run and keeps rendering assertions only. (cherry picked from commit dd8327e)
…owflake and Databricks Historical check snapshots with invalidate_hard_deletes on BigQuery, Snowflake and Databricks recorded a second, identical version when an incremental build contained a change followed by an unchanged observation of the new value: the reappearance branch treated the version closed by that change as hard-deleted. These adapters now use the shared reappearance rendering already used by DuckDB, MotherDuck, Postgres and SQL Server, which only counts a closed latest version as hard-deleted when its key is missing from the observation at valid_to. Existing duplicate rows are not rewritten. (cherry picked from commit 2b350ff)
…ical build Historical check and timestamp snapshots with invalidate_hard_deletes could only close the currently active version during an incremental build. Versions inserted by the same build ended at the next change only, so a key that was deleted and reappeared more than once in one build kept a version spanning the later deletions, and a check value that changed and changed back in one build lost the return to the earlier value. The final history was only correct after further builds. All adapters (DuckDB, MotherDuck, Postgres, SQL Server, BigQuery, Snowflake and Databricks) now render these paths from one shared, portable builder that uses window functions and joins only: no QUALIFY, no UNION DISTINCT and no correlated subqueries. Every new version now ends at the next version start or at the first later observation that is missing its key, on the initial and the incremental path alike, so one build produces the final history immediately. Initial full-history builds produce the same history as before. Existing history rows are not rewritten. (cherry picked from commit d6e86ba)
…hard delete Historical timestamp snapshots with invalidate_hard_deletes left a hard-deleted key without an active version when it reappeared in a later observation with an unchanged updated_at, on every adapter. The key now gets a new active version whose valid_from is the observation time at which it reappeared, matching the observation-time boundary used when the deletion closed the previous version. A key that reappears with a changed updated_at still starts at that updated_at, and ordinary changes keep valid_from = updated_at. Applies to DuckDB, MotherDuck, Postgres, SQL Server, BigQuery, Snowflake and Databricks, on initial and incremental builds. Existing history rows are not rewritten; a key that is currently closed by a hard delete and present in a later observation is reopened on the next build. (cherry picked from commit 3e59b1e)
…r historical timestamp build The first build of a SQL Server historical timestamp snapshot ignored invalidate_hard_deletes, so keys missing from later observations in the initial history stayed active and reappearing keys were not reopened until later incremental builds. The initial build now renders the same hard-delete history as the incremental path and the other adapters: deletions close a version at the observation where the key is missing, and a key that reappears with an unchanged updated_at opens a new version at that observation. Existing SQL Server snapshot tables are not rewritten. (cherry picked from commit 273dc4b)
… execution time Current-state timestamp snapshots with invalidate_hard_deletes reopened a key that came back after a hard delete with valid_from = updated_at. Because updated_at usually predates the deletion, the new version overlapped the closed one. A reappearing key now starts at the execution time of the build that sees it again, the same boundary the hard delete used to close it. New keys and ordinary updated_at changes keep their existing valid_from. Applies to DuckDB, MotherDuck, Postgres, SQL Server, BigQuery, Snowflake and Databricks. Existing overlapping rows are not rewritten. (cherry picked from commit 7e77b82)
Move private snapshot SQL helpers that were byte-for-byte copies across adapters into shared builders next to HistoricalSnapshotSql: - SnapshotSql.key_condition replaces seven copies of _snapshot_key_condition. - HistoricalCheckSnapshotSql holds the initial-build query and the QUALIFY-based incremental CTEs previously copied into the base, DuckDB, BigQuery, Snowflake and Databricks adapters. - HistoricalSnapshotStatementSql holds the UPDATE ... FROM close and INSERT ... WITH statements copied between BigQuery and Snowflake. Rendered SQL is unchanged for every adapter. (cherry picked from commit d650c86)
…elpers Move the historical timestamp and historical changes helpers that were copied into the base, DuckDB, BigQuery, Snowflake and Databricks adapters into HistoricalTimestampSnapshotSql: the initial-build queries and the QUALIFY-based incremental CTEs. Postgres and SQL Server now call the same shared builder where they previously imported the base adapter's module-level copy. Rendered SQL is unchanged for every adapter. (cherry picked from commit 5a81711)
Move _snapshot_initial_valid_from_expr and the standard _snapshot_hard_delete_close_sql, copied into the base, DuckDB, BigQuery, Snowflake and Databricks adapters, into SnapshotSql. Postgres and SQL Server use the shared initial valid_from expression; SQL Server keeps its T-SQL hard-delete close until the parameterised current-state slice. Rendered SQL is unchanged for every adapter. (cherry picked from commit 77ea825)
Add SnapshotSqlDialect, which names the places where adapters really render snapshot SQL differently: the null timestamp type, the null-safe inequality, how the latest stored version is selected (QUALIFY, derived table or ordered CTE), how historical versions are closed (correlated UPDATE, UPDATE ... FROM, Databricks MERGE for hard-delete closes, or T-SQL UPDATE ... FROM), where insert CTEs go, and how current-state versions are closed. Each adapter declares its dialect once. HistoricalCheckSnapshotSql now renders the complete incremental historical check statements for every adapter from that dialect, and HistoricalSnapshotStatementSql owns the shared latest-version, close and insert statements. render_apply_historical_check_snapshot_changes delegates on every adapter, and the Postgres and SQL Server copies of the incremental CTE builder are removed. BigQuery and Snowflake historical timestamp closes and inserts use the same statement builder. Rendered SQL is unchanged for every adapter. (cherry picked from commit 5098cd2)
…t SQL HistoricalTimestampSnapshotSql now renders the incremental historical timestamp and historical changes statements for every adapter from its SnapshotSqlDialect. render_apply_historical_timestamp_snapshot_changes and render_apply_historical_timestamp_changes delegate on every adapter, and the Postgres, SQL Server, DuckDB, Databricks and base copies of the incremental CTE and hard-delete close builders are removed. SQL Server's historical changes path keeps rendering QUALIFY for the latest stored version, exactly as before; it passes that explicitly instead of inheriting it from the base module. Rendered SQL is unchanged for every adapter. (cherry picked from commit 6749396)
SnapshotSql now renders the current-state initial query and the timestamp and check change statements for every adapter from its SnapshotSqlDialect: UPDATE ... FROM, Databricks MERGE, or T-SQL UPDATE ... FROM ... JOIN for closes, the T-SQL hard-delete close, the dialect's null timestamp type and null-safe inequality. render_create_initial_snapshot_destination, render_apply_timestamp_snapshot_changes and render_apply_check_snapshot_changes delegate on every adapter, and the SQL Server hard-delete close copy is removed. Rendered SQL is unchanged for every adapter. (cherry picked from commit 9cfef9b)
…one builder The BigQuery/Snowflake UPDATE ... FROM close and the Databricks MERGE close shared the same earliest-candidate query and close condition. Render both from one method. Rendered SQL is unchanged for every adapter. (cherry picked from commit 11fecb7)
…t methods (cherry picked from commit 2ef529a)
…ges snapshots Incremental builds of SQL Server historical timestamp snapshots with historical_input changes selected each key's latest stored version with QUALIFY, which SQL Server does not support, so those builds failed. The path now uses the same ordered-CTE form as the other SQL Server snapshot paths: ROW_NUMBER() in a __latest_ordered CTE filtered to the first row in __latest, inside the ;WITH statement. No other adapter or snapshot configuration changes. (cherry picked from commit 9b72b1f)
…olidate-duplicates
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.
Why
Duplicated implementations had drifted across adapters, state backends and compiler paths, causing inconsistent snapshot, selector and SQL-test behavior.
Changes
+x+may select fewer assets,a~bincludes all paths, and identical/unreachable endpoints error. Full-history snapshot sources may add previously missed versions after upgrading.Verification