Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12144 )
Change subject: IMPALA-8041, Part 2: Refactor SELECT list ...................................................................... Patch Set 2: (6 comments) Thanks Fredy for the review. Addressed your comments. Had a question about two of them. http://gerrit.cloudera.org:8080/#/c/12144/2/fe/src/main/java/org/apache/impala/analysis/AbstractExpression.java File fe/src/main/java/org/apache/impala/analysis/AbstractExpression.java: http://gerrit.cloudera.org:8080/#/c/12144/2/fe/src/main/java/org/apache/impala/analysis/AbstractExpression.java@23 PS2, Line 23: Represents an expression a while > confusing comment Done http://gerrit.cloudera.org:8080/#/c/12144/2/fe/src/main/java/org/apache/impala/analysis/SelectListItem.java File fe/src/main/java/org/apache/impala/analysis/SelectListItem.java: http://gerrit.cloudera.org:8080/#/c/12144/2/fe/src/main/java/org/apache/impala/analysis/SelectListItem.java@28 PS2, Line 28: import com.google.common.base.Predicates; > nit: empty line after import Done http://gerrit.cloudera.org:8080/#/c/12144/2/fe/src/main/java/org/apache/impala/analysis/SelectListItem.java@40 PS2, Line 40: alias_ > can be final Done http://gerrit.cloudera.org:8080/#/c/12144/2/fe/src/main/java/org/apache/impala/analysis/SelectListItem.java@63 PS2, Line 63: public void setExpr(Expr expr) { expr_ = expr; } > since we already have clone() method, is it necessary to make expr_ mutable Great question; one that touches on the key issue we're trying to address. The select list is a list of (abstract) expressions. Once we form the list (in this new version), the abstract expressions within the list will never change. This is different than the current master version in which the list of expressions do change. The analyzer does rewrites. During that time, in the base version, the analyzer replaces one select list item with the new, rewritten expression. In this version, the abstract expression is unchanged. However, the Expr node does change after rewrite, and that new version is set user using the setExpr() method. In the old version, we had ambiguity about whether the select list contained the original, unwritten expressions or the post-analysis, rewritten versions. (And we had code to try to reset them, which didn't quite work.) In this version, this class holds onto both versions making it clear which is the user's "source" expression and which is rewritten. All of this is moving toward integrating rewrites into the analyzer rather than as a separate bolt-on step. http://gerrit.cloudera.org:8080/#/c/12144/2/fe/src/main/java/org/apache/impala/analysis/SelectListItem.java@220 PS2, Line 220: return (SelectWildcard) item; > Add Preconditions.checkArgument(item instanceof SelectWildcard) Certainly could do so. But all that would do is the very same check that the cast does. Since any error thrown from the cast would clearly indicate the issue, the additional check didn't seem to add much additional checking or information. What do you think? http://gerrit.cloudera.org:8080/#/c/12144/2/fe/src/main/java/org/apache/impala/analysis/SelectListItem.java@223 PS2, Line 223: return (SelectExpr) item; > Add Preconditions.checkArgument(item instanceof SelectExpr) Seme comment/question as above. -- To view, visit http://gerrit.cloudera.org:8080/12144 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5e173dad717e739c097f1f0cd86a0352648ff886 Gerrit-Change-Number: 12144 Gerrit-PatchSet: 2 Gerrit-Owner: Paul Rogers <prog...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Paul Rogers <prog...@cloudera.com> Gerrit-Comment-Date: Wed, 23 Jan 2019 22:39:12 +0000 Gerrit-HasComments: Yes
