konstantinb commented on code in PR #6676:
URL: https://github.com/apache/hive/pull/6676#discussion_r3876116367
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/type/TypeCheckProcFactory.java:
##########
@@ -594,6 +594,13 @@ protected IntervalExprProcessor getIntervalExprProcessor()
{
return new IntervalExprProcessor();
}
+ static void checkAmbiguousName(ColumnInfo colInfo) throws SemanticException {
Review Comment:
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.
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/ColumnInfo.java:
##########
@@ -59,6 +59,8 @@ public class ColumnInfo implements Serializable {
private boolean isHiddenVirtualCol;
+ private boolean ambiguousName;
Review Comment:
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.
##########
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:
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).
##########
ql/src/test/results/clientpositive/llap/ambiguous_col_rejected.q.out:
##########
Review Comment:
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, one more per future shape); 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.
##########
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 can't 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.
##########
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:
No — 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/java/org/apache/hadoop/hive/ql/parse/rewrite/sql/MultiInsertSqlGenerator.java:
##########
@@ -169,6 +169,18 @@ public void appendAllColsOfTargetTable(String prefix) {
public void appendAllColsOfTargetTable() {
appendCols(targetTable.getAllCols(), FieldSchema::getName);
}
+
+ /**
+ * Appends the target table's columns, omitting the partition columns when
the table uses native
+ * partitioning: appendAcidSelectColumns has already emitted those, and
emitting them a second
+ * time yields a projection with duplicate column names, making any by-name
reference to them
+ * ambiguous. Non-native tables (e.g. Iceberg) carry partition columns as
regular columns, so for
+ * those all columns are appended.
+ */
+ public void appendNonPartitionColsOfTargetTable() {
Review Comment:
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.
--
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]
