impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4336: Cast exprs after unnesting union operands.
Date Wed, 26 Oct 2016 06:07:45 GMT
Alex Behm has posted comments on this change.

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

Patch Set 1:

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 alre
Shrunk comment here. Moved expanded comment to UnionStmt class comment.
File fe/src/main/java/org/apache/impala/analysis/

PS1, Line 202: Propagates DISTINCT from left to right, and checks that all union operands
             :    * are union compatible, adding implicit casts if necessary.
> I think you need to update this comment. This function seems to be doing a 
I removed this comment because it's redundant with the class comment. Let me know if you feel
the class comment still needs to be expanded/moved.

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 separat

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

PS1, Line 258: // Cast all result exprs to a compatible type.
> move above L263
We need to collect them first before we can analyzer.castToUnionCompatibleTypes(), so seems
relevant to this loop as well. Modified comment.

PS1, Line 282: // Should never happen
             :         throw new AnalysisException("Error creating agg info in UnionStmt.analyze()");
> Maybe convert it to Preconditions.checkState(false, "Error....")?
Modified to an ISE which is what the Preconditions would also throw. I prefer the ISe because
we can give it a cause.
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.
A nested union distinct cannot be unnested, so is not really related to this bug. We have
tests for that elsewhere.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I5e3ab7349df7d67d0d9c2baf4a56342d3f04e76d
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Dimitris Tsirogiannis <>
Gerrit-HasComments: Yes

View raw message