fix(tesseract): project a primary key once in the keys subquery

[waralexrom]

Summary

Fixes #11455 : a query that asks for a cube's primary_key as a dimension together with measures from 2+ cubes fails on Postgres with column reference "a__id" is ambiguous. Regression against 1.6.x, and against the legacy planner, which dedupes the same projection.

Changes

• keys_sub_query.rs: project a primary key once when it is also a query dimension. A measure multiplied by a fan-out join is read through the keys subquery — a DISTINCT grid of the query dimensions plus the primary key of the measure's cube, re-joined to that cube by the key. The key was projected unconditionally next to the query dimensions, so asking for it as a dimension put two columns under one alias and made the re-join's keys.<pk> reference ambiguous.
• The re-join still resolves to the surviving column: it looks the column up by member symbol (Schema::find_column_for_member), not by alias, so a key projected under a dimension's alias — a view or reference dimension — is found too. The dedup uses that same symbol comparison.
• Regression tests: tests::integration::multi_fact::test_multiplied_aggregate_grouped_by_own_primary_key (planner, against Postgres) and packages/cubejs-schema-compiler/test/integration/postgres/primary-key-multi-fact.test.ts (the reported model, verifying the multiplied measure is not double-counted).

Before / after

The keys subquery for the multiplied measure, before:

FROM (SELECT DISTINCT
        "a_key_a".id "a__id",        -- query dimension
        "a_key_b".date "b__date",
        "a_key_a".id "a__id"         -- re-join key, same alias
      FROM ...) AS "keys"
LEFT JOIN cube_a AS "a_key_a" ON ("keys"."a__id" = "a_key_a".id)   -- ambiguous

after:

FROM (SELECT DISTINCT
        "a_key_a".id "a__id",        -- both roles
        "a_key_b".date "b__date"
      FROM ...) AS "keys"
LEFT JOIN cube_a AS "a_key_a" ON ("keys"."a__id" = "a_key_a".id)

Testing

• Planner suite against live Postgres: cargo test --features integration-postgres — 1207 passed, 0 failed, no snapshot drift.
• Both regression tests confirmed to catch the bug: with the dedup reverted and the native addon rebuilt, the schema-compiler test fails with the reporter's exact column reference "a__id" is ambiguous.
• The schema-compiler test passes under CUBEJS_TESSERACT_SQL_PLANNER=true and =false — the legacy planner already dedupes this in BaseQuery.keyDimensions, so the fix restores parity.
• Related schema-compiler suites under Tesseract (sql-generation, multi-fact-join, calculated-measure-multi-fact, sub-query-dimensions, views, multi-stage): 147 passed, 1 skipped.

Note: the planner test only guards the bug under --features integration-postgres — build_sql() itself succeeds, the failure comes from Postgres. That matches the existing pattern in that suite.

🤖 Generated with Claude Code

[claude]

Claude finished @waralexrom's task in 4m 27s —— View job

---

Review: looks correct and well-scoped — 0 blocking, 2 non-blocking (1 maintainability, 1 test-coverage)

Full review

• Read the PR diff (keys_sub_query.rs, both tests, snapshot)
• Traced the re-join consumer (aggregate_multiplied_subquery.rs) to check the key still resolves
• Checked Schema::find_column_for_member / MemberSymbol::resolve_reference_chain / PartialEq semantics
• Checked reference-dimension and time-dimension edge cases
• Checked whether the new tests actually run in CI
• Posted inline comments

Verdict

The fix is minimal and lands in the right place. The duplicate projection was pure dead weight: ReferencesBuilder::resolve_references_for_member early-returns on a full_name already in the map (references_builder.rs:52-56), so the second call added nothing but the extra AliasedExpr — which is exactly the ambiguity. Skipping it drops nothing that the enclosing plan needs.

Things I checked that hold up:

