You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@impala.apache.org by "Paul Rogers (Code Review)" <ge...@cloudera.org> on 2018/11/20 07:27:11 UTC

[Impala-ASF-CR] MPALA-7867, part 1: Expose List in TreeNode, parser

Paul Rogers has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11954


Change subject: MPALA-7867, part 1: Expose List in TreeNode, parser
......................................................................

MPALA-7867, part 1: Expose List in TreeNode, parser

When using Java collections, a common Java best practice is to expose
the collection interface, but hide the implementation choice. This
pattern allows us to start with a generic implementation (an ArrayList,
say), but evolve to a more specific implementation to achieve certain
goals (a LinkedList or ImmutableList, say.)

For whatever reason, the Impala FE code exposes ArrayList, HashMap and
other implementation choices as variable types and in method signatures.

Also, since Java 7, the preferred way to create an array is

new ArrayList<>()

Replaced older forms:

new ArrayList<foo>() // Pre-Java 7
Lists.newArrayList() // Guava form, pre-Java 7

This ticket cleans up two files, and their dependencies:

* TreeNode (the root of all parser nodes)
* sql-parser.cup (the code which creates the parser nodes)

Many other uses exist, and will be submitted as separate patches to keep
patches small.

Tests: This is purely a refactoring, no functionality changed. Ran the
FE unit tests to verify no regressions.

Change-Id: Iebab7dccdb4b2fa0b5ca812beab0e8bdba39f539
---
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionName.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/SlotRef.java
M fe/src/main/java/org/apache/impala/analysis/UnionStmt.java
M fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java
M fe/src/main/java/org/apache/impala/analysis/WithClause.java
M fe/src/main/java/org/apache/impala/catalog/StructType.java
M fe/src/main/java/org/apache/impala/common/TreeNode.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
15 files changed, 99 insertions(+), 89 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iebab7dccdb4b2fa0b5ca812beab0e8bdba39f539
Gerrit-Change-Number: 11954
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>

[Impala-ASF-CR] IMPALA-7867 (Part 1): Expose List in TreeNode, parser

Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.

Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/11954 )

Change subject: IMPALA-7867 (Part 1): Expose List in TreeNode, parser
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11954/2/fe/src/main/java/org/apache/impala/common/TreeNode.java
File fe/src/main/java/org/apache/impala/common/TreeNode.java:

http://gerrit.cloudera.org:8080/#/c/11954/2/fe/src/main/java/org/apache/impala/common/TreeNode.java@62
PS2, Line 62: ult = new A
> You still can replace ArrayList with List instead of reverting everything.
Good catch! Was a little too quick with my Eclipse diff tool...

The way around the type issue is actually used later in this class: pass in the target class to allow runtime type checks. Either a) filter nodes that don't derive from the target class, or b) raise an error in this case.

We'll leave this as an exercise for later. Won't be too hard: there are only a couple of uses of this function and the post-order version.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebab7dccdb4b2fa0b5ca812beab0e8bdba39f539
Gerrit-Change-Number: 11954
Gerrit-PatchSet: 3
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Comment-Date: Wed, 21 Nov 2018 03:28:56 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7867 (Part 1): Expose List in TreeNode, parser

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11954 )

Change subject: IMPALA-7867 (Part 1): Expose List in TreeNode, parser
......................................................................


Patch Set 7:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3490/ DRY_RUN=false


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebab7dccdb4b2fa0b5ca812beab0e8bdba39f539
Gerrit-Change-Number: 11954
Gerrit-PatchSet: 7
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Alex Rodoni <ar...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Comment-Date: Mon, 26 Nov 2018 20:07:40 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7867 (Part 1): Expose List in TreeNode, parser

Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.

Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/11954 )

Change subject: IMPALA-7867 (Part 1): Expose List in TreeNode, parser
......................................................................


Patch Set 3:

(3 comments)

Addressed code review comments. Please take another look at your convenience.

