perf: allow V1 incremental path to skip config change metadata queries - #1403
perf: allow V1 incremental path to skip config change metadata queries#1403moomindani wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Hi @moomindani
Thanks for the PR.
Now that this code path uses process_config_changes, it adds column_tags, column_masks, and comment to the V1 incremental path as well. Can you please add V1 functional tests to validate these changes?
Also please add a functional test for behaviour when incremental_apply_config_changes is set to false and that the metadata are not fetched.
| {% endif %} | ||
| {%- endif -%} | ||
| {{ process_config_changes(target_relation) }} | ||
| {% do persist_docs(target_relation, model, for_relation=True) %} |
There was a problem hiding this comment.
Doesn't process_config_changes take care of persist_docs as well?
|
Thanks for the review! Addressed both points in 2882e9c. Re: Yes — Re: functional tests Added three tests:
|
|
Thanks @moomindani. We will look to merge this. |
|
Hi @moomindani Keeping this in mind, we will not merge this PR. Sorry for the delayed response. |
|
Thanks for the context from @benc-db about the column masks security concern. Addressed in c66209d:
|
|
Hi @moomindani can you look to rebase this on |
|
Thanks @sd-db, makes sense. I'll rebase onto |
databricks#1402) Replace inline config change detection/application in the V1 incremental path with the existing `process_config_changes()` macro already used by V2. This allows users to set `incremental_apply_config_changes: false` to skip 8 unnecessary information_schema queries per incremental model execution. Co-authored-by: Isaac
- Remove persist_docs call from V1 incremental merge path since process_config_changes -> apply_config_changeset already handles comment and column_comments changes (aligns with V2 behavior) - Add V1 functional tests for column_tags and column_masks changes - Add functional test verifying incremental_apply_config_changes=false skips all metadata fetch queries (fetch_tags, fetch_column_tags, fetch_non_null_constraint_columns, fetch_primary_key_constraints, fetch_foreign_key_constraints, fetch_column_masks) Co-authored-by: Isaac
- Add V2 guard to apply_column_masks in apply_config_changeset to prevent unmasked data exposure in V1 (CTAS writes data before masks can be applied, whereas V2 creates an empty table first) - Add TestV1IncrementalColumnMasksNotApplied: overrides apply_column_masks to raise error, verifying it is never called in V1 incremental path - Remove V1 column_tags/column_masks tests that are no longer applicable - Keep TestV1IncrementalSkipConfigChanges for metadata skip validation Co-authored-by: Isaac
c66209d to
cd551ae
Compare
|
@sd-db Rebased onto
Ready for re-review when you have time. |
…ath (#1467) Resolves #1402 Follow-up to closed #1403 with a much smaller surface area, in response to @sd-db's earlier review. ### Description The V1 incremental merge path calls `adapter.get_relation_config()` unconditionally during every incremental run, forcing metadata diff queries (tags, column_tags, constraints, column_masks, tblproperties, describe_extended) on every model. The V2 path already honors `incremental_apply_config_changes: false` via the `process_config_changes()` macro; V1 ignored the flag. #1387 partially addressed this by auto-skipping `fetch_tags` / `fetch_column_tags` when those configs are absent. This PR is complementary, not duplicate: | | #1387 (merged) | This PR | |---|---|---| | Mechanism | Auto-detect via `requires_server_metadata_for_diff()` | Explicit user-facing flag | | Coverage | tags, column_tags (2 queries) | All metadata fetches behind `get_relation_config` | | Use case | Default behavior optimization | Deterministic opt-out: cost control, edge cases where auto-detect can't decide | **Change:** wrap the V1 `get_relation_config` call in an `if config.get('incremental_apply_config_changes', True)` block. When `false`, `_configuration_changes` stays `none` and the existing apply-block guard short-circuits the apply. Default behavior unchanged. **Security:** the V1 inline apply block never pulled `column_masks` from the changeset to begin with — so `column_masks` are not applied in V1 today, and remain not applied with this change. The flag set to `false` strictly reduces work; no new application path is introduced. The `apply_column_masks` V2-only guard from the final commit of #1403 is therefore not needed in this scoped change. **Diff size:** +6 / -3 in `incremental.sql`, plus CHANGELOG and one functional test. ### Test plan - [x] Unit tests pass locally (1081 passed) - [x] New functional test \`TestV1IncrementalApplyConfigChangesFalseSkipsTagFetch\` passes against a UC SQL warehouse on \`e2-demo-tokyo\` - [x] Verified the test FAILS on unfixed \`1.12.latest\` code (raises \"tags should not be called\") ### Checklist - [x] I have run this code in development and it appears to resolve the stated issue - [x] This PR includes tests, or tests are not required/relevant for this PR - [x] I have updated the \`CHANGELOG.md\` and added information about my change to the new \"dbt-databricks 1.12.1 (TBD)\" section. Co-authored-by: Shubham Dhal <shubham.dhal@databricks.com>
Resolves #1402
Description
The V1 incremental materialization path calls
adapter.get_relation_config()unconditionally during every incremental merge run. This triggers 8 sequential metadata queries againstinformation_schemaandsystemtables — even when none of the related features (tags, constraints, column masks) are in use.The V2 path already uses
process_config_changes(), which respects theincremental_apply_config_changesconfig flag. This PR replaces the V1 inline code with the same macro, bringing V1 in line with V2.Change:
{{ process_config_changes(target_relation) }}callQueries eliminated when
incremental_apply_config_changes: false:SELECT ... FROM system.information_schema.table_tagsSELECT ... FROM system.information_schema.column_tagsSELECT ... FROM information_schema.columns(NOT NULL constraints)SELECT ... FROM information_schema.key_column_usage(PRIMARY KEY)SELECT ... FROM information_schema.key_column_usage(FOREIGN KEY)SELECT ... FROM system.information_schema.column_masksSHOW TBLPROPERTIESDESCRIBE TABLE EXTENDEDMeasured results (Serverless SQL Warehouse, incremental merge model with
auto_liquid_cluster: true):incremental_apply_config_changes: falseget_relation_configoverheadTest assets used for benchmarking
dbt_project.yml:
models/incremental_merge_test.sql (default run):
{{ config( materialized='incremental', incremental_strategy='merge', unique_key='id', auto_liquid_cluster=true ) }} SELECT id, name, category, amount, updated_at FROM ( VALUES (1, 'Alice', 'A', 100.0, current_timestamp()), (2, 'Bob', 'B', 200.0, current_timestamp()), (3, 'Charlie', 'A', 150.0, current_timestamp()), (4, 'Diana', 'C', 300.0, current_timestamp()), (5, 'Eve', 'B', 250.0, current_timestamp()) ) AS t(id, name, category, amount, updated_at)models/incremental_merge_test.sql (skip run — only config diff):
{{ config( materialized='incremental', incremental_strategy='merge', unique_key='id', auto_liquid_cluster=true, incremental_apply_config_changes=false ) }}Steps:
dbt run(initial table creation) →dbt run(incremental merge, default) → addincremental_apply_config_changes=false→dbt run(incremental merge, skip). Comparedbt.logfrom the last two runs.Query-level breakdown (default run)
Query-level breakdown (skip run)
Functional parity:
apply_config_changeset(used byprocess_config_changes) handles all config types that the V1 inline code handled (tags, tblproperties, liquid_clustering, constraints) plus additional types (column_comments, column_tags, column_masks).When
incremental_apply_config_changesistrue(default), behavior is unchanged — all 8 queries run and config changes are applied.Checklist
CHANGELOG.mdand added information about my change to the "dbt-databricks next" section.