soumyakanti3578 commented on code in PR #6676:
URL: https://github.com/apache/hive/pull/6676#discussion_r3832672473
##########
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:
Please document when this method should be called.
##########
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:
nit: Instead of calling `get` again, can we reuse the result of the first
call?
##########
ql/src/test/results/clientpositive/llap/ambiguous_col_rejected.q.out:
##########
Review Comment:
This test is better suited to be in `clientnegative`.
##########
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:
Can we move this test to `clientnegative`? There are others below which
throw an `Exception` too.
##########
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:
nit: Consider renaming this method as the name suggests "non-partition
columns" whereas we do append "all columns" sometimes.
##########
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:
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?
##########
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:
Can we add a note saying that `ambiguousName` is intentionally excluded from
equals and hashcode? Also add a brief reasoning for future devs.
##########
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:
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.
--
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]