http://gerrit.cloudera.org:8080/#/c/11954/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11954/2//COMMIT_MSG@6
PS2, Line 6: 
           : IMPALA-7867 (Part 1
> nit: IMPALA-7876 (part 1) is usually the preferred form.
Done


http://gerrit.cloudera.org:8080/#/c/11954/2/fe/src/main/cup/sql-parser.cup
File fe/src/main/cup/sql-parser.cup:

http://gerrit.cloudera.org:8080/#/c/11954/2/fe/src/main/cup/sql-parser.cup@60
PS2, Line 60: 
> Can we clean up the imports as well? I don't think we need Guava Imports an
Cleaned up unused imports. We still use Guava Lists.newArrayList(x) to create one-item lists; a method for which there is no handy JDK equivalent.


http://gerrit.cloudera.org:8080/#/c/11954/2/fe/src/main/java/org/apache/impala/common/TreeNode.java
File fe/src/main/java/org/apache/impala/common/TreeNode.java:

http://gerrit.cloudera.org:8080/#/c/11954/2/fe/src/main/java/org/apache/impala/common/TreeNode.java@62
PS2, Line 62: ult = new A
> I don't see the reason for replacing TreeNode<NodeType> with TreeNode<?>., 
Reverted this for now. The problem is that this method appears to be type-safe, but it is not. Because of type erasure, the parameterized type is not available at runtime. Thus the original "(C) this" cast is not runtime safe, it is only a compile time check. Since it is inherently type-unsafe, the IDE issues a warning. Since we know it is type-unsafe, and want to proceed, we add the suppress warnings to state our intent.

This is a bit of a tricky area when using Java generics. I'll research it and see if there is a cleaner solution we can retrofit into this existing code.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebab7dccdb4b2fa0b5ca812beab0e8bdba39f539
Gerrit-Change-Number: 11954
Gerrit-PatchSet: 3
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Comment-Date: Tue, 20 Nov 2018 22:43:29 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7867 (Part 1): Expose List in TreeNode, parser

Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.

Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/11954 )

Change subject: IMPALA-7867 (Part 1): Expose List in TreeNode, parser
......................................................................


Patch Set 6:

Alex, sorry, I botched a rebase and the title got changed to one of yours temporarily. Sorry about that, back to normal now. This change turns out to have no doc. impact.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebab7dccdb4b2fa0b5ca812beab0e8bdba39f539
Gerrit-Change-Number: 11954
Gerrit-PatchSet: 6
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Alex Rodoni <ar...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Comment-Date: Mon, 26 Nov 2018 19:25:24 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7867, part 1: Expose List in TreeNode, parser

Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.

Hello Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-7867, part 1: Expose List in TreeNode, parser
......................................................................

IMPALA-7867, part 1: Expose List in TreeNode, parser

When using Java collections, a common Java best practice is to expose
the collection interface, but hide the implementation choice. This
pattern allows us to start with a generic implementation (an ArrayList,
say), but evolve to a more specific implementation to achieve certain
goals (a LinkedList or ImmutableList, say.)

For whatever reason, the Impala FE code exposes ArrayList, HashMap and
other implementation choices as variable types and in method signatures.

Also, since Java 7, the preferred way to create an array is

new ArrayList<>()

Replaced older forms:

new ArrayList<foo>() // Pre-Java 7
Lists.newArrayList() // Guava form, pre-Java 7

This ticket cleans up two files, and their dependencies:

* TreeNode (the root of all parser nodes)
* sql-parser.cup (the code which creates the parser nodes)

Many other uses exist, and will be submitted as separate patches to keep
patches small.

Tests: This is purely a refactoring, no functionality changed. Ran the
FE unit tests to verify no regressions.

Change-Id: Iebab7dccdb4b2fa0b5ca812beab0e8bdba39f539
---
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionName.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/SlotRef.java
M fe/src/main/java/org/apache/impala/analysis/UnionStmt.java
M fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java
M fe/src/main/java/org/apache/impala/analysis/WithClause.java
M fe/src/main/java/org/apache/impala/catalog/StructType.java
M fe/src/main/java/org/apache/impala/common/TreeNode.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
15 files changed, 99 insertions(+), 89 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iebab7dccdb4b2fa0b5ca812beab0e8bdba39f539
Gerrit-Change-Number: 11954
Gerrit-PatchSet: 2
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>

[Impala-ASF-CR] IMPALA-7867 (Part 1): Expose List in TreeNode, parser

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11954 )

Change subject: IMPALA-7867 (Part 1): Expose List in TreeNode, parser
......................................................................

IMPALA-7867 (Part 1): Expose List in TreeNode, parser

When using Java collections, a common Java best practice is to expose
the collection interface, but hide the implementation choice. This
pattern allows us to start with a generic implementation (an ArrayList,
say), but evolve to a more specific implementation to achieve certain
goals (a LinkedList or ImmutableList, say.)

