konstantinb commented on code in PR #6676:
URL: https://github.com/apache/hive/pull/6676#discussion_r3876117621
##########
ql/src/test/results/clientnegative/cbo_ambiguous_colref_in_gby.q.out:
##########
@@ -6,4 +6,4 @@ POSTHOOK: query: create table t1 (a int)
POSTHOOK: type: CREATETABLE
POSTHOOK: Output: database:default
POSTHOOK: Output: default@t1
-FAILED: SemanticException Ambiguous column reference: s.a
+FAILED: SemanticException [Error 10007]: Ambiguous column reference a in s
Review Comment:
I kept `a in s` deliberately: 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.
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java:
##########
@@ -3616,6 +3616,12 @@ private RelNode genGBLogicalPlan(QB qb, RelNode srcRel)
throws SemanticException
// As we said before, here we use genSelectLogicalPlan to rewrite
AllColRef
srcRel = genSelectLogicalPlan(qb, srcRel, srcRel, null, null,
true).getKey();
RowResolver rr = relToHiveRR.get(srcRel);
+ // genSelectDIAST synthesizes one reference per rslvMap entry, each
unique by
+ // construction, so clear the HIVE-29580 ambiguity markers on this
rewrite-private
+ // projection; the subquery's own RowResolver keeps them for
user-written references.
+ for (ColumnInfo colInfo : rr.getColumnInfos()) {
+ colInfo.setAmbiguousName(false);
Review Comment:
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`.
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java:
##########
@@ -4887,6 +4894,15 @@ private RelNode genLogicalPlan(QB qb, boolean
outerMostQB,
} else if ("".equals(tmp[0]) || tmp[1] == null) {
// ast expression is not a valid column name for table
tmp[1] = colInfo.getInternalName();
+ } else if (newRR.get(alias, tmp[1]) != null) {
+ // Duplicate alias escaping the subquery boundary: tolerated for
positional use
+ // (HIVE-19770), but poison the name so a later by-name reference
fails (HIVE-29580).
+ // Binding the duplicate to its internal name here is deliberate,
not redundant:
+ // putWithCheck would otherwise do it via its own fallback AND
call keepAmbiguousInfo,
+ // whose reference-time throw in RowResolver.get would then shadow
this marker with a
+ // differently formatted message. Do not "simplify" this line away.
+ newRR.get(alias, tmp[1]).setAmbiguousName(true);
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
