[Impala-ASF-CR] IMPALA-5973: Provide query plan in JSON format
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11974 ) Change subject: IMPALA-5973: Provide query plan in JSON format .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1420/ : 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/11974 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0f07e2223be58b7323e1ea2fa44b62626cdf4944 Gerrit-Change-Number: 11974 Gerrit-PatchSet: 3 Gerrit-Owner: Pranay Singh (320) Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 21 Nov 2018 05:42:05 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5973: Provide query plan in JSON format
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11974 ) Change subject: IMPALA-5973: Provide query plan in JSON format .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1419/ : 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/11974 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0f07e2223be58b7323e1ea2fa44b62626cdf4944 Gerrit-Change-Number: 11974 Gerrit-PatchSet: 2 Gerrit-Owner: Pranay Singh (320) Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 21 Nov 2018 05:32:48 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5973: Provide query plan in JSON format
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11974 ) Change subject: IMPALA-5973: Provide query plan in JSON format .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1418/ : 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/11974 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0f07e2223be58b7323e1ea2fa44b62626cdf4944 Gerrit-Change-Number: 11974 Gerrit-PatchSet: 1 Gerrit-Owner: Pranay Singh (320) Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 21 Nov 2018 05:22:14 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5973: Provide query plan in JSON format
Hello Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11974 to look at the new patch set (#3). Change subject: IMPALA-5973: Provide query plan in JSON format .. IMPALA-5973: Provide query plan in JSON format EXPLAIN statement provides a query plan in text format, these changes provide an ability to print the query plan in a JSON format. The default option to print the query plan is in text format. In order to print the query plan in JSON format one needs to set the query option EXPLAIN_FORMAT_JSON to true followed by EXPLAIN statement to print the query plan. >set EXPLAIN_FORMAT_JSON=true; >explain select * from tmp_tbl; Change-Id: I0f07e2223be58b7323e1ea2fa44b62626cdf4944 --- M be/src/service/query-options.cc M be/src/service/query-options.h M common/thrift/ImpalaInternalService.thrift M common/thrift/ImpalaService.thrift M fe/src/main/java/org/apache/impala/planner/AggregationNode.java M fe/src/main/java/org/apache/impala/planner/AnalyticEvalNode.java M fe/src/main/java/org/apache/impala/planner/CardinalityCheckNode.java M fe/src/main/java/org/apache/impala/planner/DataSink.java M fe/src/main/java/org/apache/impala/planner/DataSourceScanNode.java M fe/src/main/java/org/apache/impala/planner/DataStreamSink.java M fe/src/main/java/org/apache/impala/planner/EmptySetNode.java M fe/src/main/java/org/apache/impala/planner/ExchangeNode.java M fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java M fe/src/main/java/org/apache/impala/planner/HBaseTableSink.java M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java M fe/src/main/java/org/apache/impala/planner/HdfsTableSink.java M fe/src/main/java/org/apache/impala/planner/JoinBuildSink.java M fe/src/main/java/org/apache/impala/planner/KuduScanNode.java M fe/src/main/java/org/apache/impala/planner/KuduTableSink.java M fe/src/main/java/org/apache/impala/planner/NestedLoopJoinNode.java M fe/src/main/java/org/apache/impala/planner/PlanFragment.java M fe/src/main/java/org/apache/impala/planner/PlanNode.java M fe/src/main/java/org/apache/impala/planner/PlanRootSink.java M fe/src/main/java/org/apache/impala/planner/Planner.java M fe/src/main/java/org/apache/impala/planner/SelectNode.java M fe/src/main/java/org/apache/impala/planner/SingularRowSrcNode.java M fe/src/main/java/org/apache/impala/planner/SortNode.java M fe/src/main/java/org/apache/impala/planner/SubplanNode.java M fe/src/main/java/org/apache/impala/planner/UnionNode.java M fe/src/main/java/org/apache/impala/planner/UnnestNode.java 31 files changed, 322 insertions(+), 78 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/74/11974/3 -- To view, visit http://gerrit.cloudera.org:8080/11974 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0f07e2223be58b7323e1ea2fa44b62626cdf4944 Gerrit-Change-Number: 11974 Gerrit-PatchSet: 3 Gerrit-Owner: Pranay Singh (320) Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-5973: Provide query plan in JSON format
Hello Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11974 to look at the new patch set (#2). Change subject: IMPALA-5973: Provide query plan in JSON format .. IMPALA-5973: Provide query plan in JSON format EXPLAIN statement provides a query plan in text format, these changes provide an ability to print the query plan in a JSON format. The default option to print the query plan is in text format. In order to print the query plan in JSON format one needs to set the query option EXPLAIN_FORMAT_JSON to true followed by EXPLAIN statement to print the query plan. >set EXPLAIN_FORMAT_JSON=true; >explain select * from tmp_tbl; Change-Id: I0f07e2223be58b7323e1ea2fa44b62626cdf4944 --- M be/src/service/query-options.cc M be/src/service/query-options.h M common/thrift/ImpalaInternalService.thrift M common/thrift/ImpalaService.thrift M fe/src/main/java/org/apache/impala/planner/AggregationNode.java M fe/src/main/java/org/apache/impala/planner/AnalyticEvalNode.java M fe/src/main/java/org/apache/impala/planner/CardinalityCheckNode.java M fe/src/main/java/org/apache/impala/planner/DataSink.java M fe/src/main/java/org/apache/impala/planner/DataSourceScanNode.java M fe/src/main/java/org/apache/impala/planner/DataStreamSink.java M fe/src/main/java/org/apache/impala/planner/EmptySetNode.java M fe/src/main/java/org/apache/impala/planner/ExchangeNode.java M fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java M fe/src/main/java/org/apache/impala/planner/HBaseTableSink.java M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java M fe/src/main/java/org/apache/impala/planner/HdfsTableSink.java M fe/src/main/java/org/apache/impala/planner/JoinBuildSink.java M fe/src/main/java/org/apache/impala/planner/KuduScanNode.java M fe/src/main/java/org/apache/impala/planner/KuduTableSink.java M fe/src/main/java/org/apache/impala/planner/NestedLoopJoinNode.java M fe/src/main/java/org/apache/impala/planner/PlanFragment.java M fe/src/main/java/org/apache/impala/planner/PlanNode.java M fe/src/main/java/org/apache/impala/planner/PlanRootSink.java M fe/src/main/java/org/apache/impala/planner/Planner.java M fe/src/main/java/org/apache/impala/planner/SelectNode.java M fe/src/main/java/org/apache/impala/planner/SingularRowSrcNode.java M fe/src/main/java/org/apache/impala/planner/SortNode.java M fe/src/main/java/org/apache/impala/planner/SubplanNode.java M fe/src/main/java/org/apache/impala/planner/UnionNode.java M fe/src/main/java/org/apache/impala/planner/UnnestNode.java 31 files changed, 323 insertions(+), 78 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/74/11974/2 -- To view, visit http://gerrit.cloudera.org:8080/11974 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0f07e2223be58b7323e1ea2fa44b62626cdf4944 Gerrit-Change-Number: 11974 Gerrit-PatchSet: 2 Gerrit-Owner: Pranay Singh (320) Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-5973: Provide query plan in JSON format
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11974 ) Change subject: IMPALA-5973: Provide query plan in JSON format .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/11974/1/fe/src/main/java/org/apache/impala/planner/PlanFragment.java File fe/src/main/java/org/apache/impala/planner/PlanFragment.java: http://gerrit.cloudera.org:8080/#/c/11974/1/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@449 PS1, Line 449: resourceProfile_.multiply(getNumInstancesPerHost(mt_dop)).getExplainString()); line has trailing whitespace -- To view, visit http://gerrit.cloudera.org:8080/11974 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0f07e2223be58b7323e1ea2fa44b62626cdf4944 Gerrit-Change-Number: 11974 Gerrit-PatchSet: 1 Gerrit-Owner: Pranay Singh (320) Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 21 Nov 2018 04:47:46 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5973: Provide query plan in JSON format
Pranay Singh has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11974 Change subject: IMPALA-5973: Provide query plan in JSON format .. IMPALA-5973: Provide query plan in JSON format EXPLAIN statement provides a query plan in text format, these changes provide an ability to print the query plan in a JSON format. The default option to print the query plan is in text format. In order to print the query plan in JSON format one needs to set the query option EXPLAIN_FORMAT_JSON to true followed by EXPLAIN statement to print the query plan. >set EXPLAIN_FORMAT_JSON=true; >explain select * from tmp_tbl; Change-Id: I0f07e2223be58b7323e1ea2fa44b62626cdf4944 --- M be/src/service/query-options.cc M be/src/service/query-options.h M common/thrift/ImpalaInternalService.thrift M common/thrift/ImpalaService.thrift M fe/src/main/java/org/apache/impala/planner/AggregationNode.java M fe/src/main/java/org/apache/impala/planner/AnalyticEvalNode.java M fe/src/main/java/org/apache/impala/planner/CardinalityCheckNode.java M fe/src/main/java/org/apache/impala/planner/DataSink.java M fe/src/main/java/org/apache/impala/planner/DataSourceScanNode.java M fe/src/main/java/org/apache/impala/planner/DataStreamSink.java M fe/src/main/java/org/apache/impala/planner/EmptySetNode.java M fe/src/main/java/org/apache/impala/planner/ExchangeNode.java M fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java M fe/src/main/java/org/apache/impala/planner/HBaseTableSink.java M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java M fe/src/main/java/org/apache/impala/planner/HdfsTableSink.java M fe/src/main/java/org/apache/impala/planner/JoinBuildSink.java M fe/src/main/java/org/apache/impala/planner/KuduScanNode.java M fe/src/main/java/org/apache/impala/planner/KuduTableSink.java M fe/src/main/java/org/apache/impala/planner/NestedLoopJoinNode.java M fe/src/main/java/org/apache/impala/planner/PlanFragment.java M fe/src/main/java/org/apache/impala/planner/PlanNode.java M fe/src/main/java/org/apache/impala/planner/PlanRootSink.java M fe/src/main/java/org/apache/impala/planner/Planner.java M fe/src/main/java/org/apache/impala/planner/SelectNode.java M fe/src/main/java/org/apache/impala/planner/SingularRowSrcNode.java M fe/src/main/java/org/apache/impala/planner/SortNode.java M fe/src/main/java/org/apache/impala/planner/SubplanNode.java M fe/src/main/java/org/apache/impala/planner/UnionNode.java M fe/src/main/java/org/apache/impala/planner/UnnestNode.java 31 files changed, 323 insertions(+), 78 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/74/11974/1 -- To view, visit http://gerrit.cloudera.org:8080/11974 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I0f07e2223be58b7323e1ea2fa44b62626cdf4944 Gerrit-Change-Number: 11974 Gerrit-PatchSet: 1 Gerrit-Owner: Pranay Singh (320)
[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-7844: HAVING clause cannot support ordinals
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/11955 ) Change subject: IMPALA-7844: HAVING clause cannot support ordinals .. Patch Set 2: Change mostly passed the pre-commit test: https://jenkins.impala.io/job/ubuntu-16.04-from-scratch/3640/console A query failed due to a memory-related error, but my speculation is that the error is spurious and not related to this change. Forbidding ordinals should either work (for queries that don't use them), or plainly fail (if we had any that did, as in the FE tests.) -- To view, visit http://gerrit.cloudera.org:8080/11955 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic2b9f9e8c60fe2b25e20c57c2ffc31d8e59d5861 Gerrit-Change-Number: 11955 Gerrit-PatchSet: 2 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Wed, 21 Nov 2018 02:37:24 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7839: Remove code duplication for getting a unique catalog object name
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11928 ) Change subject: IMPALA-7839: Remove code duplication for getting a unique catalog object name .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/11928/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java File fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java: http://gerrit.cloudera.org:8080/#/c/11928/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java@45 PS2, Line 45: return Catalog.toCatalogObjectKey(toTCatalogObject()); > Converting an object to it's thrift struct to get it's unique name seems ki This is a fair point. Initially I thought of doing that, too. Given the current code, I can't think of an elegant way of using CatalogObject in Catalog. Let me know if you have some suggestion. http://gerrit.cloudera.org:8080/#/c/11928/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java@58 PS2, Line 58: protected abstract void writeToCatalogObject(TCatalogObject catalogObject); > Rather than having 2 methods here, how about just overriding toTCatalogObje I know this is the convention that we in some places, however I'm kind of torn with using this style especially since there is mo safe way of enforcing that the subclass needs to call super.toTCatalogObject(). I've seen many places in Impala that uses this style and some subclasses call super and some subclasses just duplicate the code that's already been implemented in the super class. Another issue that I have with this style is there's no compile-time check that says the subclass must override toTCatalogObject(). If there's a new CatalogObject that simply extends CatalogObjectImpl without overriding toTCatalogObject(), the code will still compile fine but it will be broken at runtime. -- To view, visit http://gerrit.cloudera.org:8080/11928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8218e57210c06fe6f2578a792d518e4b0287f15f Gerrit-Change-Number: 11928 Gerrit-PatchSet: 2 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 21 Nov 2018 02:32:03 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7804: Mitigate s3 consistency issues for test scanners
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11959 ) Change subject: IMPALA-7804: Mitigate s3 consistency issues for test_scanners .. Patch Set 3: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/11959 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id042496beabe0d0226b347e0653b820fee369f4e Gerrit-Change-Number: 11959 Gerrit-PatchSet: 3 Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Wed, 21 Nov 2018 01:44:31 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7811: optionally count JVM heap towards process mem limit
Pooja Nilangekar has posted comments on this change. ( http://gerrit.cloudera.org:8080/10928 ) Change subject: IMPALA-7811: optionally count JVM heap towards process mem limit .. Patch Set 8: (5 comments) The approach used to compute the memory utilized and accounting for it in the process memory makes sense. Thanks for adding the test to validate it. http://gerrit.cloudera.org:8080/#/c/10928/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10928/8//COMMIT_MSG@22 PS8, Line 22: row nit: typo http://gerrit.cloudera.org:8080/#/c/10928/8//COMMIT_MSG@25 PS8, Line 25: We should not admit query memory in addition to the JVM memory. Initially, I had some trouble understanding this. Could you please clarify this? http://gerrit.cloudera.org:8080/#/c/10928/8/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: http://gerrit.cloudera.org:8080/#/c/10928/8/be/src/runtime/exec-env.cc@277 PS8, Line 277: post_jvm_bytes_limit -= JvmMemoryMetric::HEAP_MAX_USAGE->GetValue(); Is there a reason for not subtracting the "JvmMemoryMetric::NON_HEAP_COMMITTED" memory from this value? http://gerrit.cloudera.org:8080/#/c/10928/8/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/10928/8/be/src/service/impala-server.cc@1817 PS8, Line 1817: ; nit: extra semicolon http://gerrit.cloudera.org:8080/#/c/10928/8/be/src/service/impala-server.cc@1817 PS8, Line 1817: admit_mem_limit -= JvmMemoryMetric::HEAP_MAX_USAGE->GetValue(); Same concern as above. Is there a reason for not subtracting the "JvmMemoryMetric::NON_HEAP_COMMITTED" memory from this value? -- To view, visit http://gerrit.cloudera.org:8080/10928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39dd715882a32fc986755d573bd46f0fd9eefbfc Gerrit-Change-Number: 10928 Gerrit-PatchSet: 8 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 21 Nov 2018 01:15:42 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7852: Fix some flakiness in test hash join timer.py
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11964 ) Change subject: IMPALA-7852: Fix some flakiness in test_hash_join_timer.py .. Patch Set 3: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/11964/1/tests/beeswax/impala_beeswax.py File tests/beeswax/impala_beeswax.py: http://gerrit.cloudera.org:8080/#/c/11964/1/tests/beeswax/impala_beeswax.py@206 PS1, Line 206: self.close_query(handle) > I rewrote part of test_hash_join_timer.py in PS2 to use the exec_summary fr Thanks. I wonder if we should track the profile issue as a query lifecycle bug - it seems like there isn't necessarily a reliable way to get a final profile for the query. -- To view, visit http://gerrit.cloudera.org:8080/11964 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I851824dffb78c7731e60793d90f1e57050c54955 Gerrit-Change-Number: 11964 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 21 Nov 2018 00:58:24 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7852: Fix some flakiness in test hash join timer.py
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11964 ) Change subject: IMPALA-7852: Fix some flakiness in test_hash_join_timer.py .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1417/ : 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/11964 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I851824dffb78c7731e60793d90f1e57050c54955 Gerrit-Change-Number: 11964 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 21 Nov 2018 00:56:00 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7869: break up parquet-column-readers.cc
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11949 ) Change subject: IMPALA-7869: break up parquet-column-readers.cc .. Patch Set 8: There's a linking issue that I wasn't able to reproduce locally yet. It should be fixable and doesn't affect the rest of the changes so please feel free to continue reviewing. -- To view, visit http://gerrit.cloudera.org:8080/11949 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0efd5c50b781fe9e3c022b33c66c06cfb529c0b8 Gerrit-Change-Number: 11949 Gerrit-PatchSet: 8 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 21 Nov 2018 00:52:31 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7852: Fix some flakiness in test hash join timer.py
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11964 ) Change subject: IMPALA-7852: Fix some flakiness in test_hash_join_timer.py .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1416/ : 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/11964 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I851824dffb78c7731e60793d90f1e57050c54955 Gerrit-Change-Number: 11964 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 21 Nov 2018 00:48:05 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7852: Fix some flakiness in test hash join timer.py
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11964 ) Change subject: IMPALA-7852: Fix some flakiness in test_hash_join_timer.py .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/11964/3/tests/query_test/test_hash_join_timer.py File tests/query_test/test_hash_join_timer.py: http://gerrit.cloudera.org:8080/#/c/11964/3/tests/query_test/test_hash_join_timer.py@92 PS3, Line 92: = flake8: E712 comparison to False should be 'if cond is False:' or 'if not cond:' -- To view, visit http://gerrit.cloudera.org:8080/11964 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I851824dffb78c7731e60793d90f1e57050c54955 Gerrit-Change-Number: 11964 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 21 Nov 2018 00:15:12 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7852: Fix some flakiness in test hash join timer.py
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11964 ) Change subject: IMPALA-7852: Fix some flakiness in test_hash_join_timer.py .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/11964/2/tests/query_test/test_hash_join_timer.py File tests/query_test/test_hash_join_timer.py: http://gerrit.cloudera.org:8080/#/c/11964/2/tests/query_test/test_hash_join_timer.py@148 PS2, Line 148: ; flake8: E703 statement ends with a semicolon -- To view, visit http://gerrit.cloudera.org:8080/11964 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I851824dffb78c7731e60793d90f1e57050c54955 Gerrit-Change-Number: 11964 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 21 Nov 2018 00:11:03 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7852: Fix some flakiness in test hash join timer.py
Hello Thomas Marshall, Lars Volker, Tim Armstrong, Bikramjeet Vig, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11964 to look at the new patch set (#3). Change subject: IMPALA-7852: Fix some flakiness in test_hash_join_timer.py .. IMPALA-7852: Fix some flakiness in test_hash_join_timer.py test_hash_join_timer.py aims to verify the timers in the join nodes are functioning correctly. It does so by parsing the query profile for certain patterns after the query has finished. Before IMPALA-4063, each individual fragment instance will post its profile to the coordinator upon completion. After IMPALA-4063, the profiles of all fragment instances are sent together periodically and the final profile is sent once all fragment instances on a backend are done. The problem with the existing implementation of the test is that it doesn't actually fetch results before closing the query. As a result of it, the coordinator fragment never gets a chance to complete as it will block forever when inserting into the plan root sink. The lack of completion of the coordinator fragment causes the final profiles of fragment instances on the coordinator to be not sent before the query is closed. As a result, the profile of a fragment instance on the coordinator could be stale if it completes between two periodic updates, leading to random test failure. This change fixes the flankiness by always fetching results before closing the query. Ideally, if we fix IMPALA-539 and wait for all backends' final profiles before completing query unregistration, we should get the final profile from the coordinator fragment too. Change-Id: I851824dffb78c7731e60793d90f1e57050c54955 --- M be/src/runtime/query-state.cc M tests/query_test/test_hash_join_timer.py 2 files changed, 47 insertions(+), 51 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/11964/3 -- To view, visit http://gerrit.cloudera.org:8080/11964 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I851824dffb78c7731e60793d90f1e57050c54955 Gerrit-Change-Number: 11964 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-7852: Fix some flakiness in test hash join timer.py
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/11964 ) Change subject: IMPALA-7852: Fix some flakiness in test_hash_join_timer.py .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/11964/2/tests/query_test/test_hash_join_timer.py File tests/query_test/test_hash_join_timer.py: http://gerrit.cloudera.org:8080/#/c/11964/2/tests/query_test/test_hash_join_timer.py@148 PS2, Line 148: ; > flake8: E703 statement ends with a semicolon Done -- To view, visit http://gerrit.cloudera.org:8080/11964 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I851824dffb78c7731e60793d90f1e57050c54955 Gerrit-Change-Number: 11964 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 21 Nov 2018 00:14:36 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7852: Fix some flakiness in test hash join timer.py
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/11964 ) Change subject: IMPALA-7852: Fix some flakiness in test_hash_join_timer.py .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/11964/1/tests/beeswax/impala_beeswax.py File tests/beeswax/impala_beeswax.py: http://gerrit.cloudera.org:8080/#/c/11964/1/tests/beeswax/impala_beeswax.py@206 PS1, Line 206: # Get the profile after unregistering the query so ExecSummary is included in it. > Good point although in practice, given the default size of query log and th I rewrote part of test_hash_join_timer.py in PS2 to use the exec_summary from the result set. Also did a bit of clean up there so there is no need for the change in this file here. -- To view, visit http://gerrit.cloudera.org:8080/11964 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I851824dffb78c7731e60793d90f1e57050c54955 Gerrit-Change-Number: 11964 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 21 Nov 2018 00:11:38 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7852: Fix some flakiness in test hash join timer.py
Hello Thomas Marshall, Lars Volker, Tim Armstrong, Bikramjeet Vig, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11964 to look at the new patch set (#2). Change subject: IMPALA-7852: Fix some flakiness in test_hash_join_timer.py .. IMPALA-7852: Fix some flakiness in test_hash_join_timer.py test_hash_join_timer.py aims to verify the timers in the join nodes are functioning correctly. It does so by parsing the query profile for certain patterns after the query has finished. Before IMPALA-4063, each individual fragment instance will post its profile to the coordinator upon completion. After IMPALA-4063, the profiles of all fragment instances are sent together periodically and the final profile is sent once all fragment instances on a backend are done. The problem with the existing implementation of the test is that it doesn't actually fetch results before closing the query. As a result of it, the coordinator fragment never gets a chance to complete as it will block forever when inserting into the plan root sink. The lack of completion of the coordinator fragment causes the final profiles of fragment instances on the coordinator to be not sent before the query is closed. As a result, the profile of a fragment instance on the coordinator could be stale if it completes between two periodic updates, leading to random test failure. This change fixes the flankiness by always fetching results before closing the query. Ideally, if we fix IMPALA-539 and wait for all backends' final profiles before completing query unregistration, we should get the final profile from the coordinator fragment too. Change-Id: I851824dffb78c7731e60793d90f1e57050c54955 --- M be/src/runtime/query-state.cc M tests/query_test/test_hash_join_timer.py 2 files changed, 43 insertions(+), 47 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/11964/2 -- To view, visit http://gerrit.cloudera.org:8080/11964 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I851824dffb78c7731e60793d90f1e57050c54955 Gerrit-Change-Number: 11964 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-7873: Fix flakiness in TestExchangeMemUsage
Joe McDonnell has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11965 ) Change subject: IMPALA-7873: Fix flakiness in TestExchangeMemUsage .. IMPALA-7873: Fix flakiness in TestExchangeMemUsage IMPALA-7367 reduced the memory requirement of the query tested in test_exchange_mem_usage_scaling. This change reduces the mem limit to ensure that the query runs out of memory as expected. Testing: Ran the test 100 times in a loop without any failures. Change-Id: Ib2f063fb88ebf0c7f994b55ecfc860d81726fdd8 Reviewed-on: http://gerrit.cloudera.org:8080/11965 Reviewed-by: Tim Armstrong Tested-by: Impala Public Jenkins --- M testdata/workloads/functional-query/queries/QueryTest/exchange-mem-scaling.test 1 file changed, 1 insertion(+), 1 deletion(-) Approvals: Tim Armstrong: Looks good to me, approved Impala Public Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/11965 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ib2f063fb88ebf0c7f994b55ecfc860d81726fdd8 Gerrit-Change-Number: 11965 Gerrit-PatchSet: 2 Gerrit-Owner: Pooja Nilangekar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-7873: Fix flakiness in TestExchangeMemUsage
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11965 ) Change subject: IMPALA-7873: Fix flakiness in TestExchangeMemUsage .. Patch Set 1: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/11965 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib2f063fb88ebf0c7f994b55ecfc860d81726fdd8 Gerrit-Change-Number: 11965 Gerrit-PatchSet: 1 Gerrit-Owner: Pooja Nilangekar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 21 Nov 2018 00:07:57 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7829: Mark a fragment instance as done only after Close() is called
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11939 ) Change subject: IMPALA-7829: Mark a fragment instance as done only after Close() is called .. Patch Set 3: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/11939 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I61618854ae3f4e7ef20028dcb0ff5cbcfa8adb01 Gerrit-Change-Number: 11939 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 20 Nov 2018 23:49:10 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7829: Mark a fragment instance as done only after Close() is called
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11939 ) Change subject: IMPALA-7829: Mark a fragment instance as done only after Close() is called .. IMPALA-7829: Mark a fragment instance as done only after Close() is called As shown in IMPALA-7828. there is some non-determinism on whether the errors detected in FragmentInstanceState::Close() will show up in the final profile sent to the coordinator. The reason is that the current code marks a fragment instance as "done" after ExecInternal() completes but before Close() is called. There is a window between when the final status report is sent and when Close() finishes. This change fixes the problem by not sending the final report until Close() is called. This has no implication on the first row available time for normal queries. It may slightly lengthen the first row available time for DML queries. Testing done: Updated udf-no-expr-rewrite.test to exercise this test Perf run on an 8 node clusters didn't show any regression: TPCH-300 ++---+-++++ | Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) | ++---+-++++ | TPCH(_300) | parquet / none / none | 23.94 | -2.05% | 12.55 | -2.62% | ++---+-++++ Small concurrency +-+---+-++++ | Workload| File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) | +-+---+-++++ | TPCDS-UNMODIFIED(_1000) | parquet / none / none | 6.89| -0.66% | 6.62 | +0.41% | +-+---+-++++ Medium concurrency +-+---+-++++ | Workload| File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) | +-+---+-++++ | TPCDS-UNMODIFIED(_1000) | parquet / none / none | 55.57 | -1.04% | 55.27 | -0.98% | +-+---+-++++ Change-Id: I61618854ae3f4e7ef20028dcb0ff5cbcfa8adb01 Reviewed-on: http://gerrit.cloudera.org:8080/11939 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M be/src/runtime/fragment-instance-state.cc M be/src/runtime/fragment-instance-state.h M testdata/workloads/functional-query/queries/QueryTest/udf-no-expr-rewrite.test 3 files changed, 12 insertions(+), 19 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/11939 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I61618854ae3f4e7ef20028dcb0ff5cbcfa8adb01 Gerrit-Change-Number: 11939 Gerrit-PatchSet: 4 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong
[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-7839: Remove code duplication for getting a unique catalog object name
Bharath Vissapragada has posted comments on this change. (
http://gerrit.cloudera.org:8080/11928 )
Change subject: IMPALA-7839: Remove code duplication for getting a unique
catalog object name
..
Patch Set 2:
(2 comments)
Have a couple of questions, want to know your thoughts on them.
http://gerrit.cloudera.org:8080/#/c/11928/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java
File fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java:
http://gerrit.cloudera.org:8080/#/c/11928/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java@45
PS2, Line 45: return Catalog.toCatalogObjectKey(toTCatalogObject());
Converting an object to it's thrift struct to get it's unique name seems kinda
roundabout to me. What do you think?
Also if someone wants to change the unique key for a given class, they need go
through this call chain and change toCatalogObjectKey() in the Catalog class
which is weird.
Ideally I'd expect each CatalogObject to be able to define it's own unique name
impl (override getUniqueName()) and Catalog#toCatalogObjectKey() to rely on the
overriden methods.
http://gerrit.cloudera.org:8080/#/c/11928/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java@58
PS2, Line 58: protected abstract void writeToCatalogObject(TCatalogObject
catalogObject);
Rather than having 2 methods here, how about just overriding toTCatalogObject()
in the base class?
For example for a data source you'd do something like
public final TCatalogObject toTCatalogObject() {
return super.toTCatalogObject().setData_source(toThrift());
}
Don't think there is a way to enforce the overrding part but that appears more
cleaner (we follow that approach for toThrift() overrides)
--
To view, visit http://gerrit.cloudera.org:8080/11928
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8218e57210c06fe6f2578a792d518e4b0287f15f
Gerrit-Change-Number: 11928
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Tue, 20 Nov 2018 22:48:35 +
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 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-7844: HAVING clause cannot support ordinals
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11955 ) Change subject: IMPALA-7844: HAVING clause cannot support ordinals .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1414/ : 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/11955 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic2b9f9e8c60fe2b25e20c57c2ffc31d8e59d5861 Gerrit-Change-Number: 11955 Gerrit-PatchSet: 2 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 20 Nov 2018 22:42:27 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7804: Mitigate s3 consistency issues for test scanners
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/11959 ) Change subject: IMPALA-7804: Mitigate s3 consistency issues for test_scanners .. Patch Set 3: Ran s3 tests three times and didn't see any failures on test_scanners.py tests. -- To view, visit http://gerrit.cloudera.org:8080/11959 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id042496beabe0d0226b347e0653b820fee369f4e Gerrit-Change-Number: 11959 Gerrit-PatchSet: 3 Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Tue, 20 Nov 2018 22:09:19 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7844: HAVING clause cannot support ordinals
Paul Rogers has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/11955 ) Change subject: IMPALA-7844: HAVING clause cannot support ordinals .. IMPALA-7844: HAVING clause cannot support ordinals The SELECT statement has two clauses that take lists of columns and/or aliases: ORDER BY and GROUP BY. Each element is a name, a table.column reference or a number, which represents the ordinal number of a column. Thus, "GROUP BY a, 2, c" is unambiguous. SELECT also has a number of predicate clauses: WHERE and HAVING. In these, aliases are possible (though seldom suppored), but ordinals are ambiguous: is "WHERE 1 = 2" a reference to two constants, two columns by ordinal, or a combination? No SQL dialect supports ordinals in WHERE or HAVING for this reason. Impala seems to have adopted a rather odd convention: if the HAVING predicate has only one element (no opeators), than that one element can be an ordinal or alias. Thus "HAVING a" and "HAVING 1" are valid, only if alias a or the column at ordinal 1 are Boolean. However, "HAVING a = 1" and "HAVING 1 = 2" are not valid. This is odd because HAVING is normally a predicate: "HAVING a = 10". This fix removes the attempt to support ordinals in the HAVING clause, and treats HAVING like WHERE, rather than trying to treat it like ORDER BY or GROUP BY. The fix retains the limited form of alias support. A future change can add full alias support, but must do so inside any valid expression so it works with all column types. That is, "HAVING a = 10" must work if a is an alias. The existing "ordinal or alias" code was a bit messy, and not in good shape to support the eventual goal of integrating rewrites into the analysis phase. Refactored the code to allow resolution of only aliases (for HAVING) and to return whether the result expression needs a rewrite (for future rewrite integration.) Reworded a few error messages for greater clarity. Testing: * Refactored AnalyzeStmtsTest to split apart the alias and ordinals cases for easier debugging. * Disabled the tests for ordinals in HAVING. * Added new tests to verify that integers in HAVING act like (unsupported) integer constants. Change-Id: Ic2b9f9e8c60fe2b25e20c57c2ffc31d8e59d5861 --- M fe/src/main/java/org/apache/impala/analysis/Expr.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/AnalyzeStmtsTest.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java 5 files changed, 306 insertions(+), 132 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/11955/2 -- To view, visit http://gerrit.cloudera.org:8080/11955 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic2b9f9e8c60fe2b25e20c57c2ffc31d8e59d5861 Gerrit-Change-Number: 11955 Gerrit-PatchSet: 2 Gerrit-Owner: Paul Rogers
[Impala-ASF-CR] IMPALA-7804: Mitigate s3 consistency issues for test scanners
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11959 ) Change subject: IMPALA-7804: Mitigate s3 consistency issues for test_scanners .. Patch Set 3: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3482/ DRY_RUN=true -- To view, visit http://gerrit.cloudera.org:8080/11959 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id042496beabe0d0226b347e0653b820fee369f4e Gerrit-Change-Number: 11959 Gerrit-PatchSet: 3 Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Tue, 20 Nov 2018 21:45:58 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7804: Mitigate s3 consistency issues for test scanners
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11959 ) Change subject: IMPALA-7804: Mitigate s3 consistency issues for test_scanners .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1413/ : 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/11959 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id042496beabe0d0226b347e0653b820fee369f4e Gerrit-Change-Number: 11959 Gerrit-PatchSet: 3 Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Tue, 20 Nov 2018 21:40:59 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7804: Mitigate s3 consistency issues for test scanners
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/11959 ) Change subject: IMPALA-7804: Mitigate s3 consistency issues for test_scanners .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/11959/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11959/2//COMMIT_MSG@27 PS2, Line 27: This may improve the : likelihood that the filesystem is consistent by : the time we read it. > The docs at https://docs.aws.amazon.com/AmazonS3/latest/dev/Introduction.ht Based on the s3 tracing, we always do an existence check before we do a write. This puts us in the eventually consistent case. I think one thing we can do if we know the destination won't exist is to get rid of the existence check. That should change us over to read-after-write consistency. S3 tests are definitely going to need more than this change. I'm interested in trying out S3Guard. I haven't tested it, so I'm not sure whether it fixes this issue. S3Guard looks like it helps for listing directories, but we're actually trying to read the file. We'll see if it helps. There might be further changes to read our writes and wait for consistency. http://gerrit.cloudera.org:8080/#/c/11959/2/tests/common/file_utils.py File tests/common/file_utils.py: http://gerrit.cloudera.org:8080/#/c/11959/2/tests/common/file_utils.py@67 PS2, Line 67: command > nit: introducing 'command' variable seems unnecessary, and we don't do that You're right, there's no need. I think this came about from an earlier version where I was trying to work around a long line (which is now no longer long). Fixed. -- To view, visit http://gerrit.cloudera.org:8080/11959 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id042496beabe0d0226b347e0653b820fee369f4e Gerrit-Change-Number: 11959 Gerrit-PatchSet: 2 Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Tue, 20 Nov 2018 21:01:43 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7804: Mitigate s3 consistency issues for test scanners
Hello Zoltan Borok-Nagy, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11959 to look at the new patch set (#3). Change subject: IMPALA-7804: Mitigate s3 consistency issues for test_scanners .. IMPALA-7804: Mitigate s3 consistency issues for test_scanners test_scanners.py has seen several flaky failures on s3 due to eventual consistency. The symptom is Impala being unable to read a file that it just loaded to s3. A large number of tables used in test_scanners.py use the file_utils helper functions for creating the tables. These follow the pattern: 1. Copy files to temporary directory in HDFS/S3/etc 2. Create table 3. Run LOAD DATA to move the files to the table In step #3, LOAD DATA gets the metadata for the table before it runs the move statement on the files. Subsequent queries on the table will not need to reload metadata and can access the file quickly after the move. This changes the ordering to put the files in place before loading metadata. This may improve the likelihood that the filesystem is consistent by the time we read it. Specifically, we now do: 1. Put the files in directory that the table will use when it is created. 2. Create table Neither of these steps load metadata, so the next query that runs will load metadata. Change-Id: Id042496beabe0d0226b347e0653b820fee369f4e --- M tests/common/file_utils.py 1 file changed, 24 insertions(+), 13 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/59/11959/3 -- To view, visit http://gerrit.cloudera.org:8080/11959 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id042496beabe0d0226b347e0653b820fee369f4e Gerrit-Change-Number: 11959 Gerrit-PatchSet: 3 Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-7804: Mitigate s3 consistency issues for test scanners
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/11959 ) Change subject: IMPALA-7804: Mitigate s3 consistency issues for test_scanners .. Patch Set 3: Code-Review+1 Carry +1 -- To view, visit http://gerrit.cloudera.org:8080/11959 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id042496beabe0d0226b347e0653b820fee369f4e Gerrit-Change-Number: 11959 Gerrit-PatchSet: 3 Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Tue, 20 Nov 2018 21:01:57 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7873: Fix flakiness in TestExchangeMemUsage
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11965 ) Change subject: IMPALA-7873: Fix flakiness in TestExchangeMemUsage .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1412/ : 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/11965 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib2f063fb88ebf0c7f994b55ecfc860d81726fdd8 Gerrit-Change-Number: 11965 Gerrit-PatchSet: 1 Gerrit-Owner: Pooja Nilangekar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 20 Nov 2018 20:29:32 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7823: Clean up Java warnings, fix minor issues
Fredy Wijaya has posted comments on this change. (
http://gerrit.cloudera.org:8080/11893 )
Change subject: IMPALA-7823: Clean up Java warnings, fix minor issues
..
Patch Set 5:
(28 comments)
My general comments:
- We should aim to fix the warnings instead of suppressing them whenever
possible.
- Possible enable -Werror to fail on warnings in the future.
- There are many possible warnings (and logic bugs) by running IntelliJ code
analyzer that are not covered by this CR and I don't think it should be covered
by this CR either. It's a good idea to break this CR into multiple pieces to
make it easier to review.
- Inconsistent usage of log4j and slf4j. We should decide whether we want to
use log4j or slf4j in FE, but not both.
http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:
http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@1935
PS5, Line 1935:
Java 8 type inference is a lot smarter, it can infer the type. Thus, this can
be simplified to new Pair<>(id1, id2)
http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/analysis/KuduPartitionExpr.java
File fe/src/main/java/org/apache/impala/analysis/KuduPartitionExpr.java:
http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/analysis/KuduPartitionExpr.java@37
PS5, Line 37:
nit: remove extra new line. Our convention (although not universal) is to not
have a new line after a class declaration.
http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java
File fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java:
http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java@112
PS5, Line 112:
nit: remove extra new line.
http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/catalog/CatalogException.java
File fe/src/main/java/org/apache/impala/catalog/CatalogException.java:
http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/catalog/CatalogException.java@27
PS5, Line 27:
nit: remove extra line
http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
File fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java:
http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@663
PS5, Line 663: Boolean
Use Reference<>(false) instead.
http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/planner/JoinNode.java
File fe/src/main/java/org/apache/impala/planner/JoinNode.java:
http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/planner/JoinNode.java@551
PS5, Line 551: @SuppressWarnings("incomplete-switch")
Should we use default: break instead? This way the intention is clear and there
is no need to suppress the warning.
http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
File fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java:
http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@411
PS5, Line 411: new Pair
use new Pair<> form instead
http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@420
PS5, Line 420: new Pair
use new Pair<> form instead
http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java
File fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java:
http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java@37
PS5, Line 37: public static ExprRewriteRule INSTANCE = new
BetweenToCompoundRule();
this should be final
http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java@38
PS5, Line 38:
also add private constructor to prevent instantiation.
http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java@50
PS5, Line 50: } else {
: // Rewrite into conjunction.
: Predicate lower = new
BinaryPredicate(BinaryPredicate.Operator.GE,
: bp.getChild(0), bp.getChild(1));
: Predicate upper = new
BinaryPredicate(BinaryPredicate.Operator.LE,
: bp.getChild(0), bp.getChild(2));
: return new
CompoundPredicate(CompoundPredicate.Operator.AND, lower, upper);
: }
I don't know what's the convention in Impala, but I know in a different
programming language the preferred style is to not have else it's redundant.
[Impala-ASF-CR] IMPALA-7815: [DOCS] Release notes for 3.1
Alex Rodoni has posted comments on this change. ( http://gerrit.cloudera.org:8080/11922 ) Change subject: IMPALA-7815: [DOCS] Release notes for 3.1 .. Patch Set 4: Zoltan, Could you review these release related docs for 3.1? Thank you! -- To view, visit http://gerrit.cloudera.org:8080/11922 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib4d124557f29e539f97c5c52606c16f4147ab169 Gerrit-Change-Number: 11922 Gerrit-PatchSet: 4 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Tue, 20 Nov 2018 20:21:45 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7873: Fix flakiness in TestExchangeMemUsage
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11965 ) Change subject: IMPALA-7873: Fix flakiness in TestExchangeMemUsage .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11965 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib2f063fb88ebf0c7f994b55ecfc860d81726fdd8 Gerrit-Change-Number: 11965 Gerrit-PatchSet: 1 Gerrit-Owner: Pooja Nilangekar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 20 Nov 2018 20:16:20 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7795: Implement REFRESH AUTHORIZATION statement
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11888 ) Change subject: IMPALA-7795: Implement REFRESH AUTHORIZATION statement .. Patch Set 7: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1410/ : 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/11888 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5459e1c97b12dee307e0cf85b94a9f66fd9d9a8c Gerrit-Change-Number: 11888 Gerrit-PatchSet: 7 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Tue, 20 Nov 2018 19:59:02 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7873: Fix flakiness in TestExchangeMemUsage
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11965 ) Change subject: IMPALA-7873: Fix flakiness in TestExchangeMemUsage .. Patch Set 1: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3481/ DRY_RUN=true -- To view, visit http://gerrit.cloudera.org:8080/11965 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib2f063fb88ebf0c7f994b55ecfc860d81726fdd8 Gerrit-Change-Number: 11965 Gerrit-PatchSet: 1 Gerrit-Owner: Pooja Nilangekar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 20 Nov 2018 19:54:34 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7873: Fix flakiness in TestExchangeMemUsage
Pooja Nilangekar has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11965 Change subject: IMPALA-7873: Fix flakiness in TestExchangeMemUsage .. IMPALA-7873: Fix flakiness in TestExchangeMemUsage IMPALA-7367 reduced the memory requirement of the query tested in test_exchange_mem_usage_scaling. This change reduces the mem limit to ensure that the query runs out of memory as expected. Testing: Ran the test 100 times in a loop without any failures. Change-Id: Ib2f063fb88ebf0c7f994b55ecfc860d81726fdd8 --- M testdata/workloads/functional-query/queries/QueryTest/exchange-mem-scaling.test 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/65/11965/1 -- To view, visit http://gerrit.cloudera.org:8080/11965 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ib2f063fb88ebf0c7f994b55ecfc860d81726fdd8 Gerrit-Change-Number: 11965 Gerrit-PatchSet: 1 Gerrit-Owner: Pooja Nilangekar Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-7829: Mark a fragment instance as done only after Close() is called
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11939 ) Change subject: IMPALA-7829: Mark a fragment instance as done only after Close() is called .. Patch Set 3: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3480/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/11939 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I61618854ae3f4e7ef20028dcb0ff5cbcfa8adb01 Gerrit-Change-Number: 11939 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 20 Nov 2018 19:47:45 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7829: Mark a fragment instance as done only after Close() is called
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11939 ) Change subject: IMPALA-7829: Mark a fragment instance as done only after Close() is called .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11939 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I61618854ae3f4e7ef20028dcb0ff5cbcfa8adb01 Gerrit-Change-Number: 11939 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 20 Nov 2018 19:47:44 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7829: Mark a fragment instance as done only after Close() is called
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/11939 ) Change subject: IMPALA-7829: Mark a fragment instance as done only after Close() is called .. Patch Set 2: Code-Review+2 Carry +2 -- To view, visit http://gerrit.cloudera.org:8080/11939 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I61618854ae3f4e7ef20028dcb0ff5cbcfa8adb01 Gerrit-Change-Number: 11939 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 20 Nov 2018 19:45:05 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7829: Mark a fragment instance as done only after Close() is called
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/11939 ) Change subject: IMPALA-7829: Mark a fragment instance as done only after Close() is called .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/11939/1/be/src/runtime/fragment-instance-state.cc File be/src/runtime/fragment-instance-state.cc: http://gerrit.cloudera.org:8080/#/c/11939/1/be/src/runtime/fragment-instance-state.cc@100 PS1, Line 100: ReleaseThreadToken(); > Consider moving to Close()? It's acquired in Prepare() so that feels symmet Done -- To view, visit http://gerrit.cloudera.org:8080/11939 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I61618854ae3f4e7ef20028dcb0ff5cbcfa8adb01 Gerrit-Change-Number: 11939 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 20 Nov 2018 19:44:54 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7829: Mark a fragment instance as done only after Close() is called
Hello Thomas Marshall, Tim Armstrong, Bikramjeet Vig, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11939 to look at the new patch set (#2). Change subject: IMPALA-7829: Mark a fragment instance as done only after Close() is called .. IMPALA-7829: Mark a fragment instance as done only after Close() is called As shown in IMPALA-7828. there is some non-determinism on whether the errors detected in FragmentInstanceState::Close() will show up in the final profile sent to the coordinator. The reason is that the current code marks a fragment instance as "done" after ExecInternal() completes but before Close() is called. There is a window between when the final status report is sent and when Close() finishes. This change fixes the problem by not sending the final report until Close() is called. This has no implication on the first row available time for normal queries. It may slightly lengthen the first row available time for DML queries. Testing done: Updated udf-no-expr-rewrite.test to exercise this test Perf run on an 8 node clusters didn't show any regression: TPCH-300 ++---+-++++ | Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) | ++---+-++++ | TPCH(_300) | parquet / none / none | 23.94 | -2.05% | 12.55 | -2.62% | ++---+-++++ Small concurrency +-+---+-++++ | Workload| File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) | +-+---+-++++ | TPCDS-UNMODIFIED(_1000) | parquet / none / none | 6.89| -0.66% | 6.62 | +0.41% | +-+---+-++++ Medium concurrency +-+---+-++++ | Workload| File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) | +-+---+-++++ | TPCDS-UNMODIFIED(_1000) | parquet / none / none | 55.57 | -1.04% | 55.27 | -0.98% | +-+---+-++++ Change-Id: I61618854ae3f4e7ef20028dcb0ff5cbcfa8adb01 --- M be/src/runtime/fragment-instance-state.cc M be/src/runtime/fragment-instance-state.h M testdata/workloads/functional-query/queries/QueryTest/udf-no-expr-rewrite.test 3 files changed, 12 insertions(+), 19 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/39/11939/2 -- To view, visit http://gerrit.cloudera.org:8080/11939 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I61618854ae3f4e7ef20028dcb0ff5cbcfa8adb01 Gerrit-Change-Number: 11939 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong
[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-7852: Fix some flakiness in test hash join timer.py
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11964 ) Change subject: IMPALA-7852: Fix some flakiness in test_hash_join_timer.py .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1409/ : 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/11964 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I851824dffb78c7731e60793d90f1e57050c54955 Gerrit-Change-Number: 11964 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 20 Nov 2018 19:35:39 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7795: Implement REFRESH AUTHORIZATION statement
Fredy Wijaya has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/11888 ) Change subject: IMPALA-7795: Implement REFRESH AUTHORIZATION statement .. IMPALA-7795: Implement REFRESH AUTHORIZATION statement This patch implements REFRESH AUTHORIZATION statement to explicitly refresh authorization metadata. This statement is useful to force Impala to refresh its authorization metadata when there is an external update to authorization metadata without having to wait for the Sentry polling or call INVALIDATE METADATA. Some tests were updated to use REFRESH AUTHORIZATION instead of INVALIDATE METADATA to make the tests run faster. Syntax: REFRESH AUTHORIZATION (authorization must be enabled to execute this statement) Testing: - Added new FE tests - Added new E2E authorization tests - Ran all FE tests - Ran all E2E authorization tests Change-Id: I5459e1c97b12dee307e0cf85b94a9f66fd9d9a8c --- M common/thrift/CatalogService.thrift M fe/src/main/cup/sql-parser.cup M fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/main/jflex/sql-scanner.flex M fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java M fe/src/test/java/org/apache/impala/analysis/ParserTest.java M fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java M tests/authorization/test_authorization.py M tests/authorization/test_grant_revoke.py M tests/authorization/test_owner_privileges.py M tests/common/sentry_cache_test_suite.py 16 files changed, 259 insertions(+), 90 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/88/11888/7 -- To view, visit http://gerrit.cloudera.org:8080/11888 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5459e1c97b12dee307e0cf85b94a9f66fd9d9a8c Gerrit-Change-Number: 11888 Gerrit-PatchSet: 7 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger
[Impala-ASF-CR] IMPALA-7795: Implement REFRESH AUTHORIZATION statement
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11888 ) Change subject: IMPALA-7795: Implement REFRESH AUTHORIZATION statement .. Patch Set 7: (5 comments) http://gerrit.cloudera.org:8080/#/c/11888/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/11888/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3358 PS6, Line 3358:* 4) perform a refresh on authorization metadata. > 'refresh authorization' does not match any of these three items, so it coul Done http://gerrit.cloudera.org:8080/#/c/11888/6/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java File fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java: http://gerrit.cloudera.org:8080/#/c/11888/6/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@1470 PS6, Line 1470: : /** :* Test admin functions are output correctly. :*/ > This is done in more than places. A function like createTestAuthConfig coul Done http://gerrit.cloudera.org:8080/#/c/11888/6/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@1474 PS6, Line 1474: > createAnalysisCtx has an overload that expects a single AuthorizationConfig Done http://gerrit.cloudera.org:8080/#/c/11888/6/tests/authorization/test_authorization.py File tests/authorization/test_authorization.py: http://gerrit.cloudera.org:8080/#/c/11888/6/tests/authorization/test_authorization.py@541 PS6, Line 541: on > typo: one? Done http://gerrit.cloudera.org:8080/#/c/11888/6/tests/authorization/test_authorization.py@556 PS6, Line 556: s > 'unique_role' could also get a cleanup method similarly to 'unique_database Initially I thought of doing that, but creating a role requires Sentry to be enabled, the user needs to be part of Sentry admin, which can complicate things. -- To view, visit http://gerrit.cloudera.org:8080/11888 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5459e1c97b12dee307e0cf85b94a9f66fd9d9a8c Gerrit-Change-Number: 11888 Gerrit-PatchSet: 7 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Tue, 20 Nov 2018 19:32:06 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7233: [DOCS] Support for IANA timezone database
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11946 ) Change subject: IMPALA-7233: [DOCS] Support for IANA timezone database .. Patch Set 3: Verified+1 Build Successful https://jenkins.impala.io/job/gerrit-docs-auto-test/156/ : Doc tests passed. -- To view, visit http://gerrit.cloudera.org:8080/11946 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id400cda5a1be321063d17e0ee6337e92a5da732a Gerrit-Change-Number: 11946 Gerrit-PatchSet: 3 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Zoltan Ivanfi Gerrit-Comment-Date: Tue, 20 Nov 2018 19:31:29 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7823: Clean up Java warnings, fix minor issues
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11893 ) Change subject: IMPALA-7823: Clean up Java warnings, fix minor issues .. Patch Set 5: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1408/ : 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/11893 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4b174d6448371c55e98487c6d4212c74c6b0c79e Gerrit-Change-Number: 11893 Gerrit-PatchSet: 5 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Tue, 20 Nov 2018 19:28:52 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7233: [DOCS] Support for IANA timezone database
Hello Attila Jeges, Zoltan Ivanfi, Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11946 to look at the new patch set (#3). Change subject: IMPALA-7233: [DOCS] Support for IANA timezone database .. IMPALA-7233: [DOCS] Support for IANA timezone database - Updated the timezone section - Added the sections on customizing timezone db and aliases Change-Id: Id400cda5a1be321063d17e0ee6337e92a5da732a --- M docs/impala.ditamap A docs/topics/impala_custom_timezones.xml M docs/topics/impala_timestamp.xml 3 files changed, 342 insertions(+), 296 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/46/11946/3 -- To view, visit http://gerrit.cloudera.org:8080/11946 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id400cda5a1be321063d17e0ee6337e92a5da732a Gerrit-Change-Number: 11946 Gerrit-PatchSet: 3 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Zoltan Ivanfi
[Impala-ASF-CR] IMPALA-7852: Fix some flakiness in test hash join timer.py
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/11964 ) Change subject: IMPALA-7852: Fix some flakiness in test_hash_join_timer.py .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/11964/1/tests/beeswax/impala_beeswax.py File tests/beeswax/impala_beeswax.py: http://gerrit.cloudera.org:8080/#/c/11964/1/tests/beeswax/impala_beeswax.py@206 PS1, Line 206: # Get the profile after unregistering the query so ExecSummary is included in it. > I think this is introducing a new race between the query being evicted from Good point although in practice, given the default size of query log and the size of the race window, it's unlikely to occur. Let me add a check in the test_hash_join_timer.py to make sure we check if the profile is not None. -- To view, visit http://gerrit.cloudera.org:8080/11964 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I851824dffb78c7731e60793d90f1e57050c54955 Gerrit-Change-Number: 11964 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 20 Nov 2018 19:24:43 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7233: [DOCS] Support for IANA timezone database
Alex Rodoni has posted comments on this change. ( http://gerrit.cloudera.org:8080/11946 ) Change subject: IMPALA-7233: [DOCS] Support for IANA timezone database .. Patch Set 1: (1 comment) > (1 comment) Reworded http://gerrit.cloudera.org:8080/#/c/11946/1/docs/topics/impala_timestamp.xml File docs/topics/impala_timestamp.xml: http://gerrit.cloudera.org:8080/#/c/11946/1/docs/topics/impala_timestamp.xml@333 PS1, Line 333: values the same way it stores : without any adjustment. > This sentence seems to contradict the description below. Hive 2.x _does_ ad This paragraph is about Text file, and the next paragraph is about Parquet files. The incompatibility only exists in Parquet files, right? -- To view, visit http://gerrit.cloudera.org:8080/11946 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id400cda5a1be321063d17e0ee6337e92a5da732a Gerrit-Change-Number: 11946 Gerrit-PatchSet: 1 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Zoltan Ivanfi Gerrit-Comment-Date: Tue, 20 Nov 2018 19:24:07 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7852: Fix some flakiness in test hash join timer.py
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11964 ) Change subject: IMPALA-7852: Fix some flakiness in test_hash_join_timer.py .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/11964/1/tests/beeswax/impala_beeswax.py File tests/beeswax/impala_beeswax.py: http://gerrit.cloudera.org:8080/#/c/11964/1/tests/beeswax/impala_beeswax.py@206 PS1, Line 206: # Get the profile after unregistering the query so ExecSummary is included in it. I think this is introducing a new race between the query being evicted from the query log and fetching the profile. Maybe this is a fundamental problem with the query lifecycle? -- To view, visit http://gerrit.cloudera.org:8080/11964 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I851824dffb78c7731e60793d90f1e57050c54955 Gerrit-Change-Number: 11964 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 20 Nov 2018 19:00:21 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7823: Clean up Java warnings, fix minor issues
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/11893 ) Change subject: IMPALA-7823: Clean up Java warnings, fix minor issues .. Patch Set 5: Rebased on master. This one is pure clean-up. A nice-to-have, but low priority. -- To view, visit http://gerrit.cloudera.org:8080/11893 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4b174d6448371c55e98487c6d4212c74c6b0c79e Gerrit-Change-Number: 11893 Gerrit-PatchSet: 5 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Anonymous Coward (168) Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Tue, 20 Nov 2018 18:59:14 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7823: Clean up Java warnings, fix minor issues
Paul Rogers has removed Anonymous Coward (168) from this change. ( http://gerrit.cloudera.org:8080/11893 ) Change subject: IMPALA-7823: Clean up Java warnings, fix minor issues .. Removed reviewer null. -- To view, visit http://gerrit.cloudera.org:8080/11893 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: I4b174d6448371c55e98487c6d4212c74c6b0c79e Gerrit-Change-Number: 11893 Gerrit-PatchSet: 5 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers
[Impala-ASF-CR] IMPALA-7852: Fix some flakiness in test hash join timer.py
Michael Ho has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11964 Change subject: IMPALA-7852: Fix some flakiness in test_hash_join_timer.py .. IMPALA-7852: Fix some flakiness in test_hash_join_timer.py test_hash_join_timer.py aims to verify the timers in the join nodes are functioning correctly. It does so by parsing the query profile for certain patterns after the query has finished. Before IMPALA-4063, each individual fragment instance will post its profile to the coordinator upon completion. After IMPALA-4063, the profiles of all fragment instances are sent together periodically and the final profile is sent once all fragment instances on a backend are done. The problem with the existing implementation of the test is that it doesn't actually fetch results before closing the query. As a result of it, the coordinator fragment never gets a chance to complete as it will block forever when inserting into the plan root sink. The lack of completion of the coordinator fragment causes the final profiles of fragment instances on the coordinator to be not sent before the query is closed. As a result, the profile of a fragment instance on the coordinator could be stale if it completes between two periodic updates, leading to random test failure. This change fixes the flankiness by always fetching results before closing the query. Ideally, if we fix IMPALA-539 and wait for all backends' final profiles before completing query unregistration, we should get the final profile from the coordinator fragment too. Change-Id: I851824dffb78c7731e60793d90f1e57050c54955 --- M be/src/runtime/query-state.cc M tests/beeswax/impala_beeswax.py M tests/query_test/test_hash_join_timer.py 3 files changed, 9 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/11964/1 -- To view, visit http://gerrit.cloudera.org:8080/11964 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I851824dffb78c7731e60793d90f1e57050c54955 Gerrit-Change-Number: 11964 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho
[Impala-ASF-CR] IMPALA-7823: Clean up Java warnings, fix minor issues
Hello Bharath Vissapragada, Anonymous Coward (168), Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11893 to look at the new patch set (#5). Change subject: IMPALA-7823: Clean up Java warnings, fix minor issues .. IMPALA-7823: Clean up Java warnings, fix minor issues Roll-up of trivial Java fixes: for warnings, formatting, toString() methods. Done as a separate patch to keep others small and focused. Testing: no functional changes. Ran full tests to check for regressions. Change-Id: I4b174d6448371c55e98487c6d4212c74c6b0c79e --- M .gitignore M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java M fe/src/main/java/org/apache/impala/analysis/CreateFunctionStmtBase.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/KuduPartitionExpr.java M fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java M fe/src/main/java/org/apache/impala/analysis/ParquetHelper.java M fe/src/main/java/org/apache/impala/analysis/PlanHint.java M fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java M fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java M fe/src/main/java/org/apache/impala/catalog/AuthorizationException.java M fe/src/main/java/org/apache/impala/catalog/CatalogDeltaLog.java M fe/src/main/java/org/apache/impala/catalog/CatalogException.java M fe/src/main/java/org/apache/impala/catalog/CatalogObjectCache.java M fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java M fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java M fe/src/main/java/org/apache/impala/catalog/HdfsPartitionLocationCompressor.java M fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java M fe/src/main/java/org/apache/impala/catalog/PartitionStatsUtil.java M fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java M fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java M fe/src/main/java/org/apache/impala/planner/AggregationNode.java M fe/src/main/java/org/apache/impala/planner/DataPartition.java M fe/src/main/java/org/apache/impala/planner/JoinNode.java M fe/src/main/java/org/apache/impala/planner/PlanFragment.java M fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java M fe/src/main/java/org/apache/impala/planner/ValueRange.java M fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java M fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java M fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java M fe/src/main/java/org/apache/impala/rewrite/NormalizeExprsRule.java M fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java M fe/src/main/java/org/apache/impala/rewrite/SimplifyDistinctFromRule.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/main/java/org/apache/impala/service/FeSupport.java M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/main/java/org/apache/impala/service/JniCatalog.java M fe/src/main/java/org/apache/impala/service/JniFrontend.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java M fe/src/test/java/org/apache/impala/analysis/ParserTest.java M fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java M fe/src/test/java/org/apache/impala/planner/PlannerTest.java 50 files changed, 56 insertions(+), 124 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/11893/5 -- To view, visit http://gerrit.cloudera.org:8080/11893 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4b174d6448371c55e98487c6d4212c74c6b0c79e Gerrit-Change-Number: 11893 Gerrit-PatchSet: 5 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Anonymous Coward (168) Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers
[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
[Impala-ASF-CR] IMPALA-7869: break up parquet-column-readers.cc
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11949 ) Change subject: IMPALA-7869: break up parquet-column-readers.cc .. Patch Set 8: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/1407/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/11949 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0efd5c50b781fe9e3c022b33c66c06cfb529c0b8 Gerrit-Change-Number: 11949 Gerrit-PatchSet: 8 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 20 Nov 2018 18:05:13 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7795: Implement REFRESH AUTHORIZATION statement
Csaba Ringhofer has posted comments on this change. (
http://gerrit.cloudera.org:8080/11888 )
Change subject: IMPALA-7795: Implement REFRESH AUTHORIZATION statement
..
Patch Set 6:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/11888/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/11888/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3358
PS6, Line 3358:*
'refresh authorization' does not match any of these three items, so it could be
added as fourth one.
http://gerrit.cloudera.org:8080/#/c/11888/6/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
File fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java:
http://gerrit.cloudera.org:8080/#/c/11888/6/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@1470
PS6, Line 1470: AuthorizationConfig authzConfig =
AuthorizationConfig.createHadoopGroupAuthConfig(
: "server1", null, System.getenv("IMPALA_HOME") +
: "/fe/src/test/resources/sentry-site.xml");
: authzConfig.validateConfig();
This is done in more than places. A function like createTestAuthConfig could be
added to FrontendTestBase to avoid the duplication.
http://gerrit.cloudera.org:8080/#/c/11888/6/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@1474
PS6, Line 1474: System.getProperty("user.name")
createAnalysisCtx has an overload that expects a single AuthorizationConfig and
uses user.name as username.
http://gerrit.cloudera.org:8080/#/c/11888/6/tests/authorization/test_authorization.py
File tests/authorization/test_authorization.py:
http://gerrit.cloudera.org:8080/#/c/11888/6/tests/authorization/test_authorization.py@541
PS6, Line 541: on
typo: one?
http://gerrit.cloudera.org:8080/#/c/11888/6/tests/authorization/test_authorization.py@556
PS6, Line 556: s
'unique_role' could also get a cleanup method similarly to 'unique_database' to
avoid this try/finally blocks.
--
To view, visit http://gerrit.cloudera.org:8080/11888
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5459e1c97b12dee307e0cf85b94a9f66fd9d9a8c
Gerrit-Change-Number: 11888
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Comment-Date: Tue, 20 Nov 2018 17:56:59 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7869: break up parquet-column-readers.cc
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11949 ) Change subject: IMPALA-7869: break up parquet-column-readers.cc .. Patch Set 7: (4 comments) http://gerrit.cloudera.org:8080/#/c/11949/7/be/src/exec/parquet-bool-decoder.h File be/src/exec/parquet-bool-decoder.h: http://gerrit.cloudera.org:8080/#/c/11949/7/be/src/exec/parquet-bool-decoder.h@17 PS7, Line 17: > We have several parquet_* files in exec/ - it would be nice to move this to Yeah I thought about this too - the number of files under exec/ has gotten a bit unmanagable. Anyway I just went ahead and did it. Git does have some ability to detect moves and handle the rebase automatically so hopefully it won't be too disruptive. http://gerrit.cloudera.org:8080/#/c/11949/7/be/src/exec/parquet-column-readers.cc File be/src/exec/parquet-column-readers.cc: http://gerrit.cloudera.org:8080/#/c/11949/7/be/src/exec/parquet-column-readers.cc@74 PS7, Line 74: i > Where is this new variable used? It's used in an macro defined in the header. Added comment. http://gerrit.cloudera.org:8080/#/c/11949/7/be/src/exec/parquet-column-readers.cc@284 PS7, Line 284: /// The size of this column with plain encoding for FIXED_LEN_BYTE_ARRAY, or : /// the max length for VARCHAR columns. Unused otherwise. : int fixed_len_size_; : : /// Contains extra data needed for Timestamp decoding. : ParquetTimestampDecoder timestamp_decoder_; : : /// Contains extra state required to decode boolean values. Only initialised for : /// BOOLEAN columns. : unique_ptr bool_decoder_; > A note about members that are used only in specific types: I have a plan to Yeah I think using a templated implementation makes sense. http://gerrit.cloudera.org:8080/#/c/11949/7/be/src/exec/parquet-level-decoder.h File be/src/exec/parquet-level-decoder.h: http://gerrit.cloudera.org:8080/#/c/11949/7/be/src/exec/parquet-level-decoder.h@122 PS7, Line 122: /// The parquet encoding used for the levels. Only RLE is supported for now. : parquet::Encoding::type encoding_ = parquet::Encoding::PLAIN; > This was not changed, but looks a bit weird (only RLE is supported, that it That's a good point. Removed encoding_ and updated comments. -- To view, visit http://gerrit.cloudera.org:8080/11949 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0efd5c50b781fe9e3c022b33c66c06cfb529c0b8 Gerrit-Change-Number: 11949 Gerrit-PatchSet: 7 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 20 Nov 2018 17:41:59 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7869: break up parquet-column-readers.cc
Impala Public Jenkins has posted comments on this change. (
http://gerrit.cloudera.org:8080/11949 )
Change subject: IMPALA-7869: break up parquet-column-readers.cc
..
Patch Set 8:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/11949/8/be/src/exec/parquet/hdfs-parquet-scanner-test.cc
File be/src/exec/parquet/hdfs-parquet-scanner-test.cc:
http://gerrit.cloudera.org:8080/#/c/11949/8/be/src/exec/parquet/hdfs-parquet-scanner-test.cc@111
PS8, Line 111: void HdfsParquetScannerTest::TestDivideReservation(const
vector& col_range_lengths,
line too long (92 > 90)
http://gerrit.cloudera.org:8080/#/c/11949/8/be/src/exec/parquet/hdfs-parquet-scanner.cc
File be/src/exec/parquet/hdfs-parquet-scanner.cc:
http://gerrit.cloudera.org:8080/#/c/11949/8/be/src/exec/parquet/hdfs-parquet-scanner.cc@304
PS8, Line 304: int HdfsParquetScanner::CountScalarColumns(const
vector& column_readers) {
line too long (96 > 90)
http://gerrit.cloudera.org:8080/#/c/11949/8/be/src/exec/parquet/hdfs-parquet-scanner.cc@1388
PS8, Line 1388:
RETURN_IF_ERROR(CreateCountingReader(tuple_desc.tuple_path(), schema_resolver,
));
line too long (93 > 90)
http://gerrit.cloudera.org:8080/#/c/11949/8/be/src/exec/parquet/hdfs-parquet-scanner.cc@1563
PS8, Line 1563: [_range_lengths](const pair& left,
const pair& right) {
line too long (93 > 90)
http://gerrit.cloudera.org:8080/#/c/11949/8/be/src/exec/parquet/hdfs-parquet-scanner.cc@1601
PS8, Line 1601: bytes_to_add = max(min_buffer_size,
BitUtil::RoundUpToPowerOfTwo(bytes_left_in_range));
line too long (95 > 90)
http://gerrit.cloudera.org:8080/#/c/11949/8/be/src/exec/parquet/hdfs-parquet-scanner.cc@1627
PS8, Line 1627: const vector& column_readers, int
row_group_idx, int64_t rows_read) {
line too long (95 > 90)
http://gerrit.cloudera.org:8080/#/c/11949/8/be/src/exec/parquet/parquet-column-readers.h
File be/src/exec/parquet/parquet-column-readers.h:
http://gerrit.cloudera.org:8080/#/c/11949/8/be/src/exec/parquet/parquet-column-readers.h@382
PS8, Line 382: /// Try to move the the next page and buffer more values.
Return false and sets rep_level_,
line too long (93 > 90)
http://gerrit.cloudera.org:8080/#/c/11949/8/be/src/exec/parquet/parquet-column-readers.h@410
PS8, Line 410: /// 'data_page_pool_'. 'err_ctx' provides context for error
messages. On success, 'buffer'
line too long (92 > 90)
http://gerrit.cloudera.org:8080/#/c/11949/8/be/src/exec/parquet/parquet-metadata-utils.cc
File be/src/exec/parquet/parquet-metadata-utils.cc:
http://gerrit.cloudera.org:8080/#/c/11949/8/be/src/exec/parquet/parquet-metadata-utils.cc@376
PS8, Line 376: Status ParquetSchemaResolver::CreateSchemaTree(const
vector& schema,
line too long (92 > 90)
http://gerrit.cloudera.org:8080/#/c/11949/8/be/src/exec/parquet/parquet-metadata-utils.cc@481
PS8, Line 481: const SchemaPath& path, SchemaNode** node, bool* pos_field,
bool* missing_field) const {
line too long (92 > 90)
http://gerrit.cloudera.org:8080/#/c/11949/8/be/src/exec/parquet/parquet-metadata-utils.cc@699
PS8, Line 699: Status ParquetSchemaResolver::ResolveMap(const SchemaPath& path,
int idx, SchemaNode** node,
line too long (92 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/11949
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0efd5c50b781fe9e3c022b33c66c06cfb529c0b8
Gerrit-Change-Number: 11949
Gerrit-PatchSet: 8
Gerrit-Owner: Tim Armstrong
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Tue, 20 Nov 2018 17:41:32 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7869: break up parquet-column-readers.cc
Hello Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11949 to look at the new patch set (#8). Change subject: IMPALA-7869: break up parquet-column-readers.cc .. IMPALA-7869: break up parquet-column-readers.cc Move parquet classes into exec/parquet. Move CollectionColumnReader and ParquetLevelDecoder into separate files. Switch BOOLEAN decoding to use composition instead of inheritance. This lets the boolean decoding use the faster batched implementations in ScalarColumnReader and avoids some confusing aspects of the class hierarchy, like the ReadValueBatch() implementation on the base class that was shared between BoolColumnReader and CollectionColumnReader. Improve compile times by instantiating BitPacking templates in a separate file (this looks to give a 30s+ speedup for compiling parquet-column-readers.cc). Testing: Ran exhaustive tests. Change-Id: I0efd5c50b781fe9e3c022b33c66c06cfb529c0b8 --- M be/CMakeLists.txt M be/src/benchmarks/bit-packing-benchmark.cc M be/src/benchmarks/bswap-benchmark.cc M be/src/codegen/impala-ir.cc M be/src/exec/CMakeLists.txt M be/src/exec/data-source-scan-node.cc M be/src/exec/hdfs-scan-node-base.cc M be/src/exec/hdfs-table-sink.cc R be/src/exec/parquet/hdfs-parquet-scanner-ir.cc R be/src/exec/parquet/hdfs-parquet-scanner-test.cc R be/src/exec/parquet/hdfs-parquet-scanner.cc R be/src/exec/parquet/hdfs-parquet-scanner.h R be/src/exec/parquet/hdfs-parquet-table-writer.cc R be/src/exec/parquet/hdfs-parquet-table-writer.h A be/src/exec/parquet/parquet-bool-decoder.cc A be/src/exec/parquet/parquet-bool-decoder.h A be/src/exec/parquet/parquet-collection-column-reader.cc A be/src/exec/parquet/parquet-collection-column-reader.h R be/src/exec/parquet/parquet-column-readers.cc R be/src/exec/parquet/parquet-column-readers.h R be/src/exec/parquet/parquet-column-stats.cc R be/src/exec/parquet/parquet-column-stats.h R be/src/exec/parquet/parquet-column-stats.inline.h R be/src/exec/parquet/parquet-common.cc R be/src/exec/parquet/parquet-common.h A be/src/exec/parquet/parquet-level-decoder.cc A be/src/exec/parquet/parquet-level-decoder.h R be/src/exec/parquet/parquet-metadata-utils.cc R be/src/exec/parquet/parquet-metadata-utils.h R be/src/exec/parquet/parquet-plain-test.cc R be/src/exec/parquet/parquet-scratch-tuple-batch.h R be/src/exec/parquet/parquet-version-test.cc M be/src/util/CMakeLists.txt M be/src/util/bit-packing-test.cc A be/src/util/bit-packing.cc M be/src/util/bit-packing.h M be/src/util/bit-packing.inline.h M be/src/util/bit-stream-utils.inline.h M be/src/util/dict-encoding.h M be/src/util/dict-test.cc M be/src/util/parquet-reader.cc M be/src/util/rle-test.cc M common/thrift/generate_error_codes.py M testdata/workloads/functional-query/queries/QueryTest/parquet-num-values-def-levels-mismatch.test 44 files changed, 1,035 insertions(+), 749 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/49/11949/8 -- To view, visit http://gerrit.cloudera.org:8080/11949 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0efd5c50b781fe9e3c022b33c66c06cfb529c0b8 Gerrit-Change-Number: 11949 Gerrit-PatchSet: 8 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-7839: Remove code duplication for getting a unique catalog object name
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11928 ) Change subject: IMPALA-7839: Remove code duplication for getting a unique catalog object name .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1406/ : 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/11928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8218e57210c06fe6f2578a792d518e4b0287f15f Gerrit-Change-Number: 11928 Gerrit-PatchSet: 2 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 20 Nov 2018 17:03:57 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7839: Remove code duplication for getting a unique catalog object name
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11928 ) Change subject: IMPALA-7839: Remove code duplication for getting a unique catalog object name .. Patch Set 2: Code-Review+1 Carry Csaba's +1. -- To view, visit http://gerrit.cloudera.org:8080/11928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8218e57210c06fe6f2578a792d518e4b0287f15f Gerrit-Change-Number: 11928 Gerrit-PatchSet: 2 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 20 Nov 2018 16:21:39 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7839: Remove code duplication for getting a unique catalog object name
Fredy Wijaya has posted comments on this change. (
http://gerrit.cloudera.org:8080/11928 )
Change subject: IMPALA-7839: Remove code duplication for getting a unique
catalog object name
..
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11928/1/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java
File fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java:
http://gerrit.cloudera.org:8080/#/c/11928/1/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java@44
PS1, Line 44: public final String getUniqueName() {
> Does any subclass overrides this? If not, then it could be 'final'.
Yeah final is a good idea. We can remove it later if that's not the case. Done.
http://gerrit.cloudera.org:8080/#/c/11928/1/fe/src/main/java/org/apache/impala/catalog/Db.java
File fe/src/main/java/org/apache/impala/catalog/Db.java:
http://gerrit.cloudera.org:8080/#/c/11928/1/fe/src/main/java/org/apache/impala/catalog/Db.java@439
PS1, Line 439:
> +1 idea to remove code duplication:
Good suggestion. Done.
--
To view, visit http://gerrit.cloudera.org:8080/11928
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8218e57210c06fe6f2578a792d518e4b0287f15f
Gerrit-Change-Number: 11928
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Tue, 20 Nov 2018 16:21:27 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7839: Remove code duplication for getting a unique catalog object name
Fredy Wijaya has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/11928 ) Change subject: IMPALA-7839: Remove code duplication for getting a unique catalog object name .. IMPALA-7839: Remove code duplication for getting a unique catalog object name This patch removes code duplication for getting a catalog object unique name by reusing the code from Catalog::toCatalogObjectKey( TCatalogObject). Testing: - Ran all FE tests Change-Id: I8218e57210c06fe6f2578a792d518e4b0287f15f --- M fe/src/main/java/org/apache/impala/catalog/Catalog.java M fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java M fe/src/main/java/org/apache/impala/catalog/DataSource.java M fe/src/main/java/org/apache/impala/catalog/Db.java M fe/src/main/java/org/apache/impala/catalog/Function.java M fe/src/main/java/org/apache/impala/catalog/HdfsCachePool.java M fe/src/main/java/org/apache/impala/catalog/Principal.java M fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java M fe/src/main/java/org/apache/impala/catalog/Table.java 9 files changed, 38 insertions(+), 58 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/28/11928/2 -- To view, visit http://gerrit.cloudera.org:8080/11928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8218e57210c06fe6f2578a792d518e4b0287f15f Gerrit-Change-Number: 11928 Gerrit-PatchSet: 2 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-7233: [DOCS] Support for IANA timezone database
Zoltan Ivanfi has posted comments on this change. ( http://gerrit.cloudera.org:8080/11946 ) Change subject: IMPALA-7233: [DOCS] Support for IANA timezone database .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/11946/1/docs/topics/impala_timestamp.xml File docs/topics/impala_timestamp.xml: http://gerrit.cloudera.org:8080/#/c/11946/1/docs/topics/impala_timestamp.xml@367 PS1, Line 367: turned off by : default to avoid performance overhead > I still think that the improvement should mentioned, e.g "Before 3.1, this I would rephrase "eliminated most of the overhead" as "scales well to multiple threads" or similar. The overhead is still there, it just does not effectively make Impala run in a single thread when doing the adjustments. -- To view, visit http://gerrit.cloudera.org:8080/11946 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id400cda5a1be321063d17e0ee6337e92a5da732a Gerrit-Change-Number: 11946 Gerrit-PatchSet: 1 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Zoltan Ivanfi Gerrit-Comment-Date: Tue, 20 Nov 2018 16:08:54 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7233: [DOCS] Support for IANA timezone database
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/11946 ) Change subject: IMPALA-7233: [DOCS] Support for IANA timezone database .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/11946/1/docs/topics/impala_timestamp.xml File docs/topics/impala_timestamp.xml: http://gerrit.cloudera.org:8080/#/c/11946/1/docs/topics/impala_timestamp.xml@367 PS1, Line 367: turned off by : default to avoid performance overhead > Done I still think that the improvement should mentioned, e.g "Before 3.1, this option had severe impact on multi-threaded performance. The new timezone implementation in 3.1 eliminated most of the overhead." One reason why I would recommend switching this on is that 3.2 will include int64 encoded timestamps, where timezone handling is no longer influenced by this flag, but decided by metadate in the .parquet file. This means that if this flag is turned off, then int64 and int96 timestamps will behave differently. -- To view, visit http://gerrit.cloudera.org:8080/11946 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id400cda5a1be321063d17e0ee6337e92a5da732a Gerrit-Change-Number: 11946 Gerrit-PatchSet: 1 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Zoltan Ivanfi Gerrit-Comment-Date: Tue, 20 Nov 2018 16:03:49 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7839: Remove code duplication for getting a unique catalog object name
Csaba Ringhofer has posted comments on this change. (
http://gerrit.cloudera.org:8080/11928 )
Change subject: IMPALA-7839: Remove code duplication for getting a unique
catalog object name
..
Patch Set 1: Code-Review+1
(2 comments)
lgtm + I left some ideas.
http://gerrit.cloudera.org:8080/#/c/11928/1/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java
File fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java:
http://gerrit.cloudera.org:8080/#/c/11928/1/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java@44
PS1, Line 44: public String getUniqueName() { return
Catalog.toCatalogObjectKey(toTCatalogObject()); }
Does any subclass overrides this? If not, then it could be 'final'.
http://gerrit.cloudera.org:8080/#/c/11928/1/fe/src/main/java/org/apache/impala/catalog/Db.java
File fe/src/main/java/org/apache/impala/catalog/Db.java:
http://gerrit.cloudera.org:8080/#/c/11928/1/fe/src/main/java/org/apache/impala/catalog/Db.java@439
PS1, Line 439: catalogObj.setDb(toThrift());
+1 idea to remove code duplication:
as toTCatalogObject() implementations seem identical with the exception of
setting different properties, the catalogObj.set* part could be moved to a
function like writeToTCatalogObject, while the rest of toTCatalogObject() could
be moved to CatalogObjectImpl.
--
To view, visit http://gerrit.cloudera.org:8080/11928
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8218e57210c06fe6f2578a792d518e4b0287f15f
Gerrit-Change-Number: 11928
Gerrit-PatchSet: 1
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Tue, 20 Nov 2018 15:50:34 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7233: [DOCS] Support for IANA timezone database
Zoltan Ivanfi has posted comments on this change. ( http://gerrit.cloudera.org:8080/11946 ) Change subject: IMPALA-7233: [DOCS] Support for IANA timezone database .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/11946/1/docs/topics/impala_timestamp.xml File docs/topics/impala_timestamp.xml: http://gerrit.cloudera.org:8080/#/c/11946/1/docs/topics/impala_timestamp.xml@333 PS1, Line 333: values the same way it stores : without any adjustment. > Hive reads and writes TIMESTAMP values without converting with respect to t This sentence seems to contradict the description below. Hive 2.x _does_ adjust timestamps with respect to time zones. That is the cause of the incompatibility with Impala, which does not. -- To view, visit http://gerrit.cloudera.org:8080/11946 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id400cda5a1be321063d17e0ee6337e92a5da732a Gerrit-Change-Number: 11946 Gerrit-PatchSet: 1 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Zoltan Ivanfi Gerrit-Comment-Date: Tue, 20 Nov 2018 15:45:09 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7869: break up parquet-column-readers.cc
Csaba Ringhofer has posted comments on this change. (
http://gerrit.cloudera.org:8080/11949 )
Change subject: IMPALA-7869: break up parquet-column-readers.cc
..
Patch Set 7:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/11949/7/be/src/exec/parquet-bool-decoder.h
File be/src/exec/parquet-bool-decoder.h:
http://gerrit.cloudera.org:8080/#/c/11949/7/be/src/exec/parquet-bool-decoder.h@17
PS7, Line 17:
We have several parquet_* files in exec/ - it would be nice to move this to a
separate directory like exex/parquet. I am not sure that would worth the extra
pain during future merges, but if we want to do this, then this commit seems
the right time.
http://gerrit.cloudera.org:8080/#/c/11949/7/be/src/exec/parquet-column-readers.cc
File be/src/exec/parquet-column-readers.cc:
http://gerrit.cloudera.org:8080/#/c/11949/7/be/src/exec/parquet-column-readers.cc@74
PS7, Line 74: i
Where is this new variable used?
http://gerrit.cloudera.org:8080/#/c/11949/7/be/src/exec/parquet-column-readers.cc@284
PS7, Line 284: /// The size of this column with plain encoding for
FIXED_LEN_BYTE_ARRAY, or
: /// the max length for VARCHAR columns. Unused otherwise.
: int fixed_len_size_;
:
: /// Contains extra data needed for Timestamp decoding.
: ParquetTimestampDecoder timestamp_decoder_;
:
: /// Contains extra state required to decode boolean values.
Only initialised for
: /// BOOLEAN columns.
: unique_ptr bool_decoder_;
A note about members that are used only in specific types: I have a plan to
create a class like template ParquetTypeDecoder {} whose specializations could add members
needed for special cases like TIMESTAMP/CHAR(N)/BOOlEAN. The main benefit of
moving this logic into another class is that it could be reused in DictDecoder
during the implementation of IMPALA-4994.
http://gerrit.cloudera.org:8080/#/c/11949/7/be/src/exec/parquet-level-decoder.h
File be/src/exec/parquet-level-decoder.h:
http://gerrit.cloudera.org:8080/#/c/11949/7/be/src/exec/parquet-level-decoder.h@122
PS7, Line 122: /// The parquet encoding used for the levels. Only RLE is
supported for now.
: parquet::Encoding::type encoding_ = parquet::Encoding::PLAIN;
This was not changed, but looks a bit weird (only RLE is supported, that it is
set to PLAIN) 'encoding_' could be removed to simplify code - it looks unlikely
that there will be another encoding for levels in the future.
--
To view, visit http://gerrit.cloudera.org:8080/11949
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0efd5c50b781fe9e3c022b33c66c06cfb529c0b8
Gerrit-Change-Number: 11949
Gerrit-PatchSet: 7
Gerrit-Owner: Tim Armstrong
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Tue, 20 Nov 2018 15:11:03 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7804: Mitigate s3 consistency issues for test scanners
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/11959 ) Change subject: IMPALA-7804: Mitigate s3 consistency issues for test_scanners .. Patch Set 2: Code-Review+1 (2 comments) Had some questions, but lgtm http://gerrit.cloudera.org:8080/#/c/11959/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11959/2//COMMIT_MSG@27 PS2, Line 27: This may improve the : likelihood that the filesystem is consistent by : the time we read it. The docs at https://docs.aws.amazon.com/AmazonS3/latest/dev/Introduction.html says that S3 provides read-after-write consistency for PUTS of new objects, why does it fall to eventual consistency in our case? I suspect it is the listing of directory contents which is eventual consistent only. Is there a way to ensure stronger consistency to make the tests completely stable? Wouldn't S3Guard http://blog.cloudera.com/blog/2017/08/introducing-s3guard-s3-consistency-for-apache-hadoop/ solve this problem? http://gerrit.cloudera.org:8080/#/c/11959/2/tests/common/file_utils.py File tests/common/file_utils.py: http://gerrit.cloudera.org:8080/#/c/11959/2/tests/common/file_utils.py@67 PS2, Line 67: command nit: introducing 'command' variable seems unnecessary, and we don't do that at other places in the file either -- To view, visit http://gerrit.cloudera.org:8080/11959 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id042496beabe0d0226b347e0653b820fee369f4e Gerrit-Change-Number: 11959 Gerrit-PatchSet: 2 Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Tue, 20 Nov 2018 11:42:46 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7869: break up parquet-column-readers.cc
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11949 ) Change subject: IMPALA-7869: break up parquet-column-readers.cc .. Patch Set 7: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1405/ : 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/11949 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0efd5c50b781fe9e3c022b33c66c06cfb529c0b8 Gerrit-Change-Number: 11949 Gerrit-PatchSet: 7 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 20 Nov 2018 08:15:26 + Gerrit-HasComments: No
[Impala-ASF-CR] MPALA-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: MPALA-7867, part 1: Expose List in TreeNode, parser .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1404/ : 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: 1 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 20 Nov 2018 08:02:21 + Gerrit-HasComments: No