For whatever reason, the Impala FE code exposes ArrayList, HashMap and
other implementation choices as variable types and in method signatures.

Also, since Java 7, the preferred way to create an array is

new ArrayList<>()

Replaced older forms:

new ArrayList<foo>() // Pre-Java 7
Lists.newArrayList() // Guava form, pre-Java 7

This ticket cleans up two files, and their dependencies:

* TreeNode (the root of all parser nodes)
* sql-parser.cup (the code which creates the parser nodes)

Many other uses exist, and will be submitted as separate patches to keep
patches small.

In TreeNode, also cleaned up some of the generic expresions, which
caused dependencies to change in order to be more type-safe.

Tests: This is purely a refactoring, no functionality changed. Ran the
FE unit tests to verify no regressions.

Change-Id: Iebab7dccdb4b2fa0b5ca812beab0e8bdba39f539
Reviewed-on: http://gerrit.cloudera.org:8080/11954
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/AnalyticInfo.java
M fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionName.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/SlotRef.java
M fe/src/main/java/org/apache/impala/analysis/UnionStmt.java
M fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java
M fe/src/main/java/org/apache/impala/analysis/WithClause.java
M fe/src/main/java/org/apache/impala/catalog/StructType.java
M fe/src/main/java/org/apache/impala/common/TreeNode.java
M fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
17 files changed, 103 insertions(+), 92 deletions(-)

Approvals:
  Impala Public Jenkins: Looks good to me, approved; Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Iebab7dccdb4b2fa0b5ca812beab0e8bdba39f539
Gerrit-Change-Number: 11954
Gerrit-PatchSet: 8
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Alex Rodoni <ar...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>

[Impala-ASF-CR] IMPALA-7867 (Part 1): Expose List in TreeNode, parser

Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.

Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/11954 )

Change subject: IMPALA-7867 (Part 1): Expose List in TreeNode, parser
......................................................................


Patch Set 4:

(4 comments)

Fredy, addressed the generics issues as you suggested. Turned out to be a bit tricky. Please take another look.

http://gerrit.cloudera.org:8080/#/c/11954/4/fe/src/main/java/org/apache/impala/common/TreeNode.java
File fe/src/main/java/org/apache/impala/common/TreeNode.java:

http://gerrit.cloudera.org:8080/#/c/11954/4/fe/src/main/java/org/apache/impala/common/TreeNode.java@132
PS4, Line 132: ?
> this should be Class<C>
Turns out it should be Class<D>. There was a caller that had the wrong list type, had to fix that so the revised signature would work.


http://gerrit.cloudera.org:8080/#/c/11954/4/fe/src/main/java/org/apache/impala/common/TreeNode.java@166
PS4, Line 166: ?
> this should be Class<C>
Turns out it should be Class<D>. There was a caller that had the wrong list type, had to fix that so the revised signature would work. This then forced a (correct) change to a member variable, and so on.


http://gerrit.cloudera.org:8080/#/c/11954/4/fe/src/main/java/org/apache/impala/common/TreeNode.java@184
PS4, Line 184: public boolean contains(Class<?> cl)
> this should be public <C extends TreeNode<NodeType>> boolean contains(Class
Done


http://gerrit.cloudera.org:8080/#/c/11954/4/fe/src/main/java/org/apache/impala/common/TreeNode.java@213
PS4, Line 213: <?>
> this should be Class<C>
Actually ? extends C.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebab7dccdb4b2fa0b5ca812beab0e8bdba39f539
Gerrit-Change-Number: 11954
Gerrit-PatchSet: 4
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Comment-Date: Mon, 26 Nov 2018 18:14:25 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7867 (Part 1): Expose List in TreeNode, parser

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11954 )

Change subject: IMPALA-7867 (Part 1): Expose List in TreeNode, parser
......................................................................


Patch Set 3:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/1415/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebab7dccdb4b2fa0b5ca812beab0e8bdba39f539
Gerrit-Change-Number: 11954
Gerrit-PatchSet: 3
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Comment-Date: Tue, 20 Nov 2018 23:05:58 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7867 (Part 1): Expose List in TreeNode, parser

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11954 )

Change subject: IMPALA-7867 (Part 1): Expose List in TreeNode, parser
......................................................................