• The re-join still resolves. aggregate_multiplied_subquery.rs:121 and :156 use keys_query.schema().resolve_member_alias(dim), which goes through find_column_for_member — symbol-based, not alias-based. In the view/reference case the surviving column carries the view's alias and its origin_member resolves to the same base, so the lookup finds it. Worth noting this branch was already the effective behavior pre-fix: find_column_for_member returns the first match and dimensions are projected before primary keys, so the ON clause already pointed at the dimension column and the pk column was unreferenced. The change removes dead SQL there rather than altering semantics — which is consistent with the reported "no snapshot drift".
• Not over-broad. MemberSymbol::eq is full_name + discriminant, so a dimension whose SQL merely contains the pk ({cube_a.id} + 0, is_reference == false) doesn't resolve to the pk and isn't deduped. Only genuine reference chains collapse — which is what you want.
• Time dimensions don't get caught. TimeDimensionSymbol::new always appends a granularity suffix (defaulting to "day", time_dimension_symbol.rs:66-79), so a time-typed pk in the query gets alias a__id_day, never colliding with a__id. No residual ambiguity hiding behind the discriminant check.
• No render-reference loss. resolve_references_for_member recurses into get_dependencies(), so in the reference case the base pk's render reference is still populated via the referencing dimension; in the identical-symbol case the dimension loop already populated it.
• dimensions_query path unaffected — it returns the keys query before any re-join (aggregate_multiplied_subquery.rs:77).
• Nothing security- or performance-relevant here: no user input handling, and the dedup is O(pk × dims) over a handful of symbols. The generated SQL gets strictly smaller.

Two non-blocking points, both posted inline:

#	File	Severity	Point
1	keys_sub_query.rs:95-102	low (maintainability)	The dedup hand-rolls find_column_for_member's matching rule against the logical schema. SelectBuilder.result_schema already holds every projected column with its origin_member — querying that (via a small accessor) would keep one matching rule instead of two that can drift.
2	multi_fact.rs:622	low (test coverage)	The Rust regression test can't fail in CI: unit-core runs cargo test without --features integration-postgres (push.yml:233), so only build_sql().unwrap() executes and that succeeds with the bug present. The schema-compiler test does run in CI for postgres under both planner flags, so the fix isn't unguarded — but a common_sql_generation.rs-style SQL snapshot would pin the shape without a database and guard the changed line directly.

Nice touches: the code comment explains why the skip is safe (the symbol-matching invariant with the re-join), not just what it does; the TS fixture asserts the measure isn't double-counted rather than only that the query doesn't throw; and the reproduction was verified to fail with the fix reverted.

· branch `tesseract-fix-primary-key-multisource`

[claude]

Correctness looks right, but this re-implements Schema::find_column_for_member's matching rule (origin_member.resolve_reference_chain() == member.resolve_reference_chain()) against the logical schema instead of asking the physical schema that's actually being built. The comment acknowledges the coupling — the risk is that the two rules drift: if find_column_for_member ever changes (e.g. to also compare granularity or discriminant explicitly), this dedup silently stops matching it and the ambiguity comes back, with no test in default CI to catch it (see the other comment).

SelectBuilder already accumulates result_schema and add_projection_member records SchemaColumn::new(alias, Some(member)) for every projection above, so a small accessor would let this be expressed in terms of the single source of truth:

if select_builder.result_schema().find_column_for_member(member).is_some() {
    continue;
}

That also covers columns added through paths other than the all_dimensions() loop. Non-blocking — just a suggestion to keep one matching rule instead of two.

[claude]

This test can't fail in CI. unit-core in .github/workflows/push.yml:233 runs cargo test --workspace --features cubesqlplanner/integration-cubestore — no integration-postgres — so try_execute_pg returns None and only build_sql(query).unwrap() runs, which succeeds with the bug present (the failure is Postgres-side). The PR description notes this, and it does match the surrounding suite, so it's not a blocker; the schema-compiler test does run in CI (integration:postgres, and for postgres the matrix covers both CUBEJS_TESSERACT_SQL_PLANNER true and false), so the fix isn't unguarded.

Still, the defect here is purely a SQL-shape one and is cheap to pin without a database — tests/common_sql_generation.rs has the pattern (insta::assert_snapshot! over build_sql). A snapshot showing the keys subquery projecting "a__id" once would make unit-core catch a regression directly on the changed line, and would also catch drift between this dedup and find_column_for_member.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.96%. Comparing base (e090594) to head (bca2bcc).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #11527      +/-   ##
==========================================
+ Coverage   79.46%   83.96%   +4.49%     
==========================================
  Files         480      257     -223     
  Lines       98861    80919   -17942     
  Branches     3636        0    -3636     
==========================================
- Hits        78563    67940   -10623     
+ Misses      19778    12979    -6799     
+ Partials      520        0     -520     

Flag	Coverage Δ
cube-backend	?
cubesql	83.96% <ø> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.