konstantinb commented on code in PR #6676:
URL: https://github.com/apache/hive/pull/6676#discussion_r3876117166
##########
ql/src/test/results/clientpositive/llap/ambiguous_col_noncbo_baseline.q.out:
##########
@@ -0,0 +1,44 @@
+FAILED: SemanticException [Error 10007]: Ambiguous column reference c in t
+FAILED: SemanticException [Error 10007]: Ambiguous column reference c in c1
+FAILED: SemanticException [Error 10007]: Ambiguous column reference key in subq
+PREHOOK: query: create table wj3 (k int, v int)
+PREHOOK: type: CREATETABLE
+PREHOOK: Output: database:default
+PREHOOK: Output: default@wj3
+POSTHOOK: query: create table wj3 (k int, v int)
+POSTHOOK: type: CREATETABLE
+POSTHOOK: Output: database:default
+POSTHOOK: Output: default@wj3
+PREHOOK: query: create table wj4 (k int, w int)
+PREHOOK: type: CREATETABLE
+PREHOOK: Output: database:default
+PREHOOK: Output: default@wj4
+POSTHOOK: query: create table wj4 (k int, w int)
+POSTHOOK: type: CREATETABLE
+POSTHOOK: Output: database:default
+POSTHOOK: Output: default@wj4
+FAILED: SemanticException [Error 10007]: Ambiguous column reference k in t
+FAILED: SemanticException [Error 10007]: Ambiguous column reference c in a
+FAILED: SemanticException [Error 10007]: Ambiguous column reference c in sq_1
+FAILED: SemanticException [Error 10007]: Ambiguous column reference c in sq_1
+FAILED: SemanticException [Error 10007]: Ambiguous column reference key in a
Review Comment:
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.
##########
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:
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.
##########
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:
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.
--
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]
