HIVE-29580: CBO: Ambiguous column reference not detected in some CTE/CTAS/other queries

[konstantinb]

What changes were proposed in this pull request?

HIVE-29580: under CBO, detect ambiguous by-name references to duplicate-named columns that
escape a subquery/CTE boundary, instead of silently binding to an arbitrary candidate.

Mechanism. When the outer RowResolver is built at a subquery boundary and a duplicate alias
collides (CalcitePlanner block 9), the surviving ColumnInfo is marked (ambiguousName); the
duplicate remains bound under its internal name, so positional use keeps working. A later
by-name reference through the boundary alias fails with the existing
AMBIGUOUS_COLUMN (Error 10007). Check sites: TypeCheckProcFactory (qualified and unqualified
column resolution) and JoinCondTypeCheckProcFactory (both join-condition processors). The
marker rides every projection that rebuilds ColumnInfos — genColListRegex (both branches),
the DISTINCT + windowing group-by rebuild, and the post-group-by projection — and is excluded
from ColumnInfo.equals/hashCode/isSameColumnForRR. An explicit subquery/CTE column list
clears an inherited marker (the list assigns fresh unique names); a collision with an unlisted
column re-marks through the ordinary block-9 path.

Hive-internal SQL generators fixed along the way (their own output tripped the check —
both were caught by precommit on earlier iterations of this PR and fixed here):

• MergeRewriter emitted the target's partition columns twice into the rewritten MERGE source
  projection. It now branches per target type: appendAllColsOfTargetTable for non-native
  targets (their partition columns are ordinary data columns) and
  appendNonPartitionColsOfTargetTable for native ones (their partition columns were already
  emitted with the ACID select columns). No plan/golden churn: the optimizer already pruned the
  duplicates before EXPLAIN.
• The UNION DISTINCT rewrite (SELECT DISTINCT * over an internal alias) synthesizes one
  group-by reference per RowResolver entry — unique by construction — so the markers are
  cleared on that rewrite-private projection (genColListRegex copies; the subquery's own
  RowResolver keeps its markers) and re-applied to the group-by output once the plan is
  built. Outer by-name references over a DISTINCT or UNION DISTINCT output therefore still
  reject, including from HAVING and outer ORDER BY (positional ORDER BY 1 keeps compiling).

Why are the changes needed?

This completes the reference-time model documented in HIVE-20215. Its author noted in 2018 that
a query of exactly this shape "compile[s] successfully but it should throw an error since
reference to t.c1 is ambiguous" and expected HIVE-19770 to fix it. HIVE-19770 shipped the
tolerance (duplicate renamed to its internal name at the boundary) and a reference-time guard,
but the rename destroys the ambiguity information at exactly the boundary the guard needs it,
so the promised rejection never fired.

The silent binding is a wrong-results hazard, not a cosmetic one: limit_join_transpose.q
contained a derived table joining on value whose two escaped key columns hold different
data — whatever src2.key returned was an arbitrary pick between unequal columns. A CTAS over
such a reference silently persists the arbitrary choice into a table. Standard SQL
(e.g. PostgreSQL) rejects these references outright.

Design notes and deliberate choices

