impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dimitris Tsirogiannis (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4336: Cast exprs after unnesting union operands.
Date Wed, 26 Oct 2016 05:20:43 GMT
Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-4336: Cast exprs after unnesting union operands.
......................................................................


Patch Set 1:

(8 comments)

Flushing some initial comments. Still need to wrap my head around all the moving parts.

http://gerrit.cloudera.org:8080/#/c/4815/1//COMMIT_MSG
Commit Message:

PS1, Line 15: Also clarifies the use of resultExprs vs. baseTblResultExprs
            : in unions. We should consistently use resultExprs because the
            : final expr substitution happens during planning.
            : The only place where baseTblResultExprs should be used is in
            : UnionStmt.materializeRequiredSlots() because that is called
            : before plan generation and we need to mark the slots of resolved
            : exprs as materialized.
I am not sure this belongs to the commit message. If you don't have it already, you may want
to put it as a comment somewhere in the code.


http://gerrit.cloudera.org:8080/#/c/4815/1/fe/src/main/java/org/apache/impala/analysis/UnionStmt.java
File fe/src/main/java/org/apache/impala/analysis/UnionStmt.java:

PS1, Line 202: Propagates DISTINCT from left to right, and checks that all union operands
are
             :    * are union compatible, adding implicit casts if necessary.
I think you need to update this comment. This function seems to be doing a lot more than simply
propagating distinct and checking union operand compatibility.


PS1, Line 203: are
remove


PS1, Line 218: // Analyze all operands and make sure they return an equal number of exprs.
             :     for (int i = 0; i < operands_.size(); ++i) {
             :       try {
             :         operands_.get(i).analyze(analyzer);
             :         QueryStmt firstQuery = operands_.get(0).getQueryStmt();
             :         List<Expr> firstExprs = operands_.get(0).getQueryStmt().getResultExprs();
             :         QueryStmt query = operands_.get(i).getQueryStmt();
             :         List<Expr> exprs = query.getResultExprs();
             :         if (firstExprs.size() != exprs.size()) {
             :           throw new AnalysisException("Operands have unequal number of columns:\n"
+
             :               "'" + queryStmtToSql(firstQuery) + "' has " +
             :               firstExprs.size() + " column(s)\n" +
             :               "'" + queryStmtToSql(query) + "' has " + exprs.size() + " column(s)");
             :         }
             :       } catch (AnalysisException e) {
             :         if (analyzer.getMissingTbls().isEmpty()) throw e;
             :       }
             :     }
This function has grown a bit too much. I would put this block is a separate function called
analyzeOperands().


Line 241:     // Remember SQL string before unnesting operands.
the


PS1, Line 258: // Cast all result exprs to a compatible type.
move above L263


PS1, Line 282: // Should never happen
             :         throw new AnalysisException("Error creating agg info in UnionStmt.analyze()");
Maybe convert it to Preconditions.checkState(false, "Error....")?


http://gerrit.cloudera.org:8080/#/c/4815/1/testdata/workloads/functional-query/queries/QueryTest/union.test
File testdata/workloads/functional-query/queries/QueryTest/union.test:

PS1, Line 1030: (select 80 union all select 90)
Maybe add a test case with nested union distinct, if not already there.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5e3ab7349df7d67d0d9c2baf4a56342d3f04e76d
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message