Patch Set 7: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebab7dccdb4b2fa0b5ca812beab0e8bdba39f539
Gerrit-Change-Number: 11954
Gerrit-PatchSet: 7
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Alex Rodoni <ar...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Comment-Date: Mon, 26 Nov 2018 20:07:39 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7867 (Part 1): Expose List in TreeNode, parser

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.

Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11954 )

Change subject: IMPALA-7867 (Part 1): Expose List in TreeNode, parser
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11954/2/fe/src/main/java/org/apache/impala/common/TreeNode.java
File fe/src/main/java/org/apache/impala/common/TreeNode.java:

http://gerrit.cloudera.org:8080/#/c/11954/2/fe/src/main/java/org/apache/impala/common/TreeNode.java@62
PS2, Line 62: ult = new A
> Reverted this for now. The problem is that this method appears to be type-s
You still can replace ArrayList with List instead of reverting everything.

Yes, this is an issue with type erasure in Java and I'm afraid there's no way around it. Despite being "unsafe", from the API standpoint TreeNode<NodeType> is far more readable than TreeNode<?> since TreeNode<?> means TreeNode can hold anything when the fact that it's always going to be NodeType for this particular class.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebab7dccdb4b2fa0b5ca812beab0e8bdba39f539
Gerrit-Change-Number: 11954
Gerrit-PatchSet: 3
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Comment-Date: Wed, 21 Nov 2018 02:39:03 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] [DOCS] Added a note in impala scan bytes limit.xml

Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.

Hello Fredy Wijaya, Impala Public Jenkins, 

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

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

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

Change subject: [DOCS] Added a note in impala_scan_bytes_limit.xml
......................................................................

[DOCS] Added a note in impala_scan_bytes_limit.xml

- Added a note that queries can scan over the limit of this option
at times.

Change-Id: Id20f952622ce553d9c1f47e469d97a3b4c19683f
Reviewed-on: http://gerrit.cloudera.org:8080/11936
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Reviewed-by: Bikramjeet Vig <bi...@cloudera.com>

IMPALA-7867 (Part 1): Expose List in TreeNode, parser

When using Java collections, a common Java best practice is to expose
the collection interface, but hide the implementation choice. This
pattern allows us to start with a generic implementation (an ArrayList,
say), but evolve to a more specific implementation to achieve certain
goals (a LinkedList or ImmutableList, say.)

For whatever reason, the Impala FE code exposes ArrayList, HashMap and
other implementation choices as variable types and in method signatures.

Also, since Java 7, the preferred way to create an array is

new ArrayList<>()

Replaced older forms:

new ArrayList<foo>() // Pre-Java 7
Lists.newArrayList() // Guava form, pre-Java 7

This ticket cleans up two files, and their dependencies:

* TreeNode (the root of all parser nodes)
* sql-parser.cup (the code which creates the parser nodes)

Many other uses exist, and will be submitted as separate patches to keep
patches small.

In TreeNode, also cleaned up some of the generic expresions, which
caused dependencies to change in order to be more type-safe.

Tests: This is purely a refactoring, no functionality changed. Ran the
FE unit tests to verify no regressions.

Change-Id: Iebab7dccdb4b2fa0b5ca812beab0e8bdba39f539
---
M docs/topics/impala_scan_bytes_limit.xml
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/AnalyticInfo.java
M fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionName.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/SlotRef.java
M fe/src/main/java/org/apache/impala/analysis/UnionStmt.java
M fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java
M fe/src/main/java/org/apache/impala/analysis/WithClause.java
M fe/src/main/java/org/apache/impala/catalog/StructType.java
M fe/src/main/java/org/apache/impala/common/TreeNode.java
M fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
18 files changed, 111 insertions(+), 97 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/54/11954/5
-- 
To view, visit http://gerrit.cloudera.org:8080/11954
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iebab7dccdb4b2fa0b5ca812beab0e8bdba39f539
Gerrit-Change-Number: 11954
Gerrit-PatchSet: 5
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Alex Rodoni <ar...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>

[Impala-ASF-CR] IMPALA-7867 (Part 1): Expose List in TreeNode, parser

Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.

Hello Fredy Wijaya, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-7867 (Part 1): Expose List in TreeNode, parser
......................................................................

IMPALA-7867 (Part 1): Expose List in TreeNode, parser

