[Impala-ASF-CR] IMPALA-5191: Behavior of column aliases should be more standard conforming

2017-12-11 Thread Taras Bobrovytsky (Code Review) 
Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8801 )

Change subject: IMPALA-5191: Behavior of column aliases should be more standard 
conforming
..


Patch Set 3:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/8801/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8801/3//COMMIT_MSG@7
PS3, Line 7: IMPALA-5191: Behavior of column aliases should be more standard 
conforming
You should try to use the imperative mood in the subject line. For example, 
"Standardize column alias behavior"


http://gerrit.cloudera.org:8080/#/c/8801/3//COMMIT_MSG@11
PS3, Line 11: to be more standard conformant.
Add an example in the commit message of what is no longer allowed.


http://gerrit.cloudera.org:8080/#/c/8801/3/fe/src/main/java/org/apache/impala/analysis/Expr.java
File fe/src/main/java/org/apache/impala/analysis/Expr.java:

http://gerrit.cloudera.org:8080/#/c/8801/3/fe/src/main/java/org/apache/impala/analysis/Expr.java@813
PS3, Line 813: it won't substitute
"If onlyTopLevel is true, child expressions of 'this' will not be substituted."


http://gerrit.cloudera.org:8080/#/c/8801/3/fe/src/main/java/org/apache/impala/analysis/Expr.java@833
PS3, Line 833:* the type of 'this'.
Add comment describing the new parameter.


http://gerrit.cloudera.org:8080/#/c/8801/3/fe/src/main/java/org/apache/impala/analysis/Expr.java@836
PS3, Line 836: onlyTopLevel
I thought about this a little, and I think a more intuitive name for this 
parameter is
"substituteChildren", or something similar. What do you think?


http://gerrit.cloudera.org:8080/#/c/8801/3/fe/src/main/java/org/apache/impala/analysis/Expr.java@880
PS3, Line 880: onlyTopLevel
Isn't this always false?


http://gerrit.cloudera.org:8080/#/c/8801/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:

http://gerrit.cloudera.org:8080/#/c/8801/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@961
PS3, Line 961:  group by x"
It would be good to have a positive test with a HAVING clause. Maybe add 
another case where we have "group by x having x" and x is a boolean? (It won't 
work if it's not a boolean, right?)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0f82483b486acf6953876cfa672b0d034f3709a8
Gerrit-Change-Number: 8801
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 12 Dec 2017 01:11:02 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5191: Behavior of column aliases should be more standard conforming

2017-12-11 Thread Zoltan Borok-Nagy (Code Review) 
Hello Taras Bobrovytsky, Tim Armstrong, Alex Behm,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8801

to look at the new patch set (#3).

Change subject: IMPALA-5191: Behavior of column aliases should be more standard 
conforming
..

IMPALA-5191: Behavior of column aliases should be more standard conforming

We should not perform alias substitution in the
subexpressions of GROUP BY, HAVING, and ORDER BY
to be more standard conformant.

A flag called onlyTopLevel is added to the alias
substitution methods. If the flag is true, the
methods don't substitute the aliases of child
expressions.

Some extra checks were added to AnalyzeExprsTest.java.

I had to update other tests to make them pass
since the new behavior is more restrictive.

Change-Id: I0f82483b486acf6953876cfa672b0d034f3709a8
---
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/OrderByElement.java
M fe/src/main/java/org/apache/impala/analysis/QueryStmt.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java
M fe/src/main/java/org/apache/impala/planner/AnalyticEvalNode.java
M fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java
M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java
M fe/src/main/java/org/apache/impala/planner/HdfsPartitionFilter.java
M fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M 
testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/predicate-propagation.test
17 files changed, 69 insertions(+), 47 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/8801/3
--
To view, visit http://gerrit.cloudera.org:8080/8801
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0f82483b486acf6953876cfa672b0d034f3709a8
Gerrit-Change-Number: 8801
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-5191: Behavior of column aliases should be more standard conforming

2017-12-11 Thread Zoltan Borok-Nagy (Code Review) 
Hello Tim Armstrong, Alex Behm,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8801

to look at the new patch set (#2).

Change subject: IMPALA-5191: Behavior of column aliases should be more standard 
conforming
..

IMPALA-5191: Behavior of column aliases should be more standard conforming

We should not perform alias substitution in the
subexpressions of GROUP BY, HAVING, and ORDER BY
to be more standard conformant.

A flag called onlyTopLevel is added to the alias
substitution methods. If the flag is true, the
methods don't substitute the aliases of child
expressions.

Some extra checks were added to AnalyzeExprsTest.java.

I had to update other tests to make them pass
since the new behavior is more restrictive.

Change-Id: I0f82483b486acf6953876cfa672b0d034f3709a8
---
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/OrderByElement.java
M fe/src/main/java/org/apache/impala/analysis/QueryStmt.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java
M fe/src/main/java/org/apache/impala/planner/AnalyticEvalNode.java
M fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java
M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java
M fe/src/main/java/org/apache/impala/planner/HdfsPartitionFilter.java
M fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M 
testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/predicate-propagation.test
17 files changed, 69 insertions(+), 45 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/8801/2
--
To view, visit http://gerrit.cloudera.org:8080/8801
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0f82483b486acf6953876cfa672b0d034f3709a8
Gerrit-Change-Number: 8801
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-5191: Behavior of column aliases should be more standard conforming

2017-12-08 Thread Alex Behm (Code Review) 
Alex Behm has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8801 )

Change subject: IMPALA-5191: Behavior of column aliases should be more standard 
conforming
..


Patch Set 1:

Please see my comments on the JIRA


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0f82483b486acf6953876cfa672b0d034f3709a8
Gerrit-Change-Number: 8801
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 08 Dec 2017 22:56:18 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5191: Behavior of column aliases should be more standard conforming

2017-12-08 Thread Zoltan Borok-Nagy (Code Review) 
Zoltan Borok-Nagy has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8801


Change subject: IMPALA-5191: Behavior of column aliases should be more standard 
conforming
..

IMPALA-5191: Behavior of column aliases should be more standard conforming

We should not perform alias substitution in the
subexpressions of GROUP BY. This commit allows
Impala to accept "column alias" and "Column name"
indistinctly in the group by clause (also if
Column is made by a function).

A flag called onlyTopLevel is added to the alias
substitution methods. If the flag is true, the
methods don't substitute the aliases of child
expressions.

Two extra checks are added to AnalyzeExprsTest.java.

Change-Id: I0f82483b486acf6953876cfa672b0d034f3709a8
---
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/QueryStmt.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
7 files changed, 34 insertions(+), 16 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/8801/1
-- 
To view, visit http://gerrit.cloudera.org:8080/8801
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I0f82483b486acf6953876cfa672b0d034f3709a8
Gerrit-Change-Number: 8801
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy