perf: skip unnecessary metadata fetch calls for tags when not configured - #1387
Conversation
Skip fetch_tags and fetch_column_tags information_schema queries during incremental and view materializations when the model has no tags configured. This avoids unnecessary server roundtrips on every run for models that don't use tags, while preserving full fetch behavior when tags are present or when the model config is unavailable. PECOBLR-2497
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||
Add unit and functional coverage for skipping tag metadata queries when model config does not require them. This protects the new fetch-planning logic across incremental, view, streaming table, and materialized view test paths without changing unrelated unstaged work.
partial_parse.msgpack does not persist compiled_code, causing get_config_from_model to raise "Cannot compile model ... with no SQL query" for materialized view and streaming table change tests. Co-authored-by: Isaac
Co-authored-by: Isaac
The previous `dict and len(...) > 0` short-circuited to a dict literal when empty, breaking the declared `-> bool` return type. Co-authored-by: Isaac
sd-db
left a comment
There was a problem hiding this comment.
Added some more comments, overall much better. I still feel we should add a comment in all the materialisation types where we are doing this optimisation that this only works because we don't support deleting/removing tags through dbt-databricks, as that will break the optimisations done and better to encode as a comment
| results["information_schema.tags"] = adapter.execute_macro("fetch_tags", kwargs=kwargs) | ||
| results["show_tblproperties"] = adapter.execute_macro("fetch_tbl_properties", kwargs=kwargs) | ||
|
|
||
| table_tag_config = model_config.config.get(TagsProcessor.name) if model_config else None |
There was a problem hiding this comment.
rename to model_table_tag_config we should differentiate between desired(model) configs and existing (relation) configs better
There was a problem hiding this comment.
Config only comes from user and its obvious here.
sd-db
left a comment
There was a problem hiding this comment.
Changes look good, thanks !
|
/integration-test |
|
Integration tests dispatched for PR #1387 by @tejassp-db. Track progress in the Actions tab. |
|
Integration results for PR #1387 — UC cluster ❌ failure · SQL warehouse ❌ failure · All-purpose cluster ✅ success · Shard coverage ✅ success |
|
/integration-test |
|
Integration tests dispatched for PR #1387 by @tejassp-db. Track progress in the Actions tab. |
|
Integration results for PR #1387 — UC cluster ✅ success · SQL warehouse ✅ success · All-purpose cluster ✅ success · Shard coverage ✅ success |
…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>
Summary
What changed
Test plan
Unit tests: empty tags (skip), non-empty tags (fetch), null config fallback (fetch), both tags + column tags present, hive_metastore (skip)
Functional tests: override fetch_tags/fetch_column_tags macros to raise errors, confirming calls are actually skipped when expected
PECOBLR-2497