Strengthen MetricFlow ingestion into SLayer (DEV-1595)#202
Merged
ZmeiGorynych merged 18 commits intoJul 1, 2026
Merged
Conversation
Make the dbt/MetricFlow importer either represent every legal DSI construct exactly or fail it cleanly into a structured report — no silent drops, no approximate/lossy SQL. count_distinct_approx (dialect-aware aggregation): - enums: builtin + eligible on every type and PK columns + aliases - SqlDialect.build_approx_count_distinct: native function per dialect, exact COUNT(DISTINCT) fallback (Postgres/SQLite/MySQL) - generator dispatch composing with the filter-CASE wrapper Importer correctness + represent-exactly: - percentile p= (+ discrete/approx flag clean-fail), ratio nullif guard - sum_boolean -> CASE INT column, offset_window -> time_shift - metric/per-input filter push-down with leaf dedup + cross-model reachability; string-or-list filter normalization Parser completeness: cumulative/conversion/aggregation type-params, time-window parsing, measure str-or-object, join_to_timespine/fill_nulls_with, config.meta. extra="ignore" kept. Clean-fail routing + report: ConversionWarning category/severity/suggestion, render_report(), meta stash of dropped constructs, target_dialect percentile caveat, CLI grouped report + tally. Docs updated across dbt import, database-support, aggregation, model, and skill references. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Changescount_distinct_approx Aggregation
dbt MetricFlow Ingestion Strengthening
MetricFlow demo materials
Sequence Diagram(s)sequenceDiagram
participant CLI as slayer import-dbt
participant Converter as DbtToSlayerConverter
participant Models as slayer/dbt/models.py
participant MetricConv as _convert_metric
participant FilterLeaf as filtered leaf column
participant Dialect as SqlDialect
participant SQLGen as SQLGenerator
CLI->>Converter: convert(target_dialect=...)
Converter->>Models: parse dbt semantic models and metrics
Models-->>Converter: typed DSI objects
Converter->>MetricConv: convert metric into SLayer measure
alt supported metric with filters
MetricConv->>FilterLeaf: create or reuse filtered leaf
FilterLeaf-->>MetricConv: leaf column reference
MetricConv-->>Converter: model measure formula
else unsupported construct
MetricConv-->>Converter: structured ConversionWarning
Converter->>Converter: stash raw semantics in meta
end
Converter-->>CLI: ConversionResult
CLI->>CLI: render_report()
CLI->>CLI: tally()
SQLGen->>SQLGen: _build_agg("count_distinct_approx")
SQLGen->>Dialect: build_approx_count_distinct(col_sql, parse)
Dialect-->>SQLGen: native approx SQL or COUNT(DISTINCT)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
slayer/cli.py (1)
1318-1323: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPass the datasource dialect into the converter.
DbtToSlayerConverter._maybe_dialect_caveat()only runs whentarget_dialectis populated, but the CLI never sets it. Soslayer import-dbtwill print the grouped report without the MySQL/T-SQL percentile/median caveats this PR added, and users only discover the problem at query time.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@slayer/cli.py` around lines 1318 - 1323, The CLI is instantiating DbtToSlayerConverter without the target dialect, so _maybe_dialect_caveat() never emits the new percentile/median warnings during slayer import-dbt. Update the converter construction in the CLI to pass the datasource dialect from args.datasource (or the resolved datasource object) into the target_dialect field, using DbtToSlayerConverter and _maybe_dialect_caveat as the key symbols to locate the change.slayer/dbt/converter.py (2)
395-409: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMerge entity metadata into reused PK columns too.
When the entity column already exists, this branch only flips
primary_key=True. Any entitylabel,description,config.meta, or derivedrolemetadata is dropped, even though the synthetic-column branch preserves it.As per coding guidelines, column
metamust be preserved when present.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@slayer/dbt/converter.py` around lines 395 - 409, In the entity-column reuse branch, only primary_key is being updated on the existing Column, so the entity’s label, description, meta, and derived role information are lost compared with the synthetic-column path. Update the existing-column handling in the converter logic to merge the same entity metadata onto the matched Column instance in addition to setting primary_key, preserving any existing column meta when present. Focus on the branch that iterates over cols and matches c.name to col_name, and keep the behavior consistent with the Column(...) construction used when the column is newly appended.Source: Coding guidelines
1352-1378: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClean-fail unfiltered simple metrics whose backing measure never converted.
If the backing dbt measure was dropped in
_convert_measures()(for example, unsupported percentile or non-additive cases),_resolve_metric_to_name()returnsNone. The downstream callers then keep the original metric name in the formula, so derived/ratio metrics can be emitted with references that do not exist on the model. This should be routed to_fail_metric(...)at the first unresolved simple-metric boundary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@slayer/dbt/converter.py` around lines 1352 - 1378, The _resolve_metric_to_name method in the metric resolution path should clean-fail when an unfiltered simple metric’s backing measure cannot be resolved instead of returning None and letting callers keep the original metric name. Update the logic around _resolve_metric_to_name, _resolve_measure_to_name, and the first simple-metric handling branch to detect this unresolved case and route it through _fail_metric(...) immediately, so derived and ratio formulas never reference a missing model symbol.
🧹 Nitpick comments (3)
tests/test_filtered_count_forms.py (1)
45-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a filtered
count_distinct_approxregression here.
slayer/sql/generator.pynow has a dedicated_build_approx_count_distinct()path with its own_wrap_filter(...)call, but this file never exercises it. Acust:count_distinct_approxcase would lock down the new branch and catch future regressions in filtered metric SQL.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_filtered_count_forms.py` around lines 45 - 57, Add a regression test in test_filtered_count_forms for the new filtered count_distinct_approx path, using _gen("cust:count_distinct_approx") and asserting the generated SQL places the CASE filter inside the APPROX distinct count expression. This should specifically exercise the new _build_approx_count_distinct() branch in slayer/sql/generator.py and confirm its _wrap_filter(...) behavior matches the existing count, count_distinct, and sum tests.slayer/dbt/models.py (1)
28-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit
_normalize_filter()into smaller extraction helpers.The dict/list branches are doing the same clause-unwrapping work twice, which is what tripped the cognitive-complexity gate here. Normalizing the input to one iterable and extracting each clause through a tiny helper would make future DSI-shape additions much easier to extend safely.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@slayer/dbt/models.py` around lines 28 - 68, Split _normalize_filter() into smaller helpers by extracting the repeated clause-unwrapping logic from the dict and list/tuple branches into a single small helper, then have _normalize_filter() normalize the input to one iterable and build parts through that helper; keep the existing behavior for strings, None, and fallback values while reducing duplication and cognitive complexity.Source: Linters/SAST tools
tests/integration/test_integration_duckdb.py (1)
316-330: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the base monthly totals alongside
running.The docstring says this verifies
cumsum(total:sum)matches the per-grain measure, but the test only checks the derivedrunningcolumn. Assertingorders.total_sum == [300.0, 200.0, 375.0]too would make this catch regressions in the base aggregation path, not just the cumulative transform.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/test_integration_duckdb.py` around lines 316 - 330, The monthly cumsum test only validates the derived running measure, so it can miss regressions in the underlying aggregation path. Update the DuckDB integration test around SlayerQuery execution to also assert the base monthly totals from the per-grain measure returned by duckdb_env.execute, using the existing orders.total_sum field alongside the current orders.running check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/concepts/models.md`:
- Line 97: Update the aggregation contract wording in the models documentation
and any related implementation so primary-key columns are restricted only to
count and count_distinct, removing count_distinct_approx from the allowed
primary-key set. Check the logic around the model aggregation rules that
reference count_distinct_approx, primary-key restrictions, and
allowed_aggregations so the documented behavior matches the repository rule and
any validation remains consistent with the same primary-key guard.
In `@docs/dbt/dbt_import.md`:
- Line 145: The intra-doc link in the dbt importer documentation points to an
anchor that does not match the heading slug generated by the “Clean-fail /
unsupported” section. Update the target in the affected markdown so it resolves
correctly, either by changing the heading in the section to a slash-free name or
by adding a matching explicit anchor, and make sure the link text and anchor
stay consistent with the section title.
In `@slayer/core/enums.py`:
- Around line 249-252: The PRIMARY_KEY_AGGREGATIONS allowlist in the enums
definition is out of sync with the primary-key restriction contract because it
includes count_distinct_approx. Update the PRIMARY_KEY_AGGREGATIONS symbol to
only permit count and count_distinct for primary-key columns, and then align any
related documentation, tests, or validation paths that reference the primary-key
aggregation rules so they match this contract consistently.
In `@slayer/dbt/converter.py`:
- Around line 1205-1221: The _filter_reachable check in converter.py is using
only local entity names and primary_entity, so it incorrectly rejects filters
that are reachable through multiple joins. Update _filter_reachable to use the
same join-graph reachability logic SLayer uses for cross-model references,
likely by traversing joins from source_sm instead of doing a one-hop entity-name
membership check. Keep the existing _DIMENSION_RE parsing and error reporting,
but base the reachable/not-reachable decision on transitive join-path resolution
so multi-hop Dimension('entity__dim') filters pass when they can be resolved.
- Around line 1238-1256: The filtered-measure rebuild in the converter is
dropping special measure semantics by reusing dbt_measure.expr with _map_agg, so
update the logic in the filtered measure path to preserve measure-specific
behavior for special aggregations like sum_boolean and percentile instead of
flattening them into a generic aggregate. Use the existing _convert_filter /
_filtered_columns flow in converter.py to locate the fix, and make sure the
returned expression carries the original dbt_measure configuration (for example
boolean wrapping or percentile parameters) rather than only column_expr plus
_map_agg(dbt_measure.agg).
In `@tests/test_count_distinct_approx_generation.py`:
- Around line 68-70: The primary-key aggregation allowlist is too permissive
because it includes count_distinct_approx, which contradicts the contract that
PK columns only support exact count and count_distinct. Update the test
expectations in test_allowed_on_primary_key_columns and any related
generation/assertions around PRIMARY_KEY_AGGREGATIONS to remove
count_distinct_approx and keep only the exact-count aggregations, and make sure
any docs or allowlist code tied to PRIMARY_KEY_AGGREGATIONS matches that
restriction.
---
Outside diff comments:
In `@slayer/cli.py`:
- Around line 1318-1323: The CLI is instantiating DbtToSlayerConverter without
the target dialect, so _maybe_dialect_caveat() never emits the new
percentile/median warnings during slayer import-dbt. Update the converter
construction in the CLI to pass the datasource dialect from args.datasource (or
the resolved datasource object) into the target_dialect field, using
DbtToSlayerConverter and _maybe_dialect_caveat as the key symbols to locate the
change.
In `@slayer/dbt/converter.py`:
- Around line 395-409: In the entity-column reuse branch, only primary_key is
being updated on the existing Column, so the entity’s label, description, meta,
and derived role information are lost compared with the synthetic-column path.
Update the existing-column handling in the converter logic to merge the same
entity metadata onto the matched Column instance in addition to setting
primary_key, preserving any existing column meta when present. Focus on the
branch that iterates over cols and matches c.name to col_name, and keep the
behavior consistent with the Column(...) construction used when the column is
newly appended.
- Around line 1352-1378: The _resolve_metric_to_name method in the metric
resolution path should clean-fail when an unfiltered simple metric’s backing
measure cannot be resolved instead of returning None and letting callers keep
the original metric name. Update the logic around _resolve_metric_to_name,
_resolve_measure_to_name, and the first simple-metric handling branch to detect
this unresolved case and route it through _fail_metric(...) immediately, so
derived and ratio formulas never reference a missing model symbol.
---
Nitpick comments:
In `@slayer/dbt/models.py`:
- Around line 28-68: Split _normalize_filter() into smaller helpers by
extracting the repeated clause-unwrapping logic from the dict and list/tuple
branches into a single small helper, then have _normalize_filter() normalize the
input to one iterable and build parts through that helper; keep the existing
behavior for strings, None, and fallback values while reducing duplication and
cognitive complexity.
In `@tests/integration/test_integration_duckdb.py`:
- Around line 316-330: The monthly cumsum test only validates the derived
running measure, so it can miss regressions in the underlying aggregation path.
Update the DuckDB integration test around SlayerQuery execution to also assert
the base monthly totals from the per-grain measure returned by
duckdb_env.execute, using the existing orders.total_sum field alongside the
current orders.running check.
In `@tests/test_filtered_count_forms.py`:
- Around line 45-57: Add a regression test in test_filtered_count_forms for the
new filtered count_distinct_approx path, using
_gen("cust:count_distinct_approx") and asserting the generated SQL places the
CASE filter inside the APPROX distinct count expression. This should
specifically exercise the new _build_approx_count_distinct() branch in
slayer/sql/generator.py and confirm its _wrap_filter(...) behavior matches the
existing count, count_distinct, and sum tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3927fc7e-de90-49f1-b1a0-7f497b05ed0d
📒 Files selected for processing (29)
.claude/skills/slayer-models.md.claude/skills/slayer-query.mdCLAUDE.mddocs/concepts/models.mddocs/concepts/terminology.mddocs/database-support.mddocs/dbt/dbt_import.mddocs/dbt/slayer_vs_dbt.mddocs/examples/07_aggregations/aggregations.mdslayer/cli.pyslayer/core/enums.pyslayer/dbt/converter.pyslayer/dbt/models.pyslayer/sql/dialects/_tier2.pyslayer/sql/dialects/base.pyslayer/sql/dialects/bigquery.pyslayer/sql/dialects/clickhouse.pyslayer/sql/dialects/duckdb.pyslayer/sql/dialects/snowflake.pyslayer/sql/dialects/tsql.pyslayer/sql/generator.pytests/dialects/test_count_distinct_approx.pytests/integration/test_integration_duckdb.pytests/test_count_distinct_approx_generation.pytests/test_dbt_converter.pytests/test_dbt_metricflow_strengthen.pytests/test_dbt_parser.pytests/test_enums.pytests/test_filtered_count_forms.py
… complexity
Codex + CodeRabbit:
- _filtered_leaf_ref preserves special measure semantics — filtered percentile
keeps p= (clean-fails on missing/discrete/approx), filtered sum_boolean builds
the CASE-WHEN INT column instead of collapsing to SUM of a raw boolean.
- Derived-metric per-input filters now push down into single-aggregate leaves
(clean-fail on multi-aggregate inputs or offset+filter), no longer silently
ignored.
- DbtMetricInput.offset_window accepts the DSI object form {count, granularity}.
- CLI passes the datasource dialect as target_dialect so percentile/median
caveats actually print at import time.
Sonar S3776 (cognitive complexity): extracted _derived_input_replacement /
_derived_filtered_input_ref, _cumulative_clean_fail, and _clause_to_str helpers.
Sonar S7503: NOSONAR on the async resolver stubs (must be coroutines).
Docs: fixed the broken #clean-fail-and-unsupported anchor. Tests: filtered
count_distinct_approx regression, DuckDB base-totals assertion, and coverage for
all the converter follow-ups.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
…xity CodeRabbit: - _filter_reachable now walks the join graph transitively (BFS over shared entities) so a legal multi-hop cross-model filter (orders → customers → regions) pushes down instead of clean-failing. Truly-unreachable entities still clean-fail. Added a multi-hop regression test. Sonar: - S3776 cognitive complexity: extracted _simple_metric_unsupported, _substitute_derived_inputs, and _cumulative_source_model so the three convert-metric entry points drop below the 15 threshold. - S1192: hoisted the repeated cross-model-filter suggestion to a module constant. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Extract _model_entity_names / _peer_model_names so the BFS helper's nested loops drop the function below the cognitive-complexity threshold. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tio input Two major correctness bugs Codex flagged on the updated diff: - _filter_reachable's transitive BFS (added last round) admitted multi-hop cross-model filters, but convert_dbt_filter only emits a one-hop `<model>.<dim>` path that does not resolve transitively (a two-hop filter needs the full `a__b.dim` join path). Reverted to the direct-join check and clean-fail multi-hop filters (full support is DEV-1445). Flipped the test to assert the clean-fail. - A derived/ratio input naming an unsupported simple metric (time-spine gap-fill / measure-less) resolved to a name that was never materialized, producing a formula referencing a nonexistent measure. Added _input_unmaterialized() and clean-fail that case in both the derived and ratio paths, with a regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… owner check Per the chosen design, replace the inline materialization guards with one robust validation pass: - _prune_dangling_measures: after all metrics convert, validate every emitted ModelMeasure formula with parse_formula against the model's measures; drop + report any whose bare references don't resolve, run to a fixpoint so a measure depending on a just-dropped one is dropped too. Subsumes the derived/ratio-input-references-clean-failed-metric case (measure-less, time-spine, unreachable filter, filtered-leaf failure, transitive drops). Removed the narrower _input_unmaterialized guard. - _filter_reachable: clean-fail a foreign-entity filter that has no joinable owner model (convert_dbt_filter would otherwise emit a bare invalid column). Local columns and one-hop joins still pass; undeclared dimensions stay valid (column existence is a query-time schema-drift concern). Split per-token into _entity_filter_reachable. Tests for both new clean-fail paths. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… filter _entity_filter_reachable now clean-fails a foreign-entity filter whose entity is owned as primary by more than one model. convert_dbt_filter qualifies the filter to a single (lexicographically-first) owner, so a multi-owner entity would lower to a possibly-wrong joined model — emit nothing rather than ambiguously-qualified SQL. Single-owner one-hop filters are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When a derived/ratio input adds its own filter on top of an already-filtered simple metric, the push-down resolved to the bare leaf and dropped the referenced metric's filter (silently widening results, e.g. region='US' lost, leaving only channel='web'). _resolve_input_to_leaf_filtered now accumulates filters along the resolution chain, and the derived/ratio push-down paths intersect that chain filter with the input/metric-level filter. Regression test asserts both filters survive on the leaf. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…h-down Two choke-point guards so the filter push-down can't drop unsupported semantics: - _filtered_leaf_spec clean-fails a non_additive_dimension (semi-additive) leaf, covering both the simple-metric-filter path and the derived/ratio push-down path (a semi-additive measure can't lower to a plain filtered SUM). - _resolve_input_to_leaf_filtered refuses time-spine / measure-less simple metrics, so a derived/ratio per-input filter over one clean-fails instead of resurrecting it as a plain aggregate. Regression tests for both. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…mple metrics _resolve_metric_to_name collapsed a simple metric to its unfiltered backing measure whenever metric.filter was absent, but a simple metric's filter can live on type_params.measure.filter instead — so a derived/ratio reference silently used the unfiltered base, widening results. Extracted _simple_metric_is_plain, which treats a simple metric as plain (collapsible to backing measure) only when it has no filter on EITHER side and no time-spine gap fill. Regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Extract _resolve_simple_metric_leaf and replace the metric loop with a lookup so both functions drop below the cognitive-complexity threshold. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
New docs/examples/10_dbt_metricflow walks the dbt-labs ACME Insurance benchmark through the converter end-to-end: pinned-SHA shallow clone, load CSVs into DuckDB, convert MetricFlow semantic_models + metrics into SLayer models, then answer two benchmark questions (a row count and a derived-over-filtered metric) and verify each against the gold SQL. - setup_metricflow.py: self-bootstrapping helper (atomic cached clone at a pinned commit, CSV load, conversion, gold-first fetch helper) - self-verifying notebook with inline gold-vs-SLayer asserts - test_notebooks.py: GitHub-reachability skip-guard for this notebook - mkdocs nav entry + gitignore for the example .cache/ Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/test_notebooks.py (1)
74-90: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winConvert structured bootstrap failures into a skip here.
This only skips the one case where
github.com:443is unreachable up front. The notebook bootstrap was also changed to raiseMetricFlowDemoErrorfor clone/setup failures, so environments with a reachable socket but a failinggit fetchwill still fail this test instead of skipping cleanly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/test_notebooks.py` around lines 74 - 90, Update test_notebook_runs_without_errors so it also treats MetricFlowDemoError from the notebook bootstrap as a skip, not just the explicit _github_reachable() false case. Wrap the notebook load/nbclient.NotebookClient.execute path in a try/except for MetricFlowDemoError and call pytest.skip with the failure message, keeping the existing notebook_path/_METRICFLOW_NB_DIR bootstrap check intact. Use the test_notebook_runs_without_errors function and MetricFlowDemoError symbol to locate the change.
🧹 Nitpick comments (1)
docs/examples/10_dbt_metricflow/setup_metricflow.py (1)
223-228: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse keyword arguments for the helper calls in this block.
These new multi-parameter calls are positional even though the repo’s Python style requires keyword arguments here. As per coding guidelines,
**/*.py: Use keyword arguments for functions with more than one parameter.Suggested fix
- load_csvs_into_duckdb(csv_dir, DB_PATH) - result = convert_dbt_to_slayer(dbt_path, MODELS_DIR, DB_PATH) + load_csvs_into_duckdb(csv_dir=csv_dir, db_path=DB_PATH) + result = convert_dbt_to_slayer( + dbt_project_path=dbt_path, + models_dir=MODELS_DIR, + db_path=DB_PATH, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/examples/10_dbt_metricflow/setup_metricflow.py` around lines 223 - 228, The helper calls in this block use positional arguments even though the Python style requires keyword arguments for multi-parameter functions. Update the calls around clone_dbt_project(), load_csvs_into_duckdb(), and convert_dbt_to_slayer() so any function with more than one parameter is invoked with explicit keyword names, keeping the existing control flow and variable usage unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/examples/10_dbt_metricflow/setup_metricflow.py`:
- Around line 120-124: Wrap all clone failures in MetricFlowDemoError, not just
subprocess.CalledProcessError. In setup_metricflow.py, update the clone/checkout
error handling around the git subprocess call so OSError and other unexpected
exceptions are caught alongside CalledProcessError, clean up tmp with
shutil.rmtree(ignore_errors=True), and raise MetricFlowDemoError with the same
repo/SHA context from the clone helper path. Keep the existing
MetricFlowDemoError message style used in the failure branch so notebooks/tests
can consistently recognize the demo failure.
- Around line 146-149: The SQL in the csv import step interpolates csv_file
directly into a single-quoted read_csv_auto argument, so paths containing
apostrophes will break execution. Update the code in the setup_metricflow.py
import block around conn.execute to properly escape or safely quote csv_file
before embedding it in SQL, ideally by using a SQL-parameter-safe approach or a
helper that produces a valid SQL string literal for the read_csv_auto call.
- Around line 219-239: The reuse check in setup_metricflow.py is missing
validation for the persisted model storage under MODELS_DIR, so the cached path
can be reused even when the stored Slayer models are absent or incomplete.
Update the reuse condition near the reuse/cached branch to include MODELS_DIR
existence and integrity alongside _COMPLETE_MARKER,
_checkout_is_valid(DBT_CHECKOUT), and DB_PATH.exists(), and ensure the non-reuse
path rebuilds the demo when that storage is missing or partially deleted before
creating the SlayerClient.
---
Outside diff comments:
In `@tests/integration/test_notebooks.py`:
- Around line 74-90: Update test_notebook_runs_without_errors so it also treats
MetricFlowDemoError from the notebook bootstrap as a skip, not just the explicit
_github_reachable() false case. Wrap the notebook
load/nbclient.NotebookClient.execute path in a try/except for
MetricFlowDemoError and call pytest.skip with the failure message, keeping the
existing notebook_path/_METRICFLOW_NB_DIR bootstrap check intact. Use the
test_notebook_runs_without_errors function and MetricFlowDemoError symbol to
locate the change.
---
Nitpick comments:
In `@docs/examples/10_dbt_metricflow/setup_metricflow.py`:
- Around line 223-228: The helper calls in this block use positional arguments
even though the Python style requires keyword arguments for multi-parameter
functions. Update the calls around clone_dbt_project(), load_csvs_into_duckdb(),
and convert_dbt_to_slayer() so any function with more than one parameter is
invoked with explicit keyword names, keeping the existing control flow and
variable usage unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2a2b2d42-927b-4738-84a7-82808650f565
📒 Files selected for processing (8)
.gitignoredocs/examples/10_dbt_metricflow/dbt_metricflow.mddocs/examples/10_dbt_metricflow/dbt_metricflow_nb.ipynbdocs/examples/10_dbt_metricflow/setup_metricflow.pymkdocs.ymlslayer/dbt/converter.pytests/integration/test_notebooks.pytests/test_dbt_metricflow_strengthen.py
✅ Files skipped from review due to trivial changes (2)
- mkdocs.yml
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_dbt_metricflow_strengthen.py
- slayer/dbt/converter.py
Review fixes from CodeRabbit / Sonar / Codex on the demo notebook, plus one converter correctness fix: - setup_metricflow.py: wrap missing-git (OSError) in MetricFlowDemoError; bind the CSV path as a DuckDB parameter (apostrophe-in-path safe); include MODELS_DIR in the cache-reuse check; keyword args for helper calls. - notebook: next(iter(...)) instead of list(...)[0] (Sonar S8519). - test_notebooks.py: key the offline skip on the .complete marker and skip on a mid-run MetricFlowDemoError when GitHub is unreachable (stale/partial cache or fetch failure). - converter.py: merge an entity's role/label/description/meta onto a reused PK column instead of dropping it (fill-if-blank; existing column values win), with a regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The prior round wrapped OSError in clone_dbt_project, but _checkout_is_valid (called first by both the clone and the reuse check) only caught CalledProcessError. A cached checkout with the git binary gone would raise a raw FileNotFoundError, bypassing MetricFlowDemoError and the notebook skip guard. Treat OSError as "not valid" so the caller falls through to the clone path's recognized error. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Conflicts resolved:
- slayer/dbt/{converter,models}.py and slayer/sql/dialects/{_tier2,duckdb,
snowflake}.py: main's only change was the "Modern typing" pass; the branch
is the functional superset. Kept the branch content and re-applied modern
typing (ruff UP) so main's f5f532a is preserved, not reverted.
- mkdocs.yml: kept both new example nav entries.
- Renamed the demo example 10_dbt_metricflow -> 11_dbt_metricflow to avoid
colliding with main's new 10_row_level_security; updated mkdocs, .gitignore,
and the test_notebooks skip-guard accordingly.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Conversion (funnel) metrics clean-fail, but their parsed details were being
lost: _fail_metric only stashed raw={"type": ...}, and the source-model
resolver never walked conversion_type_params, so the raw was dropped entirely.
Now _collect_metric_sources_from_params walks the base/conversion measures
(so a single-model funnel resolves its owner), and the raw payload carries the
full conversion_type_params — honoring the "nothing silently lost" contract,
consistent with the other clean-fail branches. Adds a regression test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Follow-ups on the conversion-metric meta stash: - Also resolve funnel source models expressed via base_metric/conversion_metric (not just base_measure/conversion_measure), so metric-ref funnels stash their raw and mixed measure+metric funnels don't mis-attribute to a single source. - Refactor _collect_metric_sources_from_params into small helpers (_add_measure_source / _add_conversion_sources) to bring cognitive complexity back under the limit (Sonar S3776). - Strengthen tests: a fully-resolved single-model funnel (both measures present) asserting the stash, plus a cross-model funnel asserting it is reported but not mis-stashed onto one model. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Restructure the MetricFlow demo into three explicit steps: (1) ensure_dbt_data clones the pinned project and loads CSVs into DuckDB, (2) convert_dbt_to_slayer runs the dbt->SLayer conversion in its own cell, (3) build a client and query. The notebook is committed with executed outputs. Splitting the bootstrap out of the conversion also means the converted models are always rebuilt fresh, so the cache only covers the network/data step. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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
1 participant
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.
Summary
Strengthens SLayer's dbt/MetricFlow importer (
slayer/dbt/) so every legal dbt-semantic-interfaces (DSI) construct that reaches the importer is either represented exactly or failed cleanly into a structured conversion report — no silent drops, no approximate/lossy SQL.What changed
count_distinct_approx— dialect-aware aggregation (Part 2)approx_count_distinct/countdistinctapprox.SqlDialect.build_approx_count_distinctemits the DB-native approximate-distinct where one exists (approx_count_distinct,uniq,APPROX_COUNT_DISTINCT,approx_distinct,APPROXIMATE COUNT(DISTINCT)) and falls back to exactCOUNT(DISTINCT)where it doesn't (Postgres/SQLite/MySQL) — exact is more accurate, consistent with the no-approximate-SQL rule.Importer correctness + represent-exactly (Parts 1, 3)
p=value (+ clean-fail on missing value / discrete / approximate flags); ratio denominatornullifguard.sum_boolean→CASE WHEN (<expr>) THEN 1 ELSE 0 ENDINT column;offset_window→time_shift(single-aggregate inputs only, plural grains normalized).(model, expr, filter)dedup and cross-model join reachability; string-or-list filter intersection normalization.Parser completeness (Part 5): cumulative / conversion / metric-aggregation type-params, time-window parsing,
measureas string-or-object,join_to_timespine/fill_nulls_with,config.meta.extra="ignore"kept for DSI forward-compat.Clean-fail routing + report (Parts 4, 5b, 6):
ConversionWarninggainscategory/severity/suggestion;ConversionResult.render_report()groups by category; dropped constructs are stashed into the owning entity'smeta;target_dialectpercentile/median caveat; CLI prints the grouped report +N models, M unconverted, K droppedtally.Docs (Part 7): supported-mappings + clean-fail tables in
dbt_import.md/slayer_vs_dbt.md, pluscount_distinct_approxacrossdatabase-support.md, aggregation/model/terminology docs,CLAUDE.md, and the query/model skills.Testing
ruffclean.🤖 Generated with Claude Code
Summary by CodeRabbit
count_distinct_approxbuilt-in aggregation with dialect-aware SQL and exactCOUNT(DISTINCT)fallback.nullif(..., 0).count_distinct_approx, plus dbt conversion/filter/cumulative/offset regression tests.