Conversation
…d=False corrupts manifest When a test runs dbt with elementary_enabled=False (e.g. test_collect_metrics_elementary_disabled), dbt rewrites target/manifest.json with elementary models in the 'disabled' section instead of 'nodes'. If a subsequent test on the same xdist worker uses AdapterQueryRunner to read_table, it reads this stale manifest and fails with 'Cannot resolve ref: not found in dbt manifest'. Fix: _load_manifest_maps now scans both 'nodes' and 'disabled' sections. For disabled nodes that lack relation_name (dbt skips compilation for disabled nodes), the relation name is synthesized from database/schema/alias using the adapter's Relation class. Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
👋 @devin-ai-integration[bot] |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdapterQueryRunner now builds missing Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
integration_tests/tests/adapter_query_runner.py (1)
138-150: Broad exception handling is pragmatic but consider logging for debuggability.The static analysis tool flags the bare
except Exceptionblocks. While the PR notes indicate this is intentional best-effort behavior for cross-adapter compatibility, silently swallowing exceptions can make debugging difficult when unexpected failures occur.Consider logging at debug/warning level when falling back, so issues can be diagnosed in CI logs without breaking the flow:
🔧 Optional: Add debug logging for fallback paths
except Exception: # Some adapters (e.g. ClickHouse) reject certain database values. # Retry without database, then fall back to simple dot-join. + logger.debug( + "Relation.create failed with database=%r for %s; retrying without database", + database, + identifier, + ) try: relation = self._adapter.Relation.create( database="", schema=schema, identifier=identifier, ) return relation.render() except Exception: parts = [p for p in (schema, identifier) if p] + logger.debug( + "Relation.create failed without database for %s; using dot-join fallback", + identifier, + ) return ".".join(parts) if parts else None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration_tests/tests/adapter_query_runner.py` around lines 138 - 150, The broad except blocks around self._adapter.Relation.create and the fallback render swallow exceptions and hinder debugging; add non-intrusive logging there (e.g., get a module logger via logging.getLogger(__name__) or reuse an existing logger) and call logger.debug or logger.warning with a short contextual message and exc_info=True when each except Exception is hit, keeping the existing fallback behavior (return rendered relation or dot-joined parts) unchanged; reference the self._adapter.Relation.create call and the relation.render/fallback join paths so the logs identify which branch failed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@integration_tests/tests/adapter_query_runner.py`:
- Around line 170-178: The code currently treats manifest["disabled"] values as
possibly non-lists and includes an unnecessary else branch; simplify disabled
node handling by assuming values are lists per the dbt manifest schema: get
disabled = manifest.get("disabled", {}), then iterate disabled.values() and call
all_nodes.extend(versions) for each entry (remove the isinstance(versions, list)
check and the all_nodes.append branch); update the block around variables
all_nodes, disabled, and manifest accordingly.
---
Nitpick comments:
In `@integration_tests/tests/adapter_query_runner.py`:
- Around line 138-150: The broad except blocks around
self._adapter.Relation.create and the fallback render swallow exceptions and
hinder debugging; add non-intrusive logging there (e.g., get a module logger via
logging.getLogger(__name__) or reuse an existing logger) and call logger.debug
or logger.warning with a short contextual message and exc_info=True when each
except Exception is hit, keeping the existing fallback behavior (return rendered
relation or dot-joined parts) unchanged; reference the
self._adapter.Relation.create call and the relation.render/fallback join paths
so the logs identify which branch failed.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
integration_tests/tests/adapter_query_runner.py
…d node handling Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
| ) | ||
| return relation.render() | ||
| except Exception: | ||
| # Some adapters (e.g. ClickHouse) reject certain database values. |
There was a problem hiding this comment.
Can we be more deterministic and choose the approach based on the adapter, instead of double try..except?
There was a problem hiding this comment.
Good call — replaced the double try/except with the adapter's include_policy. Now we check self._adapter.Relation.get_default_include_policy().database to decide whether to pass the database field. ClickHouse has database=False in its include policy, so we pass "" — no exception handling needed.
See commit d6f6b44.
…database handling Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
1 participant
fix: resolve refs from disabled manifest nodes (transient ClickHouse CI failure)
Summary
Fixes a transient CI failure where
AdapterQueryRunner(introduced in #936) fails withCannot resolve ref('X'): not found in dbt manifeston ClickHouse (and potentially other targets with many skipped tests).Root cause: When
test_collect_metrics_elementary_disabledrunsdbt testwithelementary_enabled=False, dbt rewritestarget/manifest.jsonwith all elementary models moved to thedisabledsection instead ofnodes. If a subsequent test on the same xdist worker usesAdapterQueryRunner.read_table(), it reads this stale manifest and can't find models likeseed_run_results,model_run_results, etc. This is transient because xdist worker assignment varies between runs — ClickHouse is most susceptible because ~75% of tests are skipped, concentrating fewer tests per worker and increasing the collision probability.Fix:
_load_manifest_mapsnow scans bothnodesanddisabledsections. For disabled nodes that lackrelation_name(dbt skips compilation for disabled nodes, so this field isNone), the relation name is synthesised fromdatabase/schema/aliasusing the adapter'sRelationclass, with fallback handling for adapters like ClickHouse that reject certaindatabasevalues.Failing CI job: https://github.com/elementary-data/dbt-data-reliability/actions/runs/22520832544/job/65245341210?pr=943
Review & Testing Checklist for Human
manifest.jsonhavedatabase,schema, andalias/namefields populated. This was confirmed from the dbt dataclass definitions but has not been validated against a real serialised manifest with disabled elementary models. Consider generating one locally withelementary_enabled=Falseand inspecting the JSON.except Exceptionin_build_relation_name: The two try/except blocks catch all exceptions, which could silently swallow unexpected errors beyond adapter validation failures. This is intentional (it's a best-effort fallback), but verify the tradeoff is acceptable._build_relation_namemethod and disabled-node scanning have no dedicated tests. Consider whether the existing integration test suite (which exercises this path end-to-end on ClickHouse CI) provides sufficient coverage, or if a unit test with a mock manifest would be worthwhile.Notes
disabledmanifest section format isDict[UniqueID, List[resource]]per dbt-core'sWritableManifestdataclass. The code handles both list and non-list values defensively.ref_map.setdefaultis used, so a disabled node never overwrites an enabled one with the same name.Summary by CodeRabbit