fix: add ref() macro override for dbt-core 1.11 compatibility#2137
fix: add ref() macro override for dbt-core 1.11 compatibility#2137
Conversation
Some adapters (e.g. dbt-dremio) override the ref() Jinja macro with a
signature that doesn't support the two-argument positional form used for
cross-package references: ref('package', 'model').
dbt-dremio's ref(model_name, v=None) intercepts the second positional
arg as a version identifier, breaking refs like ref('elementary', 'dbt_models').
This override in the elementary_cli monitor project forwards all arguments
directly to builtins.ref(), which correctly handles all ref() forms in
dbt-core 1.11+. It also supports the legacy package= keyword form for
backward compatibility.
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] |
📝 WalkthroughWalkthroughAdds a new DBT macro override 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@elementary/monitor/dbt_project/macros/overrides/ref.sql`:
- Around line 17-34: The ref macro currently branches on model_name/package
before handling version, which drops v/version for combined
cross-package+version calls; update the macro ref(model_name_or_package,
model_name=none, v=none, version=none, package=none) to first compute an
effective_version (e.g., effective_version = v if v is not none else version)
and then pass that effective_version into every builtins.ref call that takes two
arguments (both the two-arg positional branch using
builtins.ref(model_name_or_package, model_name) and the legacy package branch
using builtins.ref(package, model_name_or_package)), and also use
builtins.ref(model_name_or_package, v=effective_version) for single-arg
versioned calls so that any combination of cross-package + version preserves the
version argument.
ℹ️ 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)
elementary/monitor/dbt_project/macros/overrides/ref.sql
|
Note: This PR is a workaround for an open upstream bug in dbt-dremio: dremio/dbt-dremio#232 — Override of dbt-dremio's Our project-level override properly forwards all argument forms to |
…tibility Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
elementary/monitor/dbt_project/macros/overrides/ref.sql (1)
17-40: Add a regression test matrix for this override path.Consider adding an integration/macro test that exercises
ref('pkg','model'),ref('model', package='pkg'), and their versioned variants. This will lock in the compatibility fix and prevent future regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@elementary/monitor/dbt_project/macros/overrides/ref.sql` around lines 17 - 40, The ref override macro handles multiple call patterns (macro ref, variable effective_version, and calls to builtins.ref) and needs coverage to prevent regressions; add an integration/macro test matrix that calls the two-arg positional form ref('pkg','model'), the legacy keyword form ref('model', package='pkg'), the single-arg versioned forms ref('model', v=1') and ref('model', version=1), plus the two-arg versioned variant ref('pkg','model', v=1) (or equivalent), asserting that each resolves to the same target and that builtins.ref is invoked with the expected arguments for each branch (including branches that use effective_version and package/model_name). Ensure tests exercise the branches guarded by model_name, package, and effective_version so future changes to macro ref or builtins.ref calls will be detected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@elementary/monitor/dbt_project/macros/overrides/ref.sql`:
- Around line 17-40: The ref override macro handles multiple call patterns
(macro ref, variable effective_version, and calls to builtins.ref) and needs
coverage to prevent regressions; add an integration/macro test matrix that calls
the two-arg positional form ref('pkg','model'), the legacy keyword form
ref('model', package='pkg'), the single-arg versioned forms ref('model', v=1')
and ref('model', version=1), plus the two-arg versioned variant
ref('pkg','model', v=1) (or equivalent), asserting that each resolves to the
same target and that builtins.ref is invoked with the expected arguments for
each branch (including branches that use effective_version and
package/model_name). Ensure tests exercise the branches guarded by model_name,
package, and effective_version so future changes to macro ref or builtins.ref
calls will be detected.
ℹ️ 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)
elementary/monitor/dbt_project/macros/overrides/ref.sql
Summary
Adds a
ref()Jinja macro override in the elementary CLI's internal monitor dbt project (elementary_cli) to fix cross-packageref()calls breaking on adapters that overrideref()with an incompatible signature (specifically dbt-dremio).Root cause: dbt-dremio overrides
ref()withref(model_name, v=None), which intercepts the second positional arg as a version identifier. When the monitor project callsref('elementary', 'dbt_models'), dbt-dremio treats it asbuiltins.ref('elementary', v='dbt_models')— looking for a model named "elementary" at version "dbt_models" — instead of a cross-package ref. This is a known upstream bug: dremio/dbt-dremio#232 (open since Jun 2024, unresolved).Fix: A project-level
ref()macro that properly dispatches all argument forms directly tobuiltins.ref(), which correctly handles two-arg positional cross-package refs in dbt-core 1.11+. Project macros take precedence over adapter macros in dbt's resolution order, so this overrides dbt-dremio's broken version.The macro handles all
ref()forms including combined cross-package + versioned refs (e.g.ref('package', 'model', v=1)) for forward compatibility, using aneffective_versionconsolidation approach.Review & Testing Checklist for Human
ref()actually overrides adapter-levelref(): dbt's macro resolution gives priority to root project macros over adapter macros, butrefis a special builtin — confirm this override takes effect when running with dbt-dremio (and other adapters). If it doesn't, this PR has no effect.ref()override for critical functionality that we'd be bypassing: dbt-dremio'sref()adds format/reflection handling. For the monitor project (which only reads existing tables), this should be safe to bypass — but confirm there's no case where the monitor project creates/materializes models that need adapter-specific ref behavior.edr monitororedr reportagainst a Dremio instance to confirm cross-package refs likeref('elementary', 'dbt_models')and self-refs likeref('elementary_cli', 'alerts_v2')resolve correctly.Suggested test plan: Run the Dremio CI job from PR #2135 without
continue-on-erroron the edr steps, using this branch's monitor project. Ifedr monitor reportsucceeds, the fix works.Notes
(*args, **kwargs)and forward them correctly to builtins. dbt-dremio is currently the only adapter that overridesref()this way.package=keyword form (ref('model', package='pkg')) for backward compatibility with older dbt-core versions, though dbt-core 1.11 itself removed this.numeric_column_anomalies+error_modelissues on master, unrelated to this change.Link to Devin Session: https://app.devin.ai/sessions/6c12a19d456541568bcd7974623f3557
Requested by: @haritamar
Summary by CodeRabbit