Steve Carlin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/24211 )

Change subject: IMPALA-14909: Calcite planner: Fix validation for views with 
column list
......................................................................


Patch Set 6:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/24211/6/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/ViewAliasCorrector.java
File 
java/calcite-planner/src/main/java/org/apache/impala/calcite/service/ViewAliasCorrector.java:

http://gerrit.cloudera.org:8080/#/c/24211/6/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/ViewAliasCorrector.java@37
PS6, Line 37: Impala
> nit: using "Hive" might be better. I think originally it's Hive that create
Done


http://gerrit.cloudera.org:8080/#/c/24211/6/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/ViewAliasCorrector.java@55
PS6, Line 55: The
> nit: "the"
Done


http://gerrit.cloudera.org:8080/#/c/24211/6/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/ViewAliasCorrector.java@130
PS6, Line 130:         return itemsWithAliasIssue;
> nit: Let's return Collections.emptySet() to avoid creating a new object. We
Done


http://gerrit.cloudera.org:8080/#/c/24211/6/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/ViewAliasCorrector.java@133
PS6, Line 133:       // The alias issue only happens when the columns are all 
selected out (see top
             :       // level comment), so if the sizes are different, there is 
no issue.
             :       if (topLevelSqlNodes_.size() != 
secondLevelSqlNodes_.size()) {
             :         return itemsWithAliasIssue;
             :       }
> I found that stars (*) are not expanded at this point. So a view created wi
Nice catch!

Created IMPALA-15116.  This might be tricky to fix, so yeah, I'll address this 
after.


http://gerrit.cloudera.org:8080/#/c/24211/6/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/ViewAliasCorrector.java@151
PS6, Line 151: topLevelIdentifier.names.get(1)
> Could this be null?
I don't think so?  But I put in a guard just in case.


http://gerrit.cloudera.org:8080/#/c/24211/6/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/ViewAliasCorrector.java@194
PS6, Line 194: get(1)
> nit: I'm new to Calcite so these indexes always confused me. It might be mo
Done



--
To view, visit http://gerrit.cloudera.org:8080/24211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d0229c8907f69e648034d12fe375d9d9a384e25
Gerrit-Change-Number: 24211
Gerrit-PatchSet: 6
Gerrit-Owner: Steve Carlin <[email protected]>
Gerrit-Reviewer: Aman Sinha <[email protected]>
Gerrit-Reviewer: Csaba Ringhofer <[email protected]>
Gerrit-Reviewer: Fang-Yu Rao <[email protected]>
Gerrit-Reviewer: Impala Public Jenkins <[email protected]>
Gerrit-Reviewer: Jason Fehr <[email protected]>
Gerrit-Reviewer: Joe McDonnell <[email protected]>
Gerrit-Reviewer: Michael Smith <[email protected]>
Gerrit-Reviewer: Quanlong Huang <[email protected]>
Gerrit-Reviewer: Steve Carlin <[email protected]>
Gerrit-Comment-Date: Thu, 25 Jun 2026 16:22:43 +0000
Gerrit-HasComments: Yes

Reply via email to