When using Java collections, a common Java best practice is to expose
the collection interface, but hide the implementation choice. This
pattern allows us to start with a generic implementation (an ArrayList,
say), but evolve to a more specific implementation to achieve certain
goals (a LinkedList or ImmutableList, say.)

For whatever reason, the Impala FE code exposes ArrayList, HashMap and
other implementation choices as variable types and in method signatures.

Also, since Java 7, the preferred way to create an array is

new ArrayList<>()

Replaced older forms:

new ArrayList<foo>() // Pre-Java 7
Lists.newArrayList() // Guava form, pre-Java 7

This ticket cleans up two files, and their dependencies:

* TreeNode (the root of all parser nodes)
* sql-parser.cup (the code which creates the parser nodes)

Many other uses exist, and will be submitted as separate patches to keep
patches small.

Tests: This is purely a refactoring, no functionality changed. Ran the
FE unit tests to verify no regressions.

Change-Id: Iebab7dccdb4b2fa0b5ca812beab0e8bdba39f539
---
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionName.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/SlotRef.java
M fe/src/main/java/org/apache/impala/analysis/UnionStmt.java
M fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java
M fe/src/main/java/org/apache/impala/analysis/WithClause.java
M fe/src/main/java/org/apache/impala/catalog/StructType.java
M fe/src/main/java/org/apache/impala/common/TreeNode.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
15 files changed, 86 insertions(+), 79 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iebab7dccdb4b2fa0b5ca812beab0e8bdba39f539
Gerrit-Change-Number: 11954
Gerrit-PatchSet: 3
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>

[Impala-ASF-CR] IMPALA-7867 (Part 1): Expose List in TreeNode, parser

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.

Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11954 )

Change subject: IMPALA-7867 (Part 1): Expose List in TreeNode, parser
......................................................................


Patch Set 6: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebab7dccdb4b2fa0b5ca812beab0e8bdba39f539
Gerrit-Change-Number: 11954
Gerrit-PatchSet: 6
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Alex Rodoni <ar...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Comment-Date: Mon, 26 Nov 2018 20:07:01 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7867 (Part 1): Expose List in TreeNode, parser

Posted by "Alex Rodoni (Code Review)" <ge...@cloudera.org>.

Alex Rodoni has posted comments on this change. ( http://gerrit.cloudera.org:8080/11954 )

Change subject: IMPALA-7867 (Part 1): Expose List in TreeNode, parser
......................................................................


Patch Set 6:

Paul,
Did you mean to add me as a reviewer? 
Is there a doc impact with this change?


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebab7dccdb4b2fa0b5ca812beab0e8bdba39f539
Gerrit-Change-Number: 11954
Gerrit-PatchSet: 6
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Alex Rodoni <ar...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Comment-Date: Mon, 26 Nov 2018 19:06:18 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7867 (Part 1): Expose List in TreeNode, parser

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.

Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11954 )

Change subject: IMPALA-7867 (Part 1): Expose List in TreeNode, parser
......................................................................


Patch Set 4:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/11954/4/fe/src/main/java/org/apache/impala/common/TreeNode.java
File fe/src/main/java/org/apache/impala/common/TreeNode.java:

http://gerrit.cloudera.org:8080/#/c/11954/4/fe/src/main/java/org/apache/impala/common/TreeNode.java@132
PS4, Line 132: ?
this should be Class<C>


http://gerrit.cloudera.org:8080/#/c/11954/4/fe/src/main/java/org/apache/impala/common/TreeNode.java@166
PS4, Line 166: ?
this should be Class<C>


http://gerrit.cloudera.org:8080/#/c/11954/4/fe/src/main/java/org/apache/impala/common/TreeNode.java@184
PS4, Line 184: public boolean contains(Class<?> cl)
this should be public <C extends TreeNode<NodeType>> boolean contains(Class<C> cl)


http://gerrit.cloudera.org:8080/#/c/11954/4/fe/src/main/java/org/apache/impala/common/TreeNode.java@213
PS4, Line 213: <?>
this should be Class<C>



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebab7dccdb4b2fa0b5ca812beab0e8bdba39f539
Gerrit-Change-Number: 11954
Gerrit-PatchSet: 4
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Comment-Date: Wed, 21 Nov 2018 21:46:34 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7867 (Part 1): Expose List in TreeNode, parser

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11954 )

Change subject: IMPALA-7867 (Part 1): Expose List in TreeNode, parser
......................................................................


Patch Set 7: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebab7dccdb4b2fa0b5ca812beab0e8bdba39f539
Gerrit-Change-Number: 11954
Gerrit-PatchSet: 7
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Alex Rodoni <ar...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Comment-Date: Tue, 27 Nov 2018 00:04:56 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7867 (Part 1): Expose List in TreeNode, parser

Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.

Hello Alex Rodoni, Fredy Wijaya, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-7867 (Part 1): Expose List in TreeNode, parser
......................................................................

IMPALA-7867 (Part 1): Expose List in TreeNode, parser

When using Java collections, a common Java best practice is to expose
the collection interface, but hide the implementation choice. This
pattern allows us to start with a generic implementation (an ArrayList,
say), but evolve to a more specific implementation to achieve certain
goals (a LinkedList or ImmutableList, say.)

For whatever reason, the Impala FE code exposes ArrayList, HashMap and
other implementation choices as variable types and in method signatures.

Also, since Java 7, the preferred way to create an array is

new ArrayList<>()

Replaced older forms:

new ArrayList<foo>() // Pre-Java 7
Lists.newArrayList() // Guava form, pre-Java 7

This ticket cleans up two files, and their dependencies:

* TreeNode (the root of all parser nodes)
* sql-parser.cup (the code which creates the parser nodes)

Many other uses exist, and will be submitted as separate patches to keep
patches small.

In TreeNode, also cleaned up some of the generic expresions, which
caused dependencies to change in order to be more type-safe.

Tests: This is purely a refactoring, no functionality changed. Ran the
FE unit tests to verify no regressions.

Change-Id: Iebab7dccdb4b2fa0b5ca812beab0e8bdba39f539
---
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/AnalyticInfo.java
M fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionName.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/SlotRef.java
M fe/src/main/java/org/apache/impala/analysis/UnionStmt.java
M fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java
M fe/src/main/java/org/apache/impala/analysis/WithClause.java
M fe/src/main/java/org/apache/impala/catalog/StructType.java
M fe/src/main/java/org/apache/impala/common/TreeNode.java
M fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
17 files changed, 103 insertions(+), 92 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/54/11954/6
-- 
To view, visit http://gerrit.cloudera.org:8080/11954
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iebab7dccdb4b2fa0b5ca812beab0e8bdba39f539
Gerrit-Change-Number: 11954
Gerrit-PatchSet: 6
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Alex Rodoni <ar...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>

[Impala-ASF-CR] IMPALA-7867 (Part 1): Expose List in TreeNode, parser

Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.

Hello Fredy Wijaya, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-7867 (Part 1): Expose List in TreeNode, parser
......................................................................

IMPALA-7867 (Part 1): Expose List in TreeNode, parser

When using Java collections, a common Java best practice is to expose
the collection interface, but hide the implementation choice. This
pattern allows us to start with a generic implementation (an ArrayList,
say), but evolve to a more specific implementation to achieve certain
goals (a LinkedList or ImmutableList, say.)

For whatever reason, the Impala FE code exposes ArrayList, HashMap and
other implementation choices as variable types and in method signatures.

Also, since Java 7, the preferred way to create an array is

new ArrayList<>()

Replaced older forms:

new ArrayList<foo>() // Pre-Java 7
Lists.newArrayList() // Guava form, pre-Java 7

This ticket cleans up two files, and their dependencies:

* TreeNode (the root of all parser nodes)
* sql-parser.cup (the code which creates the parser nodes)

Many other uses exist, and will be submitted as separate patches to keep
patches small.

Tests: This is purely a refactoring, no functionality changed. Ran the
FE unit tests to verify no regressions.

Change-Id: Iebab7dccdb4b2fa0b5ca812beab0e8bdba39f539
---
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionName.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/SlotRef.java
M fe/src/main/java/org/apache/impala/analysis/UnionStmt.java
M fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java
M fe/src/main/java/org/apache/impala/analysis/WithClause.java
M fe/src/main/java/org/apache/impala/catalog/StructType.java
M fe/src/main/java/org/apache/impala/common/TreeNode.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
15 files changed, 96 insertions(+), 85 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/54/11954/4
-- 
To view, visit http://gerrit.cloudera.org:8080/11954
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iebab7dccdb4b2fa0b5ca812beab0e8bdba39f539
Gerrit-Change-Number: 11954
Gerrit-PatchSet: 4
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>