[Impala-ASF-CR] IMPALA-7867 (Part 1): Expose List in TreeNode, parser
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 Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Tue, 27 Nov 2018 00:04:56 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7867 (Part 1): Expose List in TreeNode, parser
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() // 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 Tested-by: Impala Public Jenkins --- 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 Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers
[Impala-ASF-CR] IMPALA-7867 (Part 1): Expose List in TreeNode, parser
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 Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Mon, 26 Nov 2018 20:07:39 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7867 (Part 1): Expose List in TreeNode, parser
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 Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Mon, 26 Nov 2018 20:07:40 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7867 (Part 1): Expose List in TreeNode, parser
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 Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Mon, 26 Nov 2018 20:07:01 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7867 (Part 1): Expose List in TreeNode, parser
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 Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Mon, 26 Nov 2018 19:25:24 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7867 (Part 1): Expose List in TreeNode, parser
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 Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Mon, 26 Nov 2018 19:06:18 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7867 (Part 1): Expose List in TreeNode, parser
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11985 ) Change subject: IMPALA-7867 (Part 1): Expose List in TreeNode, parser .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1432/ : 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/11985 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I45e8948c2836283df622ef795aec6f53453bf8b7 Gerrit-Change-Number: 11985 Gerrit-PatchSet: 1 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Mon, 26 Nov 2018 18:45:42 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7867 (Part 1): Expose List in TreeNode, parser
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() // 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 Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers
[Impala-ASF-CR] IMPALA-7867 (Part 1): Expose List in TreeNode, parser
Paul Rogers has abandoned this change. ( http://gerrit.cloudera.org:8080/11985 ) Change subject: IMPALA-7867 (Part 1): Expose List in TreeNode, parser .. Abandoned Incorrect branch -- To view, visit http://gerrit.cloudera.org:8080/11985 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: I45e8948c2836283df622ef795aec6f53453bf8b7 Gerrit-Change-Number: 11985 Gerrit-PatchSet: 1 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-7867 (Part 1): Expose List in TreeNode, parser
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 Turns out it should be Class. 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 Turns out it should be Class. 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 > 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 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 Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Mon, 26 Nov 2018 18:14:25 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7867 (Part 1): Expose List in TreeNode, parser
Paul Rogers has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11985 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() // 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 Fix Change-Id: I45e8948c2836283df622ef795aec6f53453bf8b7 --- 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/85/11985/1 -- To view, visit http://gerrit.cloudera.org:8080/11985 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I45e8948c2836283df622ef795aec6f53453bf8b7 Gerrit-Change-Number: 11985 Gerrit-PatchSet: 1 Gerrit-Owner: Paul Rogers
[Impala-ASF-CR] IMPALA-7867 (Part 1): Expose List in TreeNode, parser
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 4: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1424/ : 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: 4 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Wed, 21 Nov 2018 21:32:14 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7867 (Part 1): Expose List in TreeNode, parser
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 Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Wed, 21 Nov 2018 03:28:56 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7867 (Part 1): Expose List in TreeNode, parser
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 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 Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Wed, 21 Nov 2018 02:39:03 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7867 (Part 1): Expose List in TreeNode, parser
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 Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Tue, 20 Nov 2018 23:05:58 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7867 (Part 1): Expose List in TreeNode, parser
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 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 Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Tue, 20 Nov 2018 22:43:29 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7867 (Part 1): Expose List in TreeNode, parser
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() // 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 Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers
[Impala-ASF-CR] IMPALA-7867, part 1: Expose List in TreeNode, parser
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 2:
(3 comments)
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.
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: parser code {:
Can we clean up the imports as well? I don't think we need Guava Imports
anymore.
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: TreeNode
I don't see the reason for replacing TreeNode with TreeNode.,
especially if TreeNode should always be of NodeType type. Using ? makes the
type system weaker.
--
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: 2
Gerrit-Owner: Paul Rogers
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Paul Rogers
Gerrit-Comment-Date: Tue, 20 Nov 2018 19:39:53 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7867, part 1: Expose List in TreeNode, parser
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 2: This is a simple clean-up patch. No urgency. -- 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: 2 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Tue, 20 Nov 2018 18:41:14 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7867, part 1: Expose List in TreeNode, parser
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() // 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 Gerrit-Reviewer: Impala Public Jenkins