• Name collision is the only criterion. It is decidable and stable across planner phases.
  Semantic equality of the candidates is deliberately not considered: it is undecidable in
  general and unstable (constant folding can flip it). This means a reference whose candidates
  are provably equal via a join predicate (cross_prod_3.q's on A.key = B.key) is still
  rejected — PostgreSQL rejects the same shape for the same reason.
• Unreferenced duplicates stay tolerated (star expansion, positional use, set operations),
  preserving HIVE-19770's behavior. Pinned by the *_tolerated tests, including master's own
  pre-existing clientpositive/ambiguous_col.q, which is untouched.
• Non-CBO paths are untouched. CBO/non-CBO parity is explicitly not a goal;
  HIVE-19770/HIVE-20215 deliberately diverged the planners in 2018.
  ambiguous_col_noncbo_baseline.q documents the non-CBO baseline;
  ambiguous_col_unreferenced_tolerated.q is its CBO mirror, so the divergence reads
  shape-by-shape across the pair.
• The 2018 RowResolver guard remains and still covers same-block collisions (e.g. lateral
  view alias reuse — ambiguous_col_lateral_view_alias.q is its only coverage). For
  boundary shapes the marker now fires earlier, which is why the four pre-existing negative
  goldens (ambiguous_col.q, ambiguous_col_2.q, cbo_ambiguous_colref_in_gby.q,
  cte_col_alias_clash.q) change message format: same rejection, now with the 10007 error code.
• A CTAS-specific duplicate-column check from earlier iterations was removed: it regressed
  CTE materialization (Hive-generated CTAS). The reference-time case is still covered by the
  marker (ambiguous_col_ctas.q — kept as a guard that a user CTAS is never exempted should
  an authorship-based exemption ever be added). Rejecting duplicate definitions in CTAS
  remains open under HIVE-12825/HIVE-18568.
• No config gate. The check ships ungated; a flag is a one-line addition at the throw site
  if reviewers prefer one.
• Remaining residual holes fall back to master-equivalent silent binding — the patch never
  behaves worse than master. The largest previously documented hole (marker loss through
  DISTINCT-star and UNION DISTINCT boundaries) is closed by the re-marking described above.

Post-review updates

• Both Copilot findings were confirmed by probes and are fixed: an inherited marker surviving a
  positional CTE/subquery column list (a regression vs master — now cleared in the column-list
  branch), and marker loss through the UNION DISTINCT clear (master-equivalent — now
  re-applied to the group-by output, which also closes the shape for user-written
  select distinct *).
• Review-driven refactors: block 9 reuses the RowResolver lookup result; the misleading
  appendNonPartitionColsOfTargetTable mode branch was dissolved into MergeRewriter (the
  method now does exactly what its name says); checkAmbiguousName and ambiguousName carry
  javadocs stating the call contract and the equals/hashCode exclusion rationale.
• Return-path invariance (hive.cbo.returnpath.hiveop=true) is pinned by a dedicated unit test;
  retpath .q tests were evaluated and rejected — execution probes hit pre-existing
  retpath-only defects that make stable goldens impossible, so the unit pin owns this surface.

Does this PR introduce any user-facing change?

Yes (backward incompatible): queries that reference a duplicate-named column through a
subquery/CTE boundary by name now fail to compile under CBO with Error 10007. This includes
references over DISTINCT and UNION DISTINCT outputs of such columns, also from HAVING and outer
ORDER BY. Two existing tests relied on such references and were updated with explicit column
aliases, preserving their plans and result data: cross_prod_3.q (candidates provably equal —
the harmless instance) and limit_join_transpose.q (candidates genuinely unequal — the
wrong-results instance). The JIRA should carry the incompatible-change label.

How was this patch tested?

• Rejected shapes: ambiguous_col_rejected.q (CBO) and ambiguous_col_noncbo_baseline.q
  (non-CBO baseline), consolidated in the hive.cli.errors.ignore format (resourceplan.q
  precedent) so each file reads as a boundary spec with one FAILED line per statement; now also
  covers the DISTINCT-star wrapper, the UNION DISTINCT boundary, and the unlisted-column
  re-collision behind a CTE column list.
• Mechanism/role-specific clientnegative tests: join-condition checks (qualified and
  unqualified), unqualified reference site, USING-clause expansion, DISTINCT + windowing
  (marker must survive the windowing projection), lateral view alias reuse (RowResolver guard),
  CTAS persistence case.
• Tolerated shapes: ambiguous_col_tolerated.q (now also pins that an explicit CTE column
  list legalizes the reference), ambiguous_col_unreferenced_tolerated.q,
  ambiguous_col_union_distinct_tolerated.q (pins the UNION DISTINCT fix value-sensitively:
  branches differing only in the duplicate-named column must not collapse).
• Unit tests: TestColumnInfo (marker plumbing), TestTypeCheckProcFactory (check helper;
  file refactored to the Enclosed runner to host parameter-free tests alongside the existing
  parameterized ones), TestJoinCondTypeCheckProcFactory (driven through the production walker
  entry point), TestSemanticAnalyzer (compile-level CBO analysis: rejection/tolerance shapes,
  direct genColListRegex propagation, the DISTINCT/UNION DISTINCT closures, HAVING message
  shape, and return-path invariance), TestMultiInsertSqlGenerator (MERGE projection).
• Mutation-verified: removing any marker/propagation/check line fails a specific named
  test — including the group-by re-mark and its alias copy. Of the 16 unit tests measured
  against pre-patch master, 13 fail without this patch (the other 3 are negative controls that
  pin the check against over-reach); the later review-driven tests pin the closures above.
• MERGE paths: TestDbTxnManager2/TestTxnCommands2 variants and the MERGE qtests run green
  with zero golden churn.

[konstantinb]

@kasakrisz @zabetak @deniskuzZ @soumyakanti3578 Could one of you take a look when you have a chance? This completes the reference-time ambiguity model from HIVE-19770/HIVE-20215 (the original author and reviewer haven't contributed to the project in several years, so I'm asking the current owners of this area instead).

Why you specifically: @kasakrisz and @zabetak — the marker lives in CalcitePlanner/RowResolver boundary handling with check sites in TypeCheckProcFactory/JoinCondTypeCheckProcFactory, and the change interacts with CTE materialization (an earlier CTAS-level check was dropped precisely because it regressed Hive-generated CTAS — context from #6423). @deniskuzZ — one of the internal-SQL fixes touches the MERGE rewrite projection in MergeRewriter/MultiInsertSqlGenerator. @soumyakanti3578 — this is the same family as HIVE-29612/HIVE-28280: shapes where CBO's RowResolver handling diverges at a query-block boundary.

CI is green across all five checks, and SonarQube passed. Note that this is a backward-incompatible change (by-name references to duplicate-named columns through a subquery/CTE boundary now fail with Error 10007 under CBO); HIVE-29580 carries the incompatible-change flag accordingly.

[soumyakanti3578]

@konstantinb I will try to review it today 👍🏼

[Copilot]

Pull request overview

Adds CBO detection for ambiguous duplicate column names crossing CTE or subquery boundaries.

Changes:

• Tracks and validates ambiguous column-name metadata across planner projections.
• Corrects generated MERGE projections for partition columns.
• Adds extensive unit and query tests and updates affected fixtures.

Reviewed changes

Copilot reviewed 44 out of 44 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
ql/src/java/org/apache/hadoop/hive/ql/exec/ColumnInfo.java	Adds ambiguity metadata.
ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java	Marks and propagates ambiguous names.
ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java	Propagates markers through star expansion.
ql/src/java/org/apache/hadoop/hive/ql/parse/type/TypeCheckProcFactory.java	Rejects ambiguous references.
ql/src/java/org/apache/hadoop/hive/ql/parse/type/JoinCondTypeCheckProcFactory.java	Checks join-condition references.
ql/src/java/org/apache/hadoop/hive/ql/parse/rewrite/MergeRewriter.java	Avoids duplicate partition projections.
ql/src/java/org/apache/hadoop/hive/ql/parse/rewrite/sql/MultiInsertSqlGenerator.java	Adds non-partition column generation.
ql/src/test/org/apache/hadoop/hive/ql/exec/TestColumnInfo.java	Tests marker defaults and copying.
ql/src/test/org/apache/hadoop/hive/ql/parse/TestSemanticAnalyzer.java	Tests CBO behavior and propagation.
ql/src/test/org/apache/hadoop/hive/ql/parse/type/TestTypeCheckProcFactory.java	Tests ambiguity validation.
ql/src/test/org/apache/hadoop/hive/ql/parse/type/TestJoinCondTypeCheckProcFactory.java	Tests join-condition validation.
ql/src/test/org/apache/hadoop/hive/ql/parse/rewrite/sql/TestMultiInsertSqlGenerator.java	Tests MERGE projection generation.
ql/src/test/queries/clientpositive/limit_join_transpose.q	Disambiguates legacy join output.
ql/src/test/queries/clientpositive/cross_prod_3.q	Disambiguates self-join output.
ql/src/test/queries/clientpositive/ambiguous_col_tolerated.q	Covers tolerated duplicates.
ql/src/test/queries/clientpositive/ambiguous_col_unreferenced_tolerated.q	Covers unreferenced duplicates.
ql/src/test/queries/clientpositive/ambiguous_col_union_distinct_tolerated.q	Covers UNION DISTINCT behavior.
ql/src/test/queries/clientpositive/ambiguous_col_rejected.q	Covers rejected CBO references.
ql/src/test/queries/clientpositive/ambiguous_col_noncbo_baseline.q	Documents non-CBO behavior.
ql/src/test/queries/clientnegative/ambiguous_col_unqualified_ref.q	Tests unqualified references.
ql/src/test/queries/clientnegative/ambiguous_col_lateral_view_alias.q	Tests lateral-view collisions.
ql/src/test/queries/clientnegative/ambiguous_col_join_using.q	Tests USING joins.
ql/src/test/queries/clientnegative/ambiguous_col_join_cond.q	Tests qualified join conditions.
ql/src/test/queries/clientnegative/ambiguous_col_join_cond_unqual.q	Tests unqualified join conditions.
ql/src/test/queries/clientnegative/ambiguous_col_distinct_window.q	Tests windowing propagation.
ql/src/test/queries/clientnegative/ambiguous_col_ctas.q	Tests CTAS rejection.
ql/src/test/results/clientpositive/llap/limit_join_transpose.q.out	Updates expected query text.
ql/src/test/results/clientpositive/llap/cross_prod_3.q.out	Updates expected query text.
ql/src/test/results/clientpositive/llap/ambiguous_col_tolerated.q.out	Records tolerated results.
ql/src/test/results/clientpositive/llap/ambiguous_col_unreferenced_tolerated.q.out	Records unreferenced results.
ql/src/test/results/clientpositive/llap/ambiguous_col_union_distinct_tolerated.q.out	Records DISTINCT results.
ql/src/test/results/clientpositive/llap/ambiguous_col_rejected.q.out	Records CBO errors.
ql/src/test/results/clientpositive/llap/ambiguous_col_noncbo_baseline.q.out	Records non-CBO behavior.
ql/src/test/results/clientnegative/cte_col_alias_clash.q.out	Updates ambiguity error.
ql/src/test/results/clientnegative/cbo_ambiguous_colref_in_gby.q.out	Updates group-by error.
ql/src/test/results/clientnegative/ambiguous_col.q.out	Updates ambiguity error.
ql/src/test/results/clientnegative/ambiguous_col_2.q.out	Updates ambiguity error.
ql/src/test/results/clientnegative/ambiguous_col_unqualified_ref.q.out	Records unqualified error.
ql/src/test/results/clientnegative/ambiguous_col_lateral_view_alias.q.out	Records lateral-view error.
ql/src/test/results/clientnegative/ambiguous_col_join_using.q.out	Records USING error.
ql/src/test/results/clientnegative/ambiguous_col_join_cond.q.out	Records qualified join error.
ql/src/test/results/clientnegative/ambiguous_col_join_cond_unqual.q.out	Records unqualified join error.
ql/src/test/results/clientnegative/ambiguous_col_distinct_window.q.out	Records windowing error.
ql/src/test/results/clientnegative/ambiguous_col_ctas.q.out	Records CTAS error.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[konstantinb]

Confirmed — and follow-up probes showed it broader than user-written select distinct *: every UNION DISTINCT boundary lost the markers the same way. Fixed: the clear now records the marked positions and re-marks the group-by output RowResolver after the plan is built (plus marker propagation through the post-group-by projection, where the re-mark alone proved insufficient). Outer references over a DISTINCT/UNION DISTINCT output now reject — pinned by new TestSemanticAnalyzer tests and the c in x / c in z statements in ambiguous_col_rejected.q.

[konstantinb]

Confirmed — this one was a genuine regression vs master. Fixed by clearing the inherited marker in the positional column-list branch (the list assigns fresh unique names). Safe on both edges: duplicate names inside the list are already rejected by processTableColumnNames, and a collision with an unlisted column re-marks via the existing branch, since the list is a prefix of the schema. Pinned by the renamed.a query in ambiguous_col_tolerated.q plus tolerated/re-collision unit tests.

[soumyakanti3578]

Please document when this method should be called.

[konstantinb]

Added a javadoc: call after each by-name resolution of a user-written column reference; expression-map resolutions (processGByExpr) deliberately stay unchecked so Hive's own rewrites (e.g. genSelectDIAST) can reference marked columns.

[soumyakanti3578]

Can we add a note saying that ambiguousName is intentionally excluded from equals and hashcode? Also add a brief reasoning for future devs.

[konstantinb]

Added a note on the field: it is deliberately excluded from equals/hashCode/isSameColumnForRR — a marked and an unmarked copy of a column are still the same column for RowResolver purposes, so including the flag would change RR dedup semantics.

[soumyakanti3578]

nit: Instead of calling get again, can we reuse the result of the first call?

[konstantinb]

Done — restructured to a nested else so the lookup result is reused (the else if chain could not reuse it without evaluating the lookup for the earlier branches too).

[soumyakanti3578]

This test is better suited to be in clientnegative.

[konstantinb]

The move is unfortunately not 1:1. clientnegative requires the run to abort at the first failure (CoreNegativeCliDriver#runTest treats a completed run as "expected to fail but didn't"), and hive.cli.errors.ignore — which this file needs to assert several rejections in one place — prevents exactly that abort. So clientnegative here means one file + golden per rejected shape (5 pairs today — 10 new files replacing these 2, and every future boundary shape costs another file+golden pair instead of two lines in this spec); an earlier iteration of this PR had exactly that layout and it was consolidated into this roll-up so the boundary contract reads as one spec. The mixed accept/reject-in-clientpositive pattern is established — 10 existing files use hive.cli.errors.ignore (resourceplan.q alone pins 40 FAILED lines this way) — and the mechanism-specific single-failure pins do live in clientnegative (the 9 ambiguous_col_*.q files there). I'd prefer keeping the roll-up for those reasons, but happy to split it into per-shape clientnegative files if you still prefer that.

[soumyakanti3578]

Can we move this test to clientnegative? There are others below which throw an Exception too.

[konstantinb]

This one is harder to move: the file is a baseline spec that deliberately interleaves accepted and rejected statements so non-CBO behavior reads shape-by-shape against its CBO mirror (ambiguous_col_unreferenced_tolerated.q). A clientnegative test ends at its first failure — the statements after it would never run — and hive.cli.errors.ignore makes the negative driver fail the test outright ("expected to fail but didn't", CoreNegativeCliDriver#runTest). Same harness constraint as the ambiguous_col_rejected.q thread. If the interleaved baseline reads as too unusual for clientpositive, the alternative I see is dropping the accepted statements and keeping only rejections split per-file — but that loses the shape-by-shape CBO/non-CBO comparison, which is the file's main value. Open to other layouts.

[soumyakanti3578]

Can you explain this a bit more please? Are we mutating the same instances of colInfo which are potentially referenced by the subquery's own RowResolver?

[konstantinb]

They aren't shared — these ColumnInfos are genColListRegex copies (the propagation line oColInfo.setAmbiguousName(...) is the proof: a flag only needs propagating onto a new object), so the subquery's own RowResolver keeps its markers and user-written references through it still reject (pinned by the union-all shape in ambiguous_col_rejected.q). Your instinct about this block was right though — the adjacent Copilot comment found that clearing without reapplying erased the marker for outer references (select x.c from (select distinct * ...)); the update pairs the clear with re-marking the group by output, and the code comment now states the copies fact.

[soumyakanti3578]

nit: Consider renaming this method as the name suggests "non-partition columns" whereas we do append "all columns" sometimes.

[konstantinb]

Agreed the name lied for the non-native arm — but renaming would have meant naming a mode branch that really belongs to the caller, so I dissolved it instead: MergeRewriter now branches (appendAllColsOfTargetTable for non-native tables, whose partition columns are ordinary data columns; appendNonPartitionColsOfTargetTable for native ones, whose partition columns were already emitted by appendAcidSelectColumns), and appendNonPartitionColsOfTargetTable shrank to the one line its name promises.

[soumyakanti3578]

nit: Do you feel s.a looks nicer than a in s? I think it's more readable as we mostly always use <alias>.<column> in sql.

[konstantinb]

Agreed it reads nicer in isolation — I kept a in s deliberately though: it mirrors the non-CBO engine's existing wording for the same error (SemanticAnalyzer#rewriteRRForSubQ composes col + " in " + alias), so Error 10007 reads identically in both engines and the noncbo-baseline/CBO mirror files compare line-by-line. Switching only the CBO site would split the format per engine; switching both would touch non-CBO behavior this PR deliberately leaves alone. There's also a third format loose in the codebase (the uncoded Ambiguous column reference: t.c from RowResolver.get). Unifying all three onto a <alias>.<column> style would make a good follow-up JIRA where every site moves together — happy to file it.

[konstantinb]

Note on the red Jenkins status: runs 6 and 7 each failed only on TestYarnQueueMetricsCollector (UnnecessaryStubbingException) — a flaky test introduced with HIVE-27126, unrelated to this PR; tracked as HIVE-29842 with a fix up in #6729. Will re-trigger once that lands.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
1.2% Duplication on New Code

See analysis details on SonarQube Cloud