DEV-1339: regression tests for multi-hop derived column qualification#93
Conversation
DEV-1339 reported that querying a derived ``Column.sql`` reached via a multi-hop join path leaves the inner bare base-column refs unqualified in emitted SQL. Investigation showed the bug is already fixed by PR #89 (DEV-1333) — running the issue's reproduction now produces correctly qualified SQL across all entry points: dim refs, cross-model measure aggregations, filters, time dimensions, and diamond join paths. Land six regression tests pinning the fixed behavior so the qualification contract can't silently regress: multi-hop dim, multi-hop cross-model measure, multi-hop filter, multi-hop time-dim, the verbatim DEV-1339 solar_panels repro (adapted from the issue's hypothetical alias/on fields to the real target_model/join_pairs API), and a diamond-path cross-model measure that asserts the non-queried arm of the diamond never leaks into the measure CTE. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ev-1339-multi-hop-alias-path-doesnt-qualify-joined-model-derived
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds regression tests and a test fixture validating recursive inlining and canonical alias qualification of cross-model derived Column.sql across multi-hop joins (A→B→C), including dimension selection, measures, filters, time-dim enrichment, a solar-panels repro, and a diamond-join case. ChangesMulti-Hop Derived Column Test Suite
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
🧹 Nitpick comments (3)
tests/test_cross_model_derived_columns.py (3)
650-905: ⚡ Quick winAdd one parse-level assertion for a new multi-hop scenario.
These regressions mostly validate aliasing with substring checks. A malformed query shape could still satisfy those assertions, so extending the existing
sqlglot.parse(...)sanity check to one representative multi-hop measure/time-dimension path would tighten the safety net for DEV-1339.🤖 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_cross_model_derived_columns.py` around lines 650 - 905, Add a sql parsing sanity check to one representative multi-hop test to catch malformed SQL that might pass substring checks: e.g. in test_multihop_cross_model_measure_over_derived_target (after generating sql via _gen_sql and normalizing to norm) call sqlglot.parse(sql) or sqlglot.parse_one(norm) to assert the SQL is syntactically valid; alternatively do the same in test_multihop_time_dim_to_derived_target_column after sql/norm is produced. Ensure you import sqlglot at top of the test file if missing and place the parse assertion before the other substring/assert checks so parse failures fail early.
659-659: ⚡ Quick winUse keyword arguments for the new
_gen_sqlcalls.These new cases pass
engine,query, andmodelpositionally, which breaks the repo’s Python calling convention and makes the setup blocks harder to scan.As per coding guidelines "Use keyword arguments for functions with more than 1 parameter".
Also applies to: 686-686, 714-714, 774-774, 818-823, 829-834, 891-891
🤖 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_cross_model_derived_columns.py` at line 659, Several tests call _gen_sql with positional args (e.g., _gen_sql(engine, query, model_a)); update those calls to use keyword arguments to follow the project's convention: call _gen_sql(engine=engine, query=query, model=model_a) (or model=model_b as appropriate). Replace every positional invocation of _gen_sql in this file (including the instances flagged around the noted ranges) so the parameters are passed as keywords: engine=..., query=..., model=.... This applies to all occurrences (the call at the shown line and the other listed occurrences).
730-731: ⚡ Quick winMove these imports to module scope.
TimeGranularityandTimeDimensionare static dependencies of this test module, so keeping them inside the test body works against the repo’s import convention.As per coding guidelines "Place imports at the top of files".
🤖 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_cross_model_derived_columns.py` around lines 730 - 731, Move the two local imports out of the test body and place them at module scope at the top of the test module: import TimeGranularity from slayer.core.enums and TimeDimension from slayer.core.query so they are module-level imports; update any existing duplicate imports to avoid redundancy and run tests to ensure no import-time side effects.
🤖 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.
Nitpick comments:
In `@tests/test_cross_model_derived_columns.py`:
- Around line 650-905: Add a sql parsing sanity check to one representative
multi-hop test to catch malformed SQL that might pass substring checks: e.g. in
test_multihop_cross_model_measure_over_derived_target (after generating sql via
_gen_sql and normalizing to norm) call sqlglot.parse(sql) or
sqlglot.parse_one(norm) to assert the SQL is syntactically valid; alternatively
do the same in test_multihop_time_dim_to_derived_target_column after sql/norm is
produced. Ensure you import sqlglot at top of the test file if missing and place
the parse assertion before the other substring/assert checks so parse failures
fail early.
- Line 659: Several tests call _gen_sql with positional args (e.g.,
_gen_sql(engine, query, model_a)); update those calls to use keyword arguments
to follow the project's convention: call _gen_sql(engine=engine, query=query,
model=model_a) (or model=model_b as appropriate). Replace every positional
invocation of _gen_sql in this file (including the instances flagged around the
noted ranges) so the parameters are passed as keywords: engine=..., query=...,
model=.... This applies to all occurrences (the call at the shown line and the
other listed occurrences).
- Around line 730-731: Move the two local imports out of the test body and place
them at module scope at the top of the test module: import TimeGranularity from
slayer.core.enums and TimeDimension from slayer.core.query so they are
module-level imports; update any existing duplicate imports to avoid redundancy
and run tests to ensure no import-time side effects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5349daaa-af36-4691-a1bf-eac440c7715b
📒 Files selected for processing (1)
tests/test_cross_model_derived_columns.py
- Parameterise ``_save_a_b_c`` with a ``c_columns`` kwarg so ``test_multihop_time_dim_to_derived_target_column`` can reuse the A/B scaffold instead of redefining all three models inline. Eliminates the 22-line duplicate block flagged by the SonarCloud quality gate (drops new-duplicated-lines density below the 3% threshold). - Add a ``sqlglot.parse(...)`` sanity assertion at the head of ``test_multihop_cross_model_measure_over_derived_target`` so structurally broken SQL short-circuits before the substring matchers — addresses the CodeRabbit review-summary nitpick on the prior commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_cross_model_derived_columns.py (1)
740-741: 💤 Low valueConsider moving imports to the top of the file.
These imports are placed inside the test function. Per coding guidelines, imports should be at the top of files. However, this is common in tests for isolation when only one test needs specific imports.
🤖 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_cross_model_derived_columns.py` around lines 740 - 741, Move the local imports for TimeGranularity and TimeDimension out of the test body and place them at the module top so they follow the project's import guidelines; find usages of TimeGranularity and TimeDimension in tests/test_cross_model_derived_columns.py and replace the in-test "from slayer.core.enums import TimeGranularity" and "from slayer.core.query import TimeDimension" with module-level imports at the top of the file, ensuring no name conflicts and keeping test isolation intact if any setup previously depended on the late import.
🤖 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.
Nitpick comments:
In `@tests/test_cross_model_derived_columns.py`:
- Around line 740-741: Move the local imports for TimeGranularity and
TimeDimension out of the test body and place them at the module top so they
follow the project's import guidelines; find usages of TimeGranularity and
TimeDimension in tests/test_cross_model_derived_columns.py and replace the
in-test "from slayer.core.enums import TimeGranularity" and "from
slayer.core.query import TimeDimension" with module-level imports at the top of
the file, ensuring no name conflicts and keeping test isolation intact if any
setup previously depended on the late import.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 831f6488-34f8-4e63-be20-69548655b80e
📒 Files selected for processing (1)
tests/test_cross_model_derived_columns.py
Per the project import-style rule ("place all imports at the top of the
file") and the CodeRabbit nitpick on PR #93, lift the in-function
imports out of ``test_multihop_time_dim_to_derived_target_column``.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
1 participant
Summary
DEV-1339 reported that derived columns reached via a multi-hop join path were emitted with unqualified inner refs. Investigation showed the bug is already fixed by PR #89 (DEV-1333) — the issue's exact reproduction (adapted to the real
target_model/join_pairsAPI; the original used hypotheticalalias/onfields that don't exist onModelJoin) now produces correctly qualified SQL across every entry point.This PR lands six regression tests in
tests/test_cross_model_derived_columns.pypinning the fixed behavior so the contract can't silently regress:solar_panelsreproNo production code changes — the existing
expand_derived_refsmachinery inslayer/engine/column_expansion.pyalready handles every scenario the issue describes.Test plan
poetry run pytest tests/test_cross_model_derived_columns.py— 24/24 passpoetry run pytest -m "not integration"— 1591/1591 passpoetry run ruff check slayer/ tests/— cleanCloses DEV-1339.
🤖 Generated with Claude Code
Summary by CodeRabbit