[Impala-ASF-CR] IMPALA-4080, IMPALA-3638: Introduce ExecNode::Codegen()
Hello Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4651 to look at the new patch set (#5). Change subject: IMPALA-4080, IMPALA-3638: Introduce ExecNode::Codegen() .. IMPALA-4080, IMPALA-3638: Introduce ExecNode::Codegen() This patch is mostly mechanical move of codegen related logic from each exec node's Prepare() to its Codegen() function. After this change, code generation will no longer happen in Prepare(). Instead, it will happen after Prepare() completes in PlanFragmentExecutor. This is an intermediate step towards the final goal of sharing compiled code among fragment instances in multi-threading. As part of the clean up, this change also removes the logic for lazy codegen object creation. In other words, if codegen is enabled, the codegen object will always be created. This simplifies some of the logic in ScalarFnCall::Prepare() and various Codegen() functions by reducing error checking needed. This change also removes the logic added for tackling IMPALA-1755 as it's not needed anymore after the clean up. The clean up also rectifies a not so well documented situation. Previously, even if a user explicitly sets DISABLE_CODEGEN to true, we may still codegen a UDF if it was written in LLVM IR or if it has more than 8 arguments. This patch enforces the query option by failing the query in both cases. To run the query, the user must enable codegen. This change also extends the number of arguments supported in the interpretation path of ScalarFn to 20. Change-Id: I207566bc9f4c6a159271ecdbc4bbdba3d78c6651 --- M be/src/exec/aggregation-node.cc M be/src/exec/aggregation-node.h M be/src/exec/exchange-node.cc M be/src/exec/exchange-node.h M be/src/exec/exec-node.cc M be/src/exec/exec-node.h M be/src/exec/hash-join-node.cc M be/src/exec/hash-join-node.h M be/src/exec/hash-table.cc M be/src/exec/hash-table.h M be/src/exec/hdfs-avro-scanner.cc M be/src/exec/hdfs-avro-scanner.h M be/src/exec/hdfs-parquet-scanner.cc M be/src/exec/hdfs-scan-node-base.cc M be/src/exec/hdfs-scan-node-base.h M be/src/exec/hdfs-scanner.cc M be/src/exec/hdfs-sequence-scanner.cc M be/src/exec/hdfs-text-scanner.cc M be/src/exec/old-hash-table.cc M be/src/exec/old-hash-table.h M be/src/exec/partitioned-aggregation-node.cc M be/src/exec/partitioned-aggregation-node.h M be/src/exec/partitioned-hash-join-builder.cc M be/src/exec/partitioned-hash-join-builder.h M be/src/exec/partitioned-hash-join-node.cc M be/src/exec/partitioned-hash-join-node.h M be/src/exec/sort-node.cc M be/src/exec/sort-node.h M be/src/exec/topn-node.cc M be/src/exec/topn-node.h M be/src/exprs/case-expr.cc M be/src/exprs/case-expr.h M be/src/exprs/compound-predicates.cc M be/src/exprs/compound-predicates.h M be/src/exprs/conditional-functions.cc M be/src/exprs/conditional-functions.h M be/src/exprs/expr.cc M be/src/exprs/expr.h M be/src/exprs/hive-udf-call.cc M be/src/exprs/hive-udf-call.h M be/src/exprs/is-not-empty-predicate.cc M be/src/exprs/is-not-empty-predicate.h M be/src/exprs/literal.cc M be/src/exprs/literal.h M be/src/exprs/null-literal.cc M be/src/exprs/null-literal.h M be/src/exprs/scalar-fn-call.cc M be/src/exprs/scalar-fn-call.h M be/src/exprs/slot-ref.cc M be/src/exprs/slot-ref.h M be/src/exprs/tuple-is-null-predicate.cc M be/src/exprs/tuple-is-null-predicate.h M be/src/runtime/plan-fragment-executor.cc M be/src/runtime/runtime-state.cc M be/src/runtime/runtime-state.h M be/src/runtime/sorted-run-merger.h M be/src/runtime/sorter.cc M be/src/runtime/sorter.h M be/src/runtime/tuple.cc M be/src/runtime/tuple.h M be/src/service/fe-support.cc M be/src/testutil/test-udfs.cc M be/src/util/tuple-row-compare.cc M be/src/util/tuple-row-compare.h M common/thrift/PlanNodes.thrift M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java M testdata/workloads/functional-query/queries/QueryTest/udf-errors.test M testdata/workloads/functional-query/queries/QueryTest/udf.test M tests/query_test/test_udfs.py 69 files changed, 642 insertions(+), 690 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/4651/5 -- To view, visit http://gerrit.cloudera.org:8080/4651 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I207566bc9f4c6a159271ecdbc4bbdba3d78c6651 Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-4080, IMPALA-3638: Introduce ExecNode::Codegen()
Michael Ho has posted comments on this change. Change subject: IMPALA-4080, IMPALA-3638: Introduce ExecNode::Codegen() .. Patch Set 4: (7 comments) http://gerrit.cloudera.org:8080/#/c/4651/4/be/src/exec/exec-node.h File be/src/exec/exec-node.h: PS4, Line 72: If overridden in subclass, must also : /// call superclass's Codegen(). > does it matter whether this happens first or last? Not really. Comments updated. http://gerrit.cloudera.org:8080/#/c/4651/4/be/src/exec/hash-join-node.cc File be/src/exec/hash-join-node.cc: Line 148: } > why not do this in ExecNode::Prepare() to avoid having to do the same thing That would have the unfortunate side effect of adding this message to operators which aren't codegen enabled. This may be confusing. http://gerrit.cloudera.org:8080/#/c/4651/4/be/src/exec/partitioned-hash-join-builder.h File be/src/exec/partitioned-hash-join-builder.h: PS4, Line 88: whether the codegen was : /// enabled > we only call this if codegen is enabled, right? So I'm not sure what this Done http://gerrit.cloudera.org:8080/#/c/4651/4/be/src/exprs/scalar-fn-call.cc File be/src/exprs/scalar-fn-call.cc: Line 409: // TODO: fix this > not for this change, but i think this timestamp stuff may have been fixed b Done. TODO removed. http://gerrit.cloudera.org:8080/#/c/4651/4/be/src/runtime/runtime-state.h File be/src/runtime/runtime-state.h: PS4, Line 161: 'codegen_'. > public method comments shouldn't really talk about private members. how abo Done http://gerrit.cloudera.org:8080/#/c/4651/4/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java: Line 532 > why is it okay to always enable codegen now, whereas before we were so care We always codegen conjunct evaluation for parquet scanner now so this code is mostly irrelevant for parquet scanner. For other file formats, they weren't really affected by this flag as they always codegen (if implemented). For parquet scanner, this flag used to affect whether codegen will happen in ScalarFnCall::Prepare() and that had performance impact on conjuncts evaluation. All this complication stems from lazy creation of the LLVM module to avoid the overhead of LLVM module creation when the fragment doesn't have any codegen enabled operator: https://github.com/cloudera/Impala/commit/0686cd9c3ed7ae48d5bd4fe602266034ef871ffc We didn't have lazy IR code materialization back then so the preparation time of an LLVM module was higher (in the order of at least 150+ ms on my dev box). With lazy IR code materialization and a recent change to lazily create the mapping of IRFunction::Type to llvm::Function*, we pushed the creation time of LLVM module to about 10~11ms. With this patch, if codegen is enabled, we will always create the codegen module. This avoids problem such as IMPALA-1755 and IMPALA-3638. http://gerrit.cloudera.org:8080/#/c/4651/4/tests/query_test/test_udfs.py File tests/query_test/test_udfs.py: Line 80: vector.get_value('exec_option')['disable_codegen'] = 1 > this doesn't invalidate any of the preexisting test cases? AFAIK, no. The preexisting test cases were mostly about failure to load the functions. Nothing tied to codegen as far as I can tell. -- To view, visit http://gerrit.cloudera.org:8080/4651 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I207566bc9f4c6a159271ecdbc4bbdba3d78c6651 Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4310: Make push to asf.py respect --apache remote
Henry Robinson has submitted this change and it was merged. Change subject: IMPALA-4310: Make push_to_asf.py respect --apache_remote .. IMPALA-4310: Make push_to_asf.py respect --apache_remote Change-Id: I03e15753e685b1b8cf953e8009fb473c9c12aa93 Reviewed-on: http://gerrit.cloudera.org:8080/4747 Reviewed-by: Sailesh Mukil Tested-by: Henry Robinson --- M bin/push_to_asf.py 1 file changed, 1 insertion(+), 1 deletion(-) Approvals: Henry Robinson: Verified Sailesh Mukil: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/4747 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I03e15753e685b1b8cf953e8009fb473c9c12aa93 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Sailesh Mukil
[Impala-ASF-CR] IMPALA-4310: Make push to asf.py respect --apache remote
Henry Robinson has posted comments on this change. Change subject: IMPALA-4310: Make push_to_asf.py respect --apache_remote .. Patch Set 1: Verified+1 Doesn't affect tests, so +1 verified. -- To view, visit http://gerrit.cloudera.org:8080/4747 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I03e15753e685b1b8cf953e8009fb473c9c12aa93 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: No
[Impala-ASF-CR] PREVIEW IMPALA-2521: Add clustered hint to insert statements
Alex Behm has posted comments on this change. Change subject: PREVIEW IMPALA-2521: Add clustered hint to insert statements .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/4745/1/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java File fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java: Line 275: public PlanFragment addClusteringFragment(PlanFragment inputFragment, > you should move the logic (minus the createSortTupleInfo part that alex bro createInsertFragment() is only called for distributed plans. Should clustered also work for single-node plans? -- To view, visit http://gerrit.cloudera.org:8080/4745 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I412153bd8435d792bd61dea268d7a3b884048f14 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Lars Volker Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Marcel Kornacker Gerrit-HasComments: Yes
[Impala-ASF-CR] PREVIEW IMPALA-2521: Add clustered hint to insert statements
Marcel Kornacker has posted comments on this change. Change subject: PREVIEW IMPALA-2521: Add clustered hint to insert statements .. Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/4745/1/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java File fe/src/main/java/org/apache/impala/analysis/InsertStmt.java: Line 121: private boolean hasClusteredHint_ = false; might be better to use ternary logic here with a single Boolean (which can be null) Line 631: if (hasNoShuffleHint_) { did you mean noclustered? Line 632: throw new AnalysisException("Conflicting INSERT hint: " + hint); list both conflicting hints (otherwise it's hard to tell how to fix the problem, other than dropping the hint). also fix the other error messages. http://gerrit.cloudera.org:8080/#/c/4745/1/fe/src/main/java/org/apache/impala/analysis/SortInfo.java File fe/src/main/java/org/apache/impala/analysis/SortInfo.java: Line 68:* sorted. Its source exprs are update to those in tupleSlotExprs. second sentence incomprehensible. Line 123:* the exprs used by the SortNode refer to the materialized tuples instead of the don't refer to the sort node here, SortInfo shouldn't have to know about that http://gerrit.cloudera.org:8080/#/c/4745/1/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java File fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java: Line 275: public PlanFragment addClusteringFragment(PlanFragment inputFragment, you should move the logic (minus the createSortTupleInfo part that alex brought up) into createInsertFragment -- To view, visit http://gerrit.cloudera.org:8080/4745 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I412153bd8435d792bd61dea268d7a3b884048f14 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Lars Volker Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Marcel Kornacker Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2916: Add warning to query profile if debug build
Internal Jenkins has submitted this change and it was merged. Change subject: IMPALA-2916: Add warning to query profile if debug build .. IMPALA-2916: Add warning to query profile if debug build Change-Id: I85ce4d4a5624382203e6b2c8f5b96d04c4482f37 Reviewed-on: http://gerrit.cloudera.org:8080/4588 Reviewed-by: Henry Robinson Tested-by: Internal Jenkins --- M be/src/service/query-exec-state.cc 1 file changed, 4 insertions(+), 0 deletions(-) Approvals: Henry Robinson: Looks good to me, approved Internal Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/4588 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I85ce4d4a5624382203e6b2c8f5b96d04c4482f37 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Lars Volker Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Lars Volker
[Impala-ASF-CR] IMPALA-2916: Add warning to query profile if debug build
Internal Jenkins has posted comments on this change. Change subject: IMPALA-2916: Add warning to query profile if debug build .. Patch Set 2: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/4588 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I85ce4d4a5624382203e6b2c8f5b96d04c4482f37 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Lars Volker Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Lars Volker Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4310: Make push to asf.py respect --apache remote
Sailesh Mukil has posted comments on this change. Change subject: IMPALA-4310: Make push_to_asf.py respect --apache_remote .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/4747 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I03e15753e685b1b8cf953e8009fb473c9c12aa93 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4310: Make push to asf.py respect --apache remote
Henry Robinson has uploaded a new change for review. http://gerrit.cloudera.org:8080/4747 Change subject: IMPALA-4310: Make push_to_asf.py respect --apache_remote .. IMPALA-4310: Make push_to_asf.py respect --apache_remote Change-Id: I03e15753e685b1b8cf953e8009fb473c9c12aa93 --- M bin/push_to_asf.py 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/4747/1 -- To view, visit http://gerrit.cloudera.org:8080/4747 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I03e15753e685b1b8cf953e8009fb473c9c12aa93 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson
[Impala-ASF-CR] IMPALA-4277: allow overriding of Hive/Hadoop versions/locations
Internal Jenkins has posted comments on this change. Change subject: IMPALA-4277: allow overriding of Hive/Hadoop versions/locations .. Patch Set 3: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/4720 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7da10e38f9c4309f2d193dc25f14a6ea308c9639 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Charlie Helin Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4277: allow overriding of Hive/Hadoop versions/locations
Internal Jenkins has submitted this change and it was merged. Change subject: IMPALA-4277: allow overriding of Hive/Hadoop versions/locations .. IMPALA-4277: allow overriding of Hive/Hadoop versions/locations This is to help with IMPALA-4277 to make it easier to build against Hadoop/Hive distributions where the directory layout doesn't exactly match our current CDH dependencies, or where we may want to temporarily override a version without making a source change. Change-Id: I7da10e38f9c4309f2d193dc25f14a6ea308c9639 Reviewed-on: http://gerrit.cloudera.org:8080/4720 Reviewed-by: Sailesh Mukil Tested-by: Internal Jenkins --- M bin/impala-config.sh M buildall.sh M cmake_modules/FindHDFS.cmake M common/thrift/CMakeLists.txt 4 files changed, 27 insertions(+), 21 deletions(-) Approvals: Internal Jenkins: Verified Sailesh Mukil: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/4720 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I7da10e38f9c4309f2d193dc25f14a6ea308c9639 Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Charlie Helin Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] PREVIEW IMPALA-2521: Add clustered hint to insert statements
Alex Behm has posted comments on this change. Change subject: PREVIEW IMPALA-2521: Add clustered hint to insert statements .. Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/4745/1/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java File fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java: Line 269:* clustered/noclustered plan hint. The clustering re-order the data in 'inputFragment' Better to be specific: Say that it adds a sort node that orders on the partition columns of the target table. Line 272: // TODO: this function shares some code with QueryStmt::createSortTupleInfo(). However You could move most of the code into SortInfo.createSortTupleInfo(List resultExprs); Line 309: sortTupleDesc.setIsMaterialized(true); To indicate that this tuple is a physical tuple needed during runtime execution and not a "virtual" tuple like e.g. the tuple of an inline view (which never gets materialized). Line 324: // TODO Why is this needed here but not in QueryStmt.java? If it is omitted, the query Before plan generation we call QueryStmt.materializeRequiredSlots() to make sure all slots needed to evaluate that QueryStmt are marked as materialized. However, we have no such mechanism here because we are adding a plan node that does not come from a SQL clause, so we must mark the slots as materialized manually. http://gerrit.cloudera.org:8080/#/c/4745/1/fe/src/main/java/org/apache/impala/planner/Planner.java File fe/src/main/java/org/apache/impala/planner/Planner.java: Line 144: rootFragment = distributedPlanner.addClusteringFragment( 1. we should also do this for single-node plans, so adding the clustering doesn't really belong in the DistributedPlanner 2. we are not adding a new fragment, so I'd rephrase this addClusteringNode() or similar -- To view, visit http://gerrit.cloudera.org:8080/4745 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I412153bd8435d792bd61dea268d7a3b884048f14 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Lars Volker Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Marcel Kornacker Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4253: impala-server.backends.client-cache.total-clients shows negative value
Juan Yu has posted comments on this change. Change subject: IMPALA-4253: impala-server.backends.client-cache.total-clients shows negative value .. Patch Set 2: Code-Review+2 (1 comment) Carry Dan's +2 http://gerrit.cloudera.org:8080/#/c/4668/1/be/src/runtime/client-cache.cc File be/src/runtime/client-cache.cc: PS1, Line 95: if (metrics_enabled_) { > the "if succeeded" explains this, so you could remove this sentence, which Done -- To view, visit http://gerrit.cloudera.org:8080/4668 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic9e28055cb232cdb543c4c9f05a558ab0f73f777 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Juan Yu Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Juan Yu Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2905: Move QueryResultSet implementations into separate module
Henry Robinson has posted comments on this change. Change subject: IMPALA-2905: Move QueryResultSet implementations into separate module .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/4736 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6ab883b62d3ec7012240edf8d56889349e7c0e32 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2905: Move QueryResultSet implementations into separate module
Henry Robinson has posted comments on this change. Change subject: IMPALA-2905: Move QueryResultSet implementations into separate module .. Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/4736/2//COMMIT_MSG Commit Message: Line 7: IMPALA-2905: Move QueryResultSet implementations into separate module > Is this the right JIRA? IMPALA-2905 is fixed. It was suggested in the 2905 review, where half the work was done. Didn't see a lot of point making a new JIRA. http://gerrit.cloudera.org:8080/#/c/4736/2/be/src/service/impala-beeswax-server.cc File be/src/service/impala-beeswax-server.cc: PS2, Line 500: ( > nit: extraneous parens. Done http://gerrit.cloudera.org:8080/#/c/4736/2/be/src/service/query-result-set.h File be/src/service/query-result-set.h: Line 27: #include > what for? Done -- To view, visit http://gerrit.cloudera.org:8080/4736 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6ab883b62d3ec7012240edf8d56889349e7c0e32 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4253: impala-server.backends.client-cache.total-clients shows negative value
Hello Sailesh Mukil, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4668 to look at the new patch set (#2). Change subject: IMPALA-4253: impala-server.backends.client-cache.total-clients shows negative value .. IMPALA-4253: impala-server.backends.client-cache.total-clients shows negative value Fixed double decrement in case a cached connection is broken and cannot be re-created. Change-Id: Ic9e28055cb232cdb543c4c9f05a558ab0f73f777 --- M be/src/runtime/client-cache.cc 1 file changed, 5 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/4668/2 -- To view, visit http://gerrit.cloudera.org:8080/4668 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic9e28055cb232cdb543c4c9f05a558ab0f73f777 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Juan Yu Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Sailesh Mukil
[Impala-ASF-CR] IMPALA-2905: Move QueryResultSet implementations into separate module
Hello Alex Behm, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4736 to look at the new patch set (#3). Change subject: IMPALA-2905: Move QueryResultSet implementations into separate module .. IMPALA-2905: Move QueryResultSet implementations into separate module This mostly mechanical change moves the definition and implementation of the Beeswax and HS2-specific result sets into their own module. Result sets are now uniformly created by one of two factory methods, so the implementation is decoupled from the client. Change-Id: I6ab883b62d3ec7012240edf8d56889349e7c0e32 --- M be/src/service/CMakeLists.txt M be/src/service/impala-beeswax-server.cc M be/src/service/impala-hs2-server.cc A be/src/service/query-result-set.cc M be/src/service/query-result-set.h 5 files changed, 503 insertions(+), 419 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/36/4736/3 -- To view, visit http://gerrit.cloudera.org:8080/4736 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I6ab883b62d3ec7012240edf8d56889349e7c0e32 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht
[Impala-ASF-CR] Add search / sort to HTML tables for metrics and threads
Henry Robinson has posted comments on this change. Change subject: Add search / sort to HTML tables for metrics and threads .. Patch Set 1: Code-Review+2 Pretty uncontroversial change, I hope. -- To view, visit http://gerrit.cloudera.org:8080/4743 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If069ce6a9eae00bacaa30605d23bea72f29e5c4f Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Internal Jenkins Gerrit-HasComments: No
[Impala-ASF-CR] Add search / sort to HTML tables for metrics and threads
Henry Robinson has submitted this change and it was merged. Change subject: Add search / sort to HTML tables for metrics and threads .. Add search / sort to HTML tables for metrics and threads Change-Id: If069ce6a9eae00bacaa30605d23bea72f29e5c4f Reviewed-on: http://gerrit.cloudera.org:8080/4743 Tested-by: Internal Jenkins Reviewed-by: Henry Robinson --- M www/metric_group.tmpl M www/thread-group.tmpl 2 files changed, 67 insertions(+), 39 deletions(-) Approvals: Henry Robinson: Looks good to me, approved Internal Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/4743 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: If069ce6a9eae00bacaa30605d23bea72f29e5c4f Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Internal Jenkins
[Impala-ASF-CR] Buffer pool: Add basic counters to buffer pool client
Internal Jenkins has posted comments on this change. Change subject: Buffer pool: Add basic counters to buffer pool client .. Patch Set 3: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/4714 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9a5a57b7cfccf67ee498e68964f1e077075ee325 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] Buffer pool: Add basic counters to buffer pool client
Internal Jenkins has submitted this change and it was merged. Change subject: Buffer pool: Add basic counters to buffer pool client .. Buffer pool: Add basic counters to buffer pool client Change-Id: I9a5a57b7cfccf67ee498e68964f1e077075ee325 Reviewed-on: http://gerrit.cloudera.org:8080/4714 Reviewed-by: Tim Armstrong Tested-by: Internal Jenkins --- A be/src/bufferpool/buffer-pool-counters.h M be/src/bufferpool/buffer-pool-test.cc M be/src/bufferpool/buffer-pool.cc M be/src/bufferpool/buffer-pool.h M be/src/bufferpool/reservation-tracker-test.cc 5 files changed, 102 insertions(+), 32 deletions(-) Approvals: Internal Jenkins: Verified Tim Armstrong: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/4714 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I9a5a57b7cfccf67ee498e68964f1e077075ee325 Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] Remove Llama dependency
Internal Jenkins has posted comments on this change. Change subject: Remove Llama dependency .. Patch Set 3: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/4739 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If2e5e21d8047097d56062ded11b0832a1d397fe0 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] Add search / sort to HTML tables for metrics and threads
Internal Jenkins has posted comments on this change. Change subject: Add search / sort to HTML tables for metrics and threads .. Patch Set 1: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/4743 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If069ce6a9eae00bacaa30605d23bea72f29e5c4f Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Internal Jenkins Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4123: Fast bit unpacking
Internal Jenkins has submitted this change and it was merged. Change subject: IMPALA-4123: Fast bit unpacking .. IMPALA-4123: Fast bit unpacking Adds utility functions for fast unpacking of batches of bit-packed values. These support reading batches of any number of values provided that the start of the batch is aligned to a byte boundary. Callers that want to read smaller batches that don't align to byte boundaries will need to implement their own buffering. The unpacking code uses only portable C++ and no SIMD intrinsics, but is fairly efficient because unpacking a full batch of 32 values compiles down to 32-bit loads, shifts by constants, masks by constants, bitwise ors when a value straddles 32-bit words and stores. Further speedups should be possible using SIMD intrinsics. Testing: Added unit tests for unpacking, exhaustively covering different bitwidths with additional test dimensions (memory alignment, various input sizes, etc). Tested under ASAN to ensure the bit unpacking doesn't read past the end of buffers. Perf: Added microbenchmark that shows on average an 8-9x speedup over the existing BitReader code. Change-Id: I12db69409483d208cd4c0f41c27a78aeb6cd3622 Reviewed-on: http://gerrit.cloudera.org:8080/4494 Reviewed-by: Tim Armstrong Tested-by: Internal Jenkins --- M be/src/benchmarks/CMakeLists.txt A be/src/benchmarks/bit-packing-benchmark.cc M be/src/benchmarks/bswap-benchmark.cc M be/src/exprs/expr-test.cc A be/src/testutil/mem-util.h M be/src/util/CMakeLists.txt A be/src/util/bit-packing-test.cc A be/src/util/bit-packing.h A be/src/util/bit-packing.inline.h M be/src/util/bit-stream-utils.h M be/src/util/bit-stream-utils.inline.h M be/src/util/bit-util.h M be/src/util/openssl-util-test.cc 13 files changed, 929 insertions(+), 84 deletions(-) Approvals: Internal Jenkins: Verified Tim Armstrong: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/4494 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I12db69409483d208cd4c0f41c27a78aeb6cd3622 Gerrit-PatchSet: 16 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-4123: Fast bit unpacking
Internal Jenkins has posted comments on this change. Change subject: IMPALA-4123: Fast bit unpacking .. Patch Set 15: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/4494 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I12db69409483d208cd4c0f41c27a78aeb6cd3622 Gerrit-PatchSet: 15 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3920: TotalStorageWaitTime counter not populated for fragments with Kudu scan node
Internal Jenkins has posted comments on this change. Change subject: IMPALA-3920: TotalStorageWaitTime counter not populated for fragments with Kudu scan node .. Patch Set 3: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/4639 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If0c793930799fdcaff53e705f94b52cadac2f53a Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: anujphadke Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-784: Use `-s in SHOW CREATE TABLE output
Lars Volker has posted comments on this change. Change subject: IMPALA-784: Use `-s in SHOW CREATE TABLE output .. Patch Set 2: If you abandon this, please leave a link in the Jira so we know it is there. Otherwise it will be easy to miss. -- To view, visit http://gerrit.cloudera.org:8080/4527 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8e66439daa35d2e3725e7277c16f3cc5fdb6f128 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zoltan Ivanfi Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Ivanfi Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3342: Add thread counters to monitor plan fragment execution
anujphadke has posted comments on this change. Change subject: IMPALA-3342: Add thread counters to monitor plan fragment execution .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/4633/5/be/src/runtime/plan-fragment-executor.cc File be/src/runtime/plan-fragment-executor.cc: Line 347: Status status; > Bad rebase? Done -- To view, visit http://gerrit.cloudera.org:8080/4633 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300 Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: anujphadke Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Yonghyun Hwang Gerrit-Reviewer: anujphadke Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3342: Add thread counters to monitor plan fragment execution
anujphadke has uploaded a new patch set (#6). Change subject: IMPALA-3342: Add thread counters to monitor plan fragment execution .. IMPALA-3342: Add thread counters to monitor plan fragment execution This change removes the use of total_cpu_timer which incorrectly monitors the CPU time. Adding THREAD_COUNTERS to measure the user and sys time in plan fragment execution. This also accounts for the time spent in the hdfs/kudu scanner and in a blocking join. Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300 --- M be/src/exec/blocking-join-node.cc M be/src/exec/hdfs-scan-node.cc M be/src/exec/kudu-scan-node.cc M be/src/runtime/plan-fragment-executor.cc M be/src/runtime/plan-fragment-executor.h M be/src/runtime/runtime-state.cc M be/src/runtime/runtime-state.h 7 files changed, 17 insertions(+), 26 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/33/4633/6 -- To view, visit http://gerrit.cloudera.org:8080/4633 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300 Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: anujphadke Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Yonghyun Hwang Gerrit-Reviewer: anujphadke
[Impala-ASF-CR] PREVIEW IMPALA-2521: Add clustered hint to insert statements
Lars Volker has posted comments on this change. Change subject: PREVIEW IMPALA-2521: Add clustered hint to insert statements .. Patch Set 1: In a first round I'd like to ask for feedback on the overall approach and the open TODO's in the code. The new method in DistributedPlanner.java is currenlty still very verbosly commented, mostly because I tried to understand the control flow. I can remove most of the comments in the next iteration, but I'd like to receive feedback if any of them are wrong. -- To view, visit http://gerrit.cloudera.org:8080/4745 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I412153bd8435d792bd61dea268d7a3b884048f14 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Lars Volker Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Marcel Kornacker Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2905: Move QueryResultSet implementations into separate module
Dan Hecht has posted comments on this change. Change subject: IMPALA-2905: Move QueryResultSet implementations into separate module .. Patch Set 2: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/4736/2/be/src/service/impala-beeswax-server.cc File be/src/service/impala-beeswax-server.cc: PS2, Line 500: ( nit: extraneous parens. -- To view, visit http://gerrit.cloudera.org:8080/4736 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6ab883b62d3ec7012240edf8d56889349e7c0e32 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-HasComments: Yes
[Impala-ASF-CR] PREVIEW IMPALA-2521: Add clustered hint to insert statements
Lars Volker has uploaded a new change for review. http://gerrit.cloudera.org:8080/4745 Change subject: PREVIEW IMPALA-2521: Add clustered hint to insert statements .. PREVIEW IMPALA-2521: Add clustered hint to insert statements This change introduces a clustered/noclustered hint for insert statements. Specifying this hint adds an additional sort node to the distributed plan, just before the table sink. This has the effect that data will be clustered by its partition prior to writing partitions, which therefore can be written sequentially. Change-Id: I412153bd8435d792bd61dea268d7a3b884048f14 --- M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java M fe/src/main/java/org/apache/impala/analysis/SortInfo.java M fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java M fe/src/main/java/org/apache/impala/planner/Planner.java M testdata/workloads/functional-planner/queries/PlannerTest/insert.test 5 files changed, 155 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/45/4745/1 -- To view, visit http://gerrit.cloudera.org:8080/4745 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I412153bd8435d792bd61dea268d7a3b884048f14 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Lars Volker
[Impala-ASF-CR] IMPALA-4080, IMPALA-3638: Introduce ExecNode::Codegen()
Dan Hecht has posted comments on this change. Change subject: IMPALA-4080, IMPALA-3638: Introduce ExecNode::Codegen() .. Patch Set 4: (7 comments) http://gerrit.cloudera.org:8080/#/c/4651/4/be/src/exec/exec-node.h File be/src/exec/exec-node.h: PS4, Line 72: If overridden in subclass, must also : /// call superclass's Codegen(). does it matter whether this happens first or last? http://gerrit.cloudera.org:8080/#/c/4651/4/be/src/exec/hash-join-node.cc File be/src/exec/hash-join-node.cc: Line 148: } why not do this in ExecNode::Prepare() to avoid having to do the same thing in every derived class? http://gerrit.cloudera.org:8080/#/c/4651/4/be/src/exec/partitioned-hash-join-builder.h File be/src/exec/partitioned-hash-join-builder.h: PS4, Line 88: whether the codegen was : /// enabled we only call this if codegen is enabled, right? So I'm not sure what this part means (maybe just delete it). http://gerrit.cloudera.org:8080/#/c/4651/4/be/src/exprs/scalar-fn-call.cc File be/src/exprs/scalar-fn-call.cc: Line 409: // TODO: fix this not for this change, but i think this timestamp stuff may have been fixed by moving things out of the IR. http://gerrit.cloudera.org:8080/#/c/4651/4/be/src/runtime/runtime-state.h File be/src/runtime/runtime-state.h: PS4, Line 161: 'codegen_'. public method comments shouldn't really talk about private members. how about "... for this fragment instance" or something like that. http://gerrit.cloudera.org:8080/#/c/4651/4/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java: Line 532 why is it okay to always enable codegen now, whereas before we were so careful to only enable it based on this heuristic? http://gerrit.cloudera.org:8080/#/c/4651/4/tests/query_test/test_udfs.py File tests/query_test/test_udfs.py: Line 80: vector.get_value('exec_option')['disable_codegen'] = 1 this doesn't invalidate any of the preexisting test cases? -- To view, visit http://gerrit.cloudera.org:8080/4651 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I207566bc9f4c6a159271ecdbc4bbdba3d78c6651 Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2905: Move QueryResultSet implementations into separate module
Alex Behm has posted comments on this change. Change subject: IMPALA-2905: Move QueryResultSet implementations into separate module .. Patch Set 2: Code-Review+1 (2 comments) http://gerrit.cloudera.org:8080/#/c/4736/2//COMMIT_MSG Commit Message: Line 7: IMPALA-2905: Move QueryResultSet implementations into separate module Is this the right JIRA? IMPALA-2905 is fixed. http://gerrit.cloudera.org:8080/#/c/4736/2/be/src/service/query-result-set.h File be/src/service/query-result-set.h: Line 27: #include what for? -- To view, visit http://gerrit.cloudera.org:8080/4736 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6ab883b62d3ec7012240edf8d56889349e7c0e32 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Alex Behm Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4120: Incorrect results with LEAD() analytic function
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-4120: Incorrect results with LEAD() analytic function .. Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/4740/1//COMMIT_MSG Commit Message: PS1, Line 18: AggFnEvaluatorthese ? http://gerrit.cloudera.org:8080/#/c/4740/1/be/src/exec/analytic-eval-node.cc File be/src/exec/analytic-eval-node.cc: PS1, Line 389: AggFnEvaluator::GetValue(evaluators_, fn_ctxs_, curr_tuple_, result_tuple, : cur_tuple_pool); wrt my comment in AggFnEvaluator, what if we didn't change GetValue() and instead we called a helper fn here that moved any backing memory from result_tuple into cur_tuple_pool, e.g. // Copies memory backing Strings (in analytic result slots) from result_tuple into memory allocated from cur_tuple_pool. RelocateStringMemory(result_tuple, cur_tuple_pool); (or something like that) http://gerrit.cloudera.org:8080/#/c/4740/1/be/src/exprs/agg-fn-evaluator.cc File be/src/exprs/agg-fn-evaluator.cc: PS1, Line 499: v = StringVal::null(); this means the allocation failed, right? should we be propagating errors? seems like that'd lead to incorrect results under low-memory conditions. http://gerrit.cloudera.org:8080/#/c/4740/1/be/src/exprs/agg-fn-evaluator.h File be/src/exprs/agg-fn-evaluator.h: PS1, Line 149: /// Puts the finalized value from Tuple* src in Tuple* dst just as Finalize() does. : /// 'pool' is the MemPool used for allocating memory for the finalized value (e.g. : /// StringVal). However, unlike Finalize(), GetValue() does not clean up state in src. : /// GetValue() can be called repeatedly with the same src. Only used internally for : /// analytic fn builtins. : void GetValue(FunctionContext* agg_fn_ctx, Tuple* src, Tuple* dst, MemPool* pool); I see how this and the below changes to the GetValue() path (all the way through SerializeOrFinalize) fix the issue, but I'm worried it's going to be more confusing when the interface isn't symmetrical, e.g. other Serialize and Finalize can't allocate mem out of pool even though it wouldn't be crazy for that to be allowed. What if the caller handled copying GetValue memory instead of having GetValue() handle the string copy? We could have a helper fn somewhere (maybe just in AggFnEvaluator, or maybe there's a better place) which moved backing memory into a specified pool. Does that make sense? -- To view, visit http://gerrit.cloudera.org:8080/4740 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I85bb1745232d8dd383a6047c86019c6378ab571f Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Michael Ho Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2916: Add warning to query profile if debug build
Henry Robinson has posted comments on this change. Change subject: IMPALA-2916: Add warning to query profile if debug build .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/4588 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I85ce4d4a5624382203e6b2c8f5b96d04c4482f37 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Lars Volker Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Lars Volker Gerrit-HasComments: No
[Impala-ASF-CR] Buffer pool: Add basic counters to buffer pool client
Tim Armstrong has posted comments on this change. Change subject: Buffer pool: Add basic counters to buffer pool client .. Patch Set 3: Code-Review+2 carry +2 -- To view, visit http://gerrit.cloudera.org:8080/4714 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9a5a57b7cfccf67ee498e68964f1e077075ee325 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2916: Add warning to query profile if debug build
Lars Volker has posted comments on this change. Change subject: IMPALA-2916: Add warning to query profile if debug build .. Patch Set 1: (2 comments) Thanks for the review, please see PS2. http://gerrit.cloudera.org:8080/#/c/4588/1/be/src/service/query-exec-state.cc File be/src/service/query-exec-state.cc: PS1, Line 88: WARNING > Let's not use such a generic key. How about "Debug Mode Warning" ? Done. I made the key all uppercase, so it becomes very easy to spot. Let me know if you think it's too much. PS1, Line 88: "WARNING", "This profile has been created by a DEBUG build. If " : "you are looking for performance related information, we suggest to use a RELEASE " : "build instead." > Just a suggestion to make it snappier: Thanks, done. -- To view, visit http://gerrit.cloudera.org:8080/4588 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I85ce4d4a5624382203e6b2c8f5b96d04c4482f37 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Lars Volker Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Lars Volker Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2916: Add warning to query profile if debug build
Lars Volker has uploaded a new patch set (#2). Change subject: IMPALA-2916: Add warning to query profile if debug build .. IMPALA-2916: Add warning to query profile if debug build Change-Id: I85ce4d4a5624382203e6b2c8f5b96d04c4482f37 --- M be/src/service/query-exec-state.cc 1 file changed, 4 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/88/4588/2 -- To view, visit http://gerrit.cloudera.org:8080/4588 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I85ce4d4a5624382203e6b2c8f5b96d04c4482f37 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Lars Volker Gerrit-Reviewer: Henry Robinson
[Impala-ASF-CR] Add search / sort to HTML tables for metrics and threads
Henry Robinson has uploaded a new change for review. http://gerrit.cloudera.org:8080/4743 Change subject: Add search / sort to HTML tables for metrics and threads .. Add search / sort to HTML tables for metrics and threads Change-Id: If069ce6a9eae00bacaa30605d23bea72f29e5c4f --- M www/metric_group.tmpl M www/thread-group.tmpl 2 files changed, 67 insertions(+), 39 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/4743/1 -- To view, visit http://gerrit.cloudera.org:8080/4743 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: If069ce6a9eae00bacaa30605d23bea72f29e5c4f Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson
[Impala-ASF-CR] Buffer pool: Add basic counters to buffer pool client
Dan Hecht has posted comments on this change. Change subject: Buffer pool: Add basic counters to buffer pool client .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/4714 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9a5a57b7cfccf67ee498e68964f1e077075ee325 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables
Alex Behm has posted comments on this change. Change subject: IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables .. Patch Set 7: (26 comments) Next round over the code. http://gerrit.cloudera.org:8080/#/c/4414/7/be/src/service/frontend.cc File be/src/service/frontend.cc: Line 62: "value should be a comma separated list of hostnames or IP addresses."); are ports optional or mandatory? http://gerrit.cloudera.org:8080/#/c/4414/7/common/thrift/CatalogObjects.thrift File common/thrift/CatalogObjects.thrift: Line 358: 1: required list values Why not a list of TExpr that are expected to be literals? Seems more future proof. Line 368: // the type parameter. which type parameter? http://gerrit.cloudera.org:8080/#/c/4414/7/common/thrift/JniCatalog.thrift File common/thrift/JniCatalog.thrift: Line 398: 14: optional list distribute_by; for consistency let's remove trailing ";" http://gerrit.cloudera.org:8080/#/c/4414/7/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: Line 1033: // class doesn't inherit from CreateTableStmt. whitespace Line 1047: // Used for creating external Kudu tables for which the schema is loaded from Kudu. There seem to be more uses of this production, so this comment could be misleading. Maybe generalize to something like "Used for creating tables where the schema is inferred externally, e.g., from an Avro schema, Kudu table or query statement." Line 1112: // or one RANGE clause typo: clauses http://gerrit.cloudera.org:8080/#/c/4414/7/fe/src/main/java/org/apache/impala/analysis/ColumnDef.java File fe/src/main/java/org/apache/impala/analysis/ColumnDef.java: Line 93: void setIsPrimaryKey() { isPrimaryKey_ = true; } do we need this? Line 191: Preconditions.checkState(!colDefsByColName.containsKey(colDef.getColName())); can check return value of put() http://gerrit.cloudera.org:8080/#/c/4414/7/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java File fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java: Line 208:* Analyzes and checks table properties which are common for both managed and external typo: common to Line 255: "PARTITIONED BY cannot be used with an Kudu table."); typo: a Kudu table this also needs to be checked for managed tables right? Line 273: AnalysisUtils.throwIfNullOrEmpty(getPrimaryKeyColumnDefs(), Shouldn't this check hasPrimaryKey()? Line 284: "zero. Given number of replicas is: " + r.toString() + ".'"); remove trailing .' or add the opening single-quote Line 318: private boolean hasPrimaryKey() { Isn't it enough to check primaryKeyColDefs_ in tableDef_? http://gerrit.cloudera.org:8080/#/c/4414/7/fe/src/main/java/org/apache/impala/analysis/DistributeParam.java File fe/src/main/java/org/apache/impala/analysis/DistributeParam.java: Line 105: for (String colName: colNames_) { we could specify the same distribution column multiple times Line 127: throw new AnalysisException("Split values cannot be NULL"); do we have a test for this? Line 223: colNames_.addAll(colNames); do we need toLower()? http://gerrit.cloudera.org:8080/#/c/4414/7/fe/src/main/java/org/apache/impala/analysis/TableDef.java File fe/src/main/java/org/apache/impala/analysis/TableDef.java: Line 69: // Populated during analysis. Authoritative list of primary key column definitions populated during analysis. Line 177: fqTableName_.analyze(); Do you know if Kudu has more permissive or more restrictive constraints on what strings can be used as table/column names? I'd be surprised if HMS and Kudu were identical in that respect. Better to file a JIRA and leave that investigation/fix out of this patch. Line 181: if (analyzer.dbContainsTable(getTblName().getDb(), getTbl(), Privilege.CREATE) Are we going to check Sentry privs for Kudu tables? Also ok to defer this fix, but let's not forget. Line 220:* Analyzes the primary key columns. Primary keys are only supported for Kudu Replace the second sentence with a brief description what this checks. It does not check the format and succeeds if no primary keys are given, so there is nothing Kudu specific here (that is checked in CreateTableStmt). Line 234: StringBuilder columnDefStr = new StringBuilder(); Not used? http://gerrit.cloudera.org:8080/#/c/4414/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: Line 1117: if (db != null && params.cascade) dropTablesFromKudu(db); I think it might be a good idea to do this under the metastoreDdlLock_ as well to ensure that the Kudu and HMS table deletions are atomic. http://gerrit.cloudera.org:8080/#/c/4414/7/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java: Line 47: /*
[Impala-ASF-CR] Remove Llama dependency
Henry Robinson has posted comments on this change. Change subject: Remove Llama dependency .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/4739/1/common/thrift/metrics.json File common/thrift/metrics.json: PS1, Line 573: : : : : : : : : : : : : : : : : : : > I don't have any way to easily test it. These metrics no longer exist so I Looks like we (I) have removed metrics pretty recently anyhow. I couldn't remember whether this file constituted any kind of public interface that we couldn't remove things from. You should be good to go. -- To view, visit http://gerrit.cloudera.org:8080/4739 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If2e5e21d8047097d56062ded11b0832a1d397fe0 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] Remove Llama dependency
Tim Armstrong has posted comments on this change. Change subject: Remove Llama dependency .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/4739/1/common/thrift/metrics.json File common/thrift/metrics.json: PS1, Line 573: : : : : : : : : : : : : : : : : : : > Does anything break in (e.g.) Cloudera Manager if you remove this? I don't have any way to easily test it. These metrics no longer exist so I can't see how anything downstream could be doing anything with them. -- To view, visit http://gerrit.cloudera.org:8080/4739 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If2e5e21d8047097d56062ded11b0832a1d397fe0 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] Remove Llama dependency
Henry Robinson has posted comments on this change. Change subject: Remove Llama dependency .. Patch Set 3: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/4739/1/common/thrift/metrics.json File common/thrift/metrics.json: PS1, Line 573: : : : : : : : : : : : : : : : : : : Does anything break in (e.g.) Cloudera Manager if you remove this? -- To view, visit http://gerrit.cloudera.org:8080/4739 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If2e5e21d8047097d56062ded11b0832a1d397fe0 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4230: ASF policy issues from 2.7.0 rc3.
Tim Armstrong has posted comments on this change. Change subject: IMPALA-4230: ASF policy issues from 2.7.0 rc3. .. Patch Set 5: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/4582 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I30ff77d7ac28ce67511c200764fba19ae69922e0 Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Jim Apple Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] Impala-3342: Adding thread counters to measure time spent during plan fragment execution
Tim Armstrong has posted comments on this change. Change subject: Impala-3342: Adding thread counters to measure time spent during plan fragment execution .. Patch Set 5: (2 comments) http://gerrit.cloudera.org:8080/#/c/4633/5//COMMIT_MSG Commit Message: Line 7: Impala-3342: Adding thread counters to measure time spent during plan The formatting is still weird. http://gerrit.cloudera.org:8080/#/c/4633/5/be/src/runtime/plan-fragment-executor.cc File be/src/runtime/plan-fragment-executor.cc: Line 347: <<< HEAD Bad rebase? -- To view, visit http://gerrit.cloudera.org:8080/4633 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300 Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: anujphadke Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Yonghyun Hwang Gerrit-Reviewer: anujphadke Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4024: Add "system" database and expose Impala metrics as a table
Tim Armstrong has abandoned this change. Change subject: IMPALA-4024: Add "system" database and expose Impala metrics as a table .. Abandoned I need to make time to polish this up and put out a version for review. -- To view, visit http://gerrit.cloudera.org:8080/3863 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: abandon Gerrit-Change-Id: I7adbeb45220c468e43b424d70c30b952f6cec2cd Gerrit-PatchSet: 22 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Kathy Sun Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Kathy Sun Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-4277: allow overriding of Hive/Hadoop versions/locations
Sailesh Mukil has posted comments on this change. Change subject: IMPALA-4277: allow overriding of Hive/Hadoop versions/locations .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/4720 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7da10e38f9c4309f2d193dc25f14a6ea308c9639 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Charlie Helin Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4123: Fast bit unpacking
Tim Armstrong has posted comments on this change. Change subject: IMPALA-4123: Fast bit unpacking .. Patch Set 15: (1 comment) http://gerrit.cloudera.org:8080/#/c/4494/11/be/src/util/bit-packing-test.cc File be/src/util/bit-packing-test.cc: Line 132: > No seed is the same as a single default seed for this type. Oh ok, this should be ok too then. -- To view, visit http://gerrit.cloudera.org:8080/4494 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I12db69409483d208cd4c0f41c27a78aeb6cd3622 Gerrit-PatchSet: 15 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4123: Fast bit unpacking
Tim Armstrong has posted comments on this change. Change subject: IMPALA-4123: Fast bit unpacking .. Patch Set 15: Code-Review+2 Carry +2 -- To view, visit http://gerrit.cloudera.org:8080/4494 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I12db69409483d208cd4c0f41c27a78aeb6cd3622 Gerrit-PatchSet: 15 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4123: Fast bit unpacking
Hello Jim Apple, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4494 to look at the new patch set (#14). Change subject: IMPALA-4123: Fast bit unpacking .. IMPALA-4123: Fast bit unpacking Adds utility functions for fast unpacking of batches of bit-packed values. These support reading batches of any number of values provided that the start of the batch is aligned to a byte boundary. Callers that want to read smaller batches that don't align to byte boundaries will need to implement their own buffering. The unpacking code uses only portable C++ and no SIMD intrinsics, but is fairly efficient because unpacking a full batch of 32 values compiles down to 32-bit loads, shifts by constants, masks by constants, bitwise ors when a value straddles 32-bit words and stores. Further speedups should be possible using SIMD intrinsics. Testing: Added unit tests for unpacking, exhaustively covering different bitwidths with additional test dimensions (memory alignment, various input sizes, etc). Tested under ASAN to ensure the bit unpacking doesn't read past the end of buffers. Perf: Added microbenchmark that shows on average an 8-9x speedup over the existing BitReader code. Change-Id: I12db69409483d208cd4c0f41c27a78aeb6cd3622 --- M be/src/benchmarks/CMakeLists.txt A be/src/benchmarks/bit-packing-benchmark.cc M be/src/benchmarks/bswap-benchmark.cc M be/src/exprs/expr-test.cc A be/src/testutil/mem-util.h M be/src/util/CMakeLists.txt A be/src/util/bit-packing-test.cc A be/src/util/bit-packing.h A be/src/util/bit-packing.inline.h M be/src/util/bit-stream-utils.h M be/src/util/bit-stream-utils.inline.h M be/src/util/bit-util.h M be/src/util/openssl-util-test.cc 13 files changed, 929 insertions(+), 84 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/4494/14 -- To view, visit http://gerrit.cloudera.org:8080/4494 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I12db69409483d208cd4c0f41c27a78aeb6cd3622 Gerrit-PatchSet: 14 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-4277: allow overriding of Hive/Hadoop versions/locations
Tim Armstrong has posted comments on this change. Change subject: IMPALA-4277: allow overriding of Hive/Hadoop versions/locations .. Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/4720/2/bin/impala-config.sh File bin/impala-config.sh: PS2, Line 333: exanded > "expanded" Done PS2, Line 335: ${HADOOP_HOME}/share/hadoop/tools/lib/* > Just checking, but does this need updating too? I don't think right now - we don't need this to build Impala. PS2, Line 404: ${HADOOP_HOME}/lib/native/ > There are more here. Done PS2, Line 424: ${HADOOP_HOME}/lib/native > And here. Done. I suspect this line isn't necessary but we can leave that to a separate change. -- To view, visit http://gerrit.cloudera.org:8080/4720 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7da10e38f9c4309f2d193dc25f14a6ea308c9639 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Charlie Helin Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4277: allow overriding of Hive/Hadoop versions/locations
Tim Armstrong has uploaded a new patch set (#3). Change subject: IMPALA-4277: allow overriding of Hive/Hadoop versions/locations .. IMPALA-4277: allow overriding of Hive/Hadoop versions/locations This is to help with IMPALA-4277 to make it easier to build against Hadoop/Hive distributions where the directory layout doesn't exactly match our current CDH dependencies, or where we may want to temporarily override a version without making a source change. Change-Id: I7da10e38f9c4309f2d193dc25f14a6ea308c9639 --- M bin/impala-config.sh M buildall.sh M cmake_modules/FindHDFS.cmake M common/thrift/CMakeLists.txt 4 files changed, 27 insertions(+), 21 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/4720/3 -- To view, visit http://gerrit.cloudera.org:8080/4720 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7da10e38f9c4309f2d193dc25f14a6ea308c9639 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Charlie Helin Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-3725 Support Kudu UPSERT in Impala
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-3725 Support Kudu UPSERT in Impala .. Patch Set 6: (5 comments) http://gerrit.cloudera.org:8080/#/c/4047/6/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: PS6, Line 734: RESULT = new InsertStmt(w, table, false, null, null, query, null, false, true); :} nit: can you add some newlines to make this spaced like the case above? http://gerrit.cloudera.org:8080/#/c/4047/6/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java File fe/src/main/java/org/apache/impala/analysis/InsertStmt.java: PS6, Line 75: all non-partition columns in Hive : // order. doesn't quite make sense for the Kudu code. Can you indicate how it's handled in that case? PS6, Line 342: setTargetTable This does more than sets the table. analyzeTargetTable() would be more accurate. http://gerrit.cloudera.org:8080/#/c/4047/6/fe/src/test/java/org/apache/impala/analysis/ParserTest.java File fe/src/test/java/org/apache/impala/analysis/ParserTest.java: PS6, Line 869: ParsesOk("with t as (select 1) upsert into x with t as (select 2) select * from t"); : ParsesOk("with t(c1) as (select 1) " + : "upsert into x with t(c2) as (select 2) select * from t"); This removes the insert test cases, I assume you didn't mean to change these lines http://gerrit.cloudera.org:8080/#/c/4047/6/testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test File testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test: PS6, Line 272: VARCHAR(20)), false), : (1, 'known', 1, 0, cast('a' as VARCHAR why varchars? -- To view, visit http://gerrit.cloudera.org:8080/4047 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8df5cea36b642e267f85ff6b163f3dd96b8386e9 Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4299: add buildall.sh option to start test cluster
Internal Jenkins has submitted this change and it was merged. Change subject: IMPALA-4299: add buildall.sh option to start test cluster .. IMPALA-4299: add buildall.sh option to start test cluster A previous commit "IMPALA-4259: build Impala without any test cluster setup" altered some undocumented side-effects of buildall.sh. Previously the following commands reconfigured and restarted the test cluster. It worked because buildall.sh unconditionally regenerated the test cluster configs. ./buildall.sh -notests && ./testdata/bin/run-all.sh ./buildall.sh -noclean -notests && ./testdata/bin/run-all.sh Instead of restoring the old behaviour and continuing to encourage mixing use of low and high level scripts like testdata/bin/run-all.sh as part of the "standard" workflow, this commit adds another high-level option to buildall.sh, -start_minicluster, that accomplishes the high-level task of restarting a minicluster with fresh configs. The above commands can be replaced with: ./buildall.sh -notests -start_minicluster ./buildall.sh -notests -noclean -start_minicluster Change-Id: I0ab3461f8ff3de49b3f28a0dc22fa0a6d5569da5 Reviewed-on: http://gerrit.cloudera.org:8080/4734 Reviewed-by: Alex Behm Tested-by: Internal Jenkins --- M buildall.sh 1 file changed, 14 insertions(+), 3 deletions(-) Approvals: Internal Jenkins: Verified Alex Behm: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/4734 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I0ab3461f8ff3de49b3f28a0dc22fa0a6d5569da5 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Internal Jenkins
[Impala-ASF-CR] IMPALA-4299: add buildall.sh option to start test cluster
Internal Jenkins has posted comments on this change. Change subject: IMPALA-4299: add buildall.sh option to start test cluster .. Patch Set 1: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/4734 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0ab3461f8ff3de49b3f28a0dc22fa0a6d5569da5 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Internal Jenkins Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2789: More compact mem layout with null bits at the end.
Alex Behm has posted comments on this change. Change subject: IMPALA-2789: More compact mem layout with null bits at the end. .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/4673/7/be/src/runtime/descriptors.cc File be/src/runtime/descriptors.cc: PS7, Line 575: // (in this case the tuple contains only a nullable double) : // define void @SetNotNull({ i8, double }* %tuple) { : // entry: : // %null_byte_ptr = getelementptr inbounds { i8, double }* %tuple, i32 0, i32 0 : // %null_byte = load i8* %null_byte_ptr : // %0 = and i8 %null_byte, -2 : // store i8 %0, i8* %null_byte_ptr : // ret void > this looks outdated. Thanks, I'll post a follow-on to update this. -- To view, visit http://gerrit.cloudera.org:8080/4673 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib6510c75d841bddafa6638f1bd2ac6731a7053f6 Gerrit-PatchSet: 7 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Alex Behm Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2789: More compact mem layout with null bits at the end.
Dan Hecht has posted comments on this change. Change subject: IMPALA-2789: More compact mem layout with null bits at the end. .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/4673/7/be/src/runtime/descriptors.cc File be/src/runtime/descriptors.cc: PS7, Line 575: // (in this case the tuple contains only a nullable double) : // define void @SetNotNull({ i8, double }* %tuple) { : // entry: : // %null_byte_ptr = getelementptr inbounds { i8, double }* %tuple, i32 0, i32 0 : // %null_byte = load i8* %null_byte_ptr : // %0 = and i8 %null_byte, -2 : // store i8 %0, i8* %null_byte_ptr : // ret void this looks outdated. -- To view, visit http://gerrit.cloudera.org:8080/4673 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib6510c75d841bddafa6638f1bd2ac6731a7053f6 Gerrit-PatchSet: 7 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Alex Behm Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3920: TotalStorageWaitTime counter not populated for fragments with Kudu scan node
Hello Matthew Jacobs, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4639 to look at the new patch set (#3). Change subject: IMPALA-3920: TotalStorageWaitTime counter not populated for fragments with Kudu scan node .. IMPALA-3920: TotalStorageWaitTime counter not populated for fragments with Kudu scan node Currently we do not start the TotalStorageWaitTime timer in the kudu-scanner. This patch replaces the kudu_read_timer with the TotalStorageWaitTime which measures the intended time. Change-Id: If0c793930799fdcaff53e705f94b52cadac2f53a --- M be/src/exec/kudu-scan-node.cc M be/src/exec/kudu-scan-node.h M be/src/exec/kudu-scanner.cc 3 files changed, 2 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/39/4639/3 -- To view, visit http://gerrit.cloudera.org:8080/4639 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: If0c793930799fdcaff53e705f94b52cadac2f53a Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: anujphadke Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Matthew Jacobs
[Impala-ASF-CR] IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables
Alex Behm has posted comments on this change. Change subject: IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables .. Patch Set 5: (8 comments) Responses to comments. Starting next round on code. http://gerrit.cloudera.org:8080/#/c/4414/5/fe/src/main/java/org/apache/impala/catalog/KuduTable.java File fe/src/main/java/org/apache/impala/catalog/KuduTable.java: Line 195: msClient.alter_table(msTable_.getDbName(), msTable_.getTableName(), msTable_); > Good point. We can potentially avoid this by checking if there has been som Right, most of the time there will be no changes. I'm not only worried about the perf, but all the extra weird states we can be in when doing this extra RPC to modify the HMS. Line 216: String colName = ToSqlUtils.getIdentSql(colSchema.getName()); > Hm, can you explain why this is the case given that we will use this name t We don't parse the column names in the HMS backend table and neither does Hive. The backticks are only needed to tell our parser to interpret a token as an identifier, but the parser is never invoked for column names from the HMS. I recommend trying this out with some Impala keywords to see what happens. Line 223: numClusteringCols_ = primaryKeyColumnNames_.size(); > Actually, I removed this and left a TODO. There are a few places that inter Yea, there's definitely some inconsistency here. http://gerrit.cloudera.org:8080/#/c/4414/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: Line 1151: // the metadata must be fetched from the Hive Metastore. > In order to drop a table from Kudu we need the Kudu table name which is sto Makes sense now that you've explained, please leave a brief comment. Line 1172: if (!KuduTable.isKuduTable(msTable)) continue; > Well, it's an easy check we can do to avoid RPC calls to Kudu for dropping Got it, thanks! http://gerrit.cloudera.org:8080/#/c/4414/5/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java: Line 85: kudu.createTable(kuduTableName, schema, tableOpts); > I see what you're saying but I don't think this will work. We need to be ab Agree. Can you file a JIRA against Kudu? Thanks! Line 178: if (kudu.tableExists(tableName)) { > I get it. See my comment above and let me know what you think. I agree with you. Line 214: String colName = ToSqlUtils.getIdentSql(colSchema.getName()); > I'd appreciate it if you could explain why is it wrong to use this here and Explained in other comment, let me know if still unclear :) -- To view, visit http://gerrit.cloudera.org:8080/4414 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7b9d51b2720ab57649abdb7d5c710ea04ff50dc1 Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Dimitris Tsirogiannis Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Michael Brown Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4123: Fast bit unpacking
Jim Apple has posted comments on this change. Change subject: IMPALA-4123: Fast bit unpacking .. Patch Set 13: Code-Review+2 (2 comments) http://gerrit.cloudera.org:8080/#/c/4494/11/be/src/util/bit-packing-test.cc File be/src/util/bit-packing-test.cc: Line 132: > Done but didn't seed it since flakiness would only happen if we have a bad No seed is the same as a single default seed for this type. http://gerrit.cloudera.org:8080/#/c/4494/13/be/src/util/bit-packing-test.cc File be/src/util/bit-packing-test.cc: PS13, Line 137: 0, numeric_limits::max() these are the default arguments to the constructor. -- To view, visit http://gerrit.cloudera.org:8080/4494 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I12db69409483d208cd4c0f41c27a78aeb6cd3622 Gerrit-PatchSet: 13 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] Impala-3342: Adding thread counters to measure time spent during plan fragment execution
anujphadke has posted comments on this change. Change subject: Impala-3342: Adding thread counters to measure time spent during plan fragment execution .. Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/4633/2//COMMIT_MSG Commit Message: PS2, Line 7: Impala-3342 Adding thread counters to measure time spent during plan : fragment execution > Please fix the formatting of the msg. Done PS2, Line 11: meausure > measure Done PS2, Line 13: hdfs/kudu scanner and in a blocking join > Why is this worth calling out? Doesn't this measure all exec nodes? This change replaces every instance of the total_cpu_timer. Replacing this timer in those 2 places and adding the THREAD_COUNTERS which get aggregated in the plan-fragment-executor. Adding thread counters in every thread would bulk up the profile. Line 14: > What does the profile look like? Would be helpful to see what the plan frag Fragment F01: Instance c64650f62b67d849:ff790c360002 (host=anuj-OptiPlex-9020:22000):(Total: 31.008ms, non-child: 0.000ns, % non-child: 0.00%) Hdfs split stats (:<# splits>/): 0:29/3.60 GB MemoryUsage(500.000ms): 57.78 MB, 129.77 MB, 137.79 MB, 129.79 MB, 129.79 MB, 121.79 MB, 153.78 MB, 121.79 MB, 129.79 MB, 129.79 MB, 129.79 MB, 129.79 MB, 97.79 MB, 137.79 MB, 137.79 MB, 129.79 MB, 121.79 MB, 105.79 MB, 105.89 MB, 121.79 MB, 129.79 MB, 121.79 MB, 137.79 MB, 113.79 MB, 113.79 MB, 137.79 MB, 121.79 MB, 129.79 MB, 129.79 MB, 113.79 MB, 105.86 MB, 97.76 MB, 113.76 MB, 105.76 MB, 73.72 MB ThreadUsage(500.000ms): 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 5, 5, 5, 4 - AverageThreadTokens: 5.86 - BloomFilterBytes: 0 - PeakMemoryUsage: 169.83 MB (178079056) - PerHostPeakMemUsage: 1.17 GB (1256325736) - PlanFragmentThreadInvoluntaryContextSwitches: 4.32K (4321) - PlanFragmentThreadTotalWallClockTime: 1m42s - PlanFragmentThreadSysTime: 206.195ms - PlanFragmentThreadUserTime: 41s769ms - PlanFragmentThreadVoluntaryContextSwitches: 35.57K (35570) - PrepareTime: 30.293ms - RowsProduced: 30.00M (2795) - TotalNetworkReceiveTime: 0.000ns - TotalNetworkSendTime: 2s396ms - TotalStorageWaitTime: 920.765ms CodeGen:(Total: 41.328ms, non-child: 41.328ms, % non-child: 100.00%) - CodegenTime: 518.571us - CompileTime: 3.161ms - LoadTime: 0.000ns - ModuleBitcodeSize: 1.90 MB (1996720) - NumFunctions: 9 (9) - NumInstructions: 113 (113) - OptimizationTime: 7.696ms - PrepareTime: 30.095ms DataStreamSender (dst_id=5):(Total: 16s230ms, non-child: 16s230ms, % non-child: 100.00%) - BytesSent: 428.18 MB (448979535) - NetworkThroughput(*): 236.47 MB/sec - OverallThroughput: 26.38 MB/sec http://gerrit.cloudera.org:8080/#/c/4633/2/be/src/runtime/plan-fragment-executor.cc File be/src/runtime/plan-fragment-executor.cc: Line 213: ADD_COUNTER(profile(), PER_HOST_PEAK_MEM_COUNTER, TUnit::BYTES); > Let's create the new counter here along with the other ones so that the set Done http://gerrit.cloudera.org:8080/#/c/4633/2/be/src/runtime/runtime-state.h File be/src/runtime/runtime-state.h: Line 241: ///Fragment thread counters > The comment isn't too helpful. Maybe: Done -- To view, visit http://gerrit.cloudera.org:8080/4633 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: anujphadke Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Yonghyun Hwang Gerrit-Reviewer: anujphadke Gerrit-HasComments: Yes
[Impala-ASF-CR](asf-site) Follow Apache Project Branding Requirements
Jim Apple has posted comments on this change. Change subject: Follow Apache Project Branding Requirements .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/4683/1/bylaws.html File bylaws.html: PS1, Line 134: : : : > I don't quite understand this change - is all the content going in the mast This is a separate issue that I threw in here to get this one page to have the same width for the body text as the other pages. http://gerrit.cloudera.org:8080/#/c/4683/1/downloads.html File downloads.html: PS1, Line 176: : : : Apache Impala is an effort undergoing incubation at the Apache Software Foundation : (ASF), sponsored by the Apache Incubator PMC. Incubation is required of all newly : accepted projects until a further review indicates that the infrastructure, : communications, and decision making process have stabilized in a manner consistent : with other successful ASF projects. While incubation status is not necessarily a : reflection of the completeness or stability of the code, it does indicate that the : project has yet to be fully endorsed by the ASF. : : Apache Impala, Impala, Apache, the Apache feather logo, and the Apache Impala : project logo are either registered trademarks or trademarks of The Apache Software : Foundation in the United States and other countries. : : > Can you try and keep the indentation consistent for s? It's hard to se I'm not thrilled with the indentation for these pages, either. https://issues.cloudera.org/browse/IMPALA-4307 -- To view, visit http://gerrit.cloudera.org:8080/4683 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I263790d7c9aba7bfc57de7c16a367fe5204c49ff Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: asf-site Gerrit-Owner: Jim Apple Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Jim Apple Gerrit-HasComments: Yes
[Impala-ASF-CR](asf-site) Follow Apache Project Branding Requirements
Jim Apple has uploaded a new patch set (#2). Change subject: Follow Apache Project Branding Requirements .. Follow Apache Project Branding Requirements See: http://www.apache.org/foundation/marks/pmcs.html Specifically: 1. Add TM to logo 2. Add links back to ASF 3. Add trademark notice to footers Change-Id: I263790d7c9aba7bfc57de7c16a367fe5204c49ff --- M bylaws.html M community.html M downloads.html M img/impala-logo.png M impala-docs.html M index.html M overview.html 7 files changed, 186 insertions(+), 53 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/83/4683/2 -- To view, visit http://gerrit.cloudera.org:8080/4683 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I263790d7c9aba7bfc57de7c16a367fe5204c49ff Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: asf-site Gerrit-Owner: Jim Apple Gerrit-Reviewer: Henry Robinson
[Impala-ASF-CR] IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables
Alex Behm has posted comments on this change. Change subject: IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables .. Patch Set 7: (29 comments) http://gerrit.cloudera.org:8080/#/c/4414/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java: Line 1350: // CTAS in an external Kudu table What's the rationale for not allowing this? Line 1716: AnalyzesOk("create table tab (x int, y string, primary key(x, y)) " + Can you add a brief comment explaining the table layout for this one? Line 1717: "distribute by hash(x) into 3 buckets, range(y) split rows " + can we distribute by range, hash? Line 1723: AnalyzesOk("create table tab (a int, b int, c int, d int, primary key (a, b, c))" + What happens if I specify PARTITION BY for a Kudu table? Line 1739: AnalysisError(String.format("create table tab (x int) distribute by hash (x) " + What about specifying the other params in tblproperties like the distribute by and split rows. Line 1748: AnalysisError("create table tab (x int primary key, primary key(x)) stored " + Add negative test where two ColumnDefs are declared as PK. Line 1762: AnalysisError("create table tab (a int, b int, c int, d int, primary key(a, b, c)) " + Do we have tests somewhere for checking which types are supported with Kudu? We should make sure that: * you can create a table with all supported types (and same for the specific clauses like primary key, distributed by, etc.) * you cannot create tables with unsupported types like TIMESTAMP/DECIMAL and nested types (should fail gracefully with "not supported") * or alternatively, if we want to defer the type checks to Kudu (and not bake it into Impala analysis), then we should document that somewhere Line 1772: // No float split keys Even if the column type is float? If so, might want to test that. Line 1810: // DISTRIBUTE BY is required for managed tables. Primary key is also required, do we have a test? Line 1812: "Table partitioning must be specified for managed Kudu tables."); Let's rephrase this as "Table distribution must be specified ..." PS6, Line 1828: AnalysisError("create external table t tblproperties (" + Not that it makes any sense, but hat happens with: create external table t (primary key()) ... Line 1863: public void TestCreateAvroTest() { Add a test with some of the Kudu-specific clauses and STORED AS AVRO Line 2822: public void TestShowFiles() throws AnalysisException { DO we have a TODO/JIRA somewhere to go through all DDL/SHOW commands and make them behave properly for Kudu? http://gerrit.cloudera.org:8080/#/c/4414/6/fe/src/test/java/org/apache/impala/analysis/ParserTest.java File fe/src/test/java/org/apache/impala/analysis/ParserTest.java: Line 1632 Why remove this? It will break my setup :) Line 2235: ParsesOk("CREATE TABLE foo (i INT PRIMARY KEY, PRIMARY KEY(i)) STORED AS KUDU"); Add case like this: CREATE TABLE foo (i INT PRIMARY KEY, j PRIMARY KEY) STORED AS KUDU Line 2541: ParsesOk("CREATE TABLE Foo TBLPROPERTIES ('a'='b', 'c'='d') AS SELECT * from bar"); nit: let's be consistent with how we chop up string literals across lines (space at end of previous line xor beginning of next line) http://gerrit.cloudera.org:8080/#/c/4414/6/fe/src/test/java/org/apache/impala/catalog/HdfsStorageDescriptorTest.java File fe/src/test/java/org/apache/impala/catalog/HdfsStorageDescriptorTest.java: Line 58: "org.apache.impala.hive.serde.ParquetInputFormat", revert http://gerrit.cloudera.org:8080/#/c/4414/6/testdata/bin/generate-schema-statements.py File testdata/bin/generate-schema-statements.py: Line 203: if row_format and file_format == 'text': I think row_format is also valid for sequence files (maybe we're not using it though) http://gerrit.cloudera.org:8080/#/c/4414/6/testdata/datasets/functional/functional_schema_template.sql File testdata/datasets/functional/functional_schema_template.sql: Line 823: distribute by range(id) split rows ((1003), (1007)) stored as kudu; ah so much better http://gerrit.cloudera.org:8080/#/c/4414/6/testdata/workloads/functional-query/queries/QueryTest/create-table-like-table.test File testdata/workloads/functional-query/queries/QueryTest/create-table-like-table.test: Line 218: create table kudu_table_clone like functional_kudu.alltypes Why can't we check this in analysis? http://gerrit.cloudera.org:8080/#/c/4414/6/testdata/workloads/functional-query/queries/QueryTest/kudu_create.test File testdata/workloads/functional-query/queries/QueryTest/kudu_create.test: Line 3: # This test file contains several cases for what basically amount to analysis errors, Should mention that some of this behavior is intentional and why. Line 11: ImpalaRuntimeException: Type TIMESTAMP is not supported in Kudu Do we get the same message
[Impala-ASF-CR] IMPALA-4120: Incorrect results with LEAD() analytic function
Michael Ho has posted comments on this change. Change subject: IMPALA-4120: Incorrect results with LEAD() analytic function .. Patch Set 1: This patch will copy the string returned from AggFnEvaluator::GetValue() into MemPool, similar to what we did for PAGG node. We can consider optimizing it by changing the GetValue() interface in the future to avoid this double copying. -- To view, visit http://gerrit.cloudera.org:8080/4740 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I85bb1745232d8dd383a6047c86019c6378ab571f Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Michael Ho Gerrit-HasComments: No
[Impala-ASF-CR] Remove Llama dependency
Tim Armstrong has posted comments on this change. Change subject: Remove Llama dependency .. Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/4739/2/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: Line 92: 21: optional i16 v_cpu_cores > add some TODO here Done Line 108: 25: optional i64 rm_initial_mem = 0 > add same TODO here Done http://gerrit.cloudera.org:8080/#/c/4739/2/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: Line 137: V_CPU_CORES, > add same TODO Done Line 153: RM_INITIAL_MEM, > add same TODO Done -- To view, visit http://gerrit.cloudera.org:8080/4739 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If2e5e21d8047097d56062ded11b0832a1d397fe0 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] Remove Llama dependency
Tim Armstrong has posted comments on this change. Change subject: Remove Llama dependency .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/4739 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If2e5e21d8047097d56062ded11b0832a1d397fe0 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] Remove Llama dependency
Hello Alex Behm, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4739 to look at the new patch set (#3). Change subject: Remove Llama dependency .. Remove Llama dependency This change prevents us from depending on LLAMA to build. Note that the LLAMA MiniKDC is left in - it is a test utility that does not depend on LLAMA itself. IMPALA-4292 tracks cleaning this up. Testing: Ran a private build to verify that all tests pass. Change-Id: If2e5e21d8047097d56062ded11b0832a1d397fe0 --- M bin/bootstrap_toolchain.py M bin/generate_minidump_collection_testdata.py M bin/impala-config.sh M bin/start-impala-cluster.py M common/thrift/ImpalaInternalService.thrift M common/thrift/ImpalaService.thrift M common/thrift/generate_metrics.py M common/thrift/metrics.json M infra/deploy/deploy.py 9 files changed, 13 insertions(+), 43 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/39/4739/3 -- To view, visit http://gerrit.cloudera.org:8080/4739 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: If2e5e21d8047097d56062ded11b0832a1d397fe0 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-4080, IMPALA-3638: Introduce ExecNode::Codegen()
Michael Ho has posted comments on this change. Change subject: IMPALA-4080, IMPALA-3638: Introduce ExecNode::Codegen() .. Patch Set 4: That's still the plan. This is just a stepping stone. We need to do this refactoring sooner or later anyway. -- To view, visit http://gerrit.cloudera.org:8080/4651 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I207566bc9f4c6a159271ecdbc4bbdba3d78c6651 Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] Impala-3342: Adding thread counters to measure time spent during plan fragment execution
anujphadke has uploaded a new patch set (#5). Change subject: Impala-3342: Adding thread counters to measure time spent during plan fragment execution .. Impala-3342: Adding thread counters to measure time spent during plan fragment execution This change removes the use of total_cpu_timer which incorrectly monitors the CPU time. Adding THREAD_COUNTERS to measure the user and sys time in plan fragment execution. This also accounts for the time spent in the hdfs/kudu scanner and in a blocking join. Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300 --- M be/src/exec/blocking-join-node.cc M be/src/exec/hdfs-scan-node.cc M be/src/exec/kudu-scan-node.cc M be/src/runtime/plan-fragment-executor.cc M be/src/runtime/plan-fragment-executor.h M be/src/runtime/runtime-state.cc M be/src/runtime/runtime-state.h 7 files changed, 34 insertions(+), 25 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/33/4633/5 -- To view, visit http://gerrit.cloudera.org:8080/4633 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300 Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: anujphadke Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Yonghyun Hwang
[Impala-ASF-CR] IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables
Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables .. Patch Set 5: (41 comments) http://gerrit.cloudera.org:8080/#/c/4414/5//COMMIT_MSG Commit Message: Line 45:"KEY" is expected to be common enough that the ident style > Might also want to mention that "key" is used for nested map types, so it's Done Line 51:on the HMS database and table name. If the database is "default", > Does this last regarding "default" make the code easier or more complicated Not sure why the special case. Removing it unless someone objects. http://gerrit.cloudera.org:8080/#/c/4414/5/be/src/service/frontend.cc File be/src/service/frontend.cc: Line 61: DEFINE_string(kudu_master_addrs, "", "Specifies the default Kudu master(s). The given " > Can we make this more consistent with out existing options? For example: Done http://gerrit.cloudera.org:8080/#/c/4414/5/fe/src/main/java/org/apache/impala/analysis/ColumnDef.java File fe/src/main/java/org/apache/impala/analysis/ColumnDef.java: Line 176: static List toColumnNames(Collection columnDefs) { > colDefs Done Line 184: static Map mapByColumnNames(Collection colDefs) { > comment that colDefs is assumed have no duplicate names, or alternatively d Done http://gerrit.cloudera.org:8080/#/c/4414/5/fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java File fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java: Line 99: "does not support (%s) file format. Supported formats are: (%s)", > the (%s) file format. Done Line 100: createStmt_.getFileFormat().toString().replace("_", ""), > what's with this weird "_" replacement? I believe the intention was to make the file format name from THdfsFileFormat consistent with the file format name as specified in a "STORED AS" clause. RC_FILE is RCFILE, TEXT is TEXTFILE. So, as you can tell we aren't very consistent here. That's also the reason for hardcoding the values in L101. Left a TODO in the CatalogObjects.thrift for now if that's ok with you. Line 101: "PARQUET, TEXTFILE, KUDU")); > use SUPPORTED_INSERT_FORMATS instead of hardcoding See comment above. http://gerrit.cloudera.org:8080/#/c/4414/5/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java File fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java: Line 80: private void setColumnDefs(List columnDefs) { > nit: colDefs everywhere Done Line 95: Map getTblProperties() { return tableDef_.getTblProperties(); } > is there any significance to not adding public/private to some of these met Casey started using the default access modifier that essentially gives public access only to classes in the same package. In theory, we should be using the most restrictive access modifier, so I kind of like this. For sure the codebase is not consistent with using this rule, so if you think we shouldn't be doing this I'll make them public. Line 192: "Only Kudu tables can use DISTRIBUTE BY clause."); > the DISTRIBUTE BY clause Done http://gerrit.cloudera.org:8080/#/c/4414/5/fe/src/main/java/org/apache/impala/analysis/TableDef.java File fe/src/main/java/org/apache/impala/analysis/TableDef.java: Line 43: * Represents the table parameters in a CREATE TABLE statement. > Let's be more precise and list the clauses that this captures. Done http://gerrit.cloudera.org:8080/#/c/4414/5/fe/src/main/java/org/apache/impala/catalog/KuduTable.java File fe/src/main/java/org/apache/impala/catalog/KuduTable.java: Line 97: // Table name in Kudu storage engine. It may not neccessarily be the same as the > the Kudu storage engine Done Line 101: // 1. For managed tables, 'kuduTableName_' is prefixed with '__IMPALA__' to > Can we make the prefix 'impala::' instead? Sure no prob. Done Line 154:* are loaded from HMS. The function also updates the table schema in HMS. > Add a comment about why we also update the HMS. My understanding is that we Done Line 195: msClient.alter_table(msTable_.getDbName(), msTable_.getTableName(), msTable_); > This means every invalidate/refresh will do an extra alter to HMS. Are we c Good point. We can potentially avoid this by checking if there has been some change in the schema. At this point, I am not so terribly worried about this. I'll keep a note to actually profile it and see if it's worth optimizing that path. Line 202:* Loads the schema from the Kudu table including column definitions and primary key > mention that it loads the schema into this table as well as the HMS table Done Line 216: String colName = ToSqlUtils.getIdentSql(colSchema.getName()); > getIdentSql() may add backticks, I don't think we want that here Hm, can you explain why this is the case given that we will use this name to populate the columns in the HMS table? Line 223: numClus
[Impala-ASF-CR] IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables
Dimitris Tsirogiannis has uploaded a new patch set (#7). Change subject: IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables .. IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables With this commit we simplify the syntax and handling of CREATE TABLE statements for both managed and external Kudu tables. Syntax example: CREATE TABLE foo(a INT, b STRING, PRIMARY KEY (a, b)) DISTRIBUTE BY HASH (a) INTO 3 BUCKETS, RANGE (b) SPLIT ROWS (('abc', 'def')) STORED AS KUDU Changes: 1) Remove the requirement to specify table properties such as key columns in tblproperties. 2) Read table schema (column definitions, primary keys, and distribution schemes) from Kudu instead of the HMS. 3) For external tables, the Kudu table is now required to exist at the time of creation in Impala. 4) Disallow table properties that could conflict with an existing table. Ex: key_columns cannot be specified. 5) Add KUDU as a file format. 6) Add a startup flag to impalad to specify the default Kudu master addresses. The flag is used as the default value for the table property kudu_master_addresses but it can still be overriden using TBLPROPERTIES. 7) Fix a post merge issue (IMPALA-3178) where DROP DATABASE CASCADE wasn't implemented for Kudu tables and silently ignored. The Kudu tables wouldn't be removed in Kudu. 8) Remove DDL delegates. There was only one functional delegate (for Kudu) the existence of the other delegate and the use of delegates in general has led to confusion. The Kudu delegate only exists to provide functionality missing from Hive. 9) Add PRIMARY KEY at the column and table level. This syntax is fairly standard. When used at the column level, only one column can be marked as a key. When used at the table level, multiple columns can be used as a key. Only Kudu tables are allowed to use PRIMARY KEY. The old "kudu.key_columns" table property is no longer accepted though it is still used internally. "PRIMARY" is now a keyword. The ident style declaration is used for "KEY" because it is also used for nested map types. 10) For managed tables, infer a Kudu table name if none was given. The table property "kudu.table_name" is optional for managed tables and is required for external tables. If for a managed table a Kudu table name is not provided, a table name will be generated based on the HMS database and table name. 11) Use Kudu master as the source of truth for table metadata instead of HMS when a table is loaded or refreshed. Table/column metadata are cached in the catalog and are stored in HMS in order to be able to use table and column statistics. Change-Id: I7b9d51b2720ab57649abdb7d5c710ea04ff50dc1 --- M be/src/service/frontend.cc M bin/start-catalogd.sh M bin/start-impala-cluster.py M common/thrift/CatalogObjects.thrift M common/thrift/JniCatalog.thrift M fe/src/main/cup/sql-parser.cup A fe/src/main/java/org/apache/impala/analysis/AnalysisUtils.java M fe/src/main/java/org/apache/impala/analysis/ColumnDef.java M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateTableDataSrcStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeFileStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java M fe/src/main/java/org/apache/impala/analysis/DistributeParam.java M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java A fe/src/main/java/org/apache/impala/analysis/TableDataLayout.java A fe/src/main/java/org/apache/impala/analysis/TableDef.java M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java M fe/src/main/java/org/apache/impala/catalog/Catalog.java M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java M fe/src/main/java/org/apache/impala/catalog/Db.java M fe/src/main/java/org/apache/impala/catalog/HdfsFileFormat.java M fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java M fe/src/main/java/org/apache/impala/catalog/KuduTable.java M fe/src/main/java/org/apache/impala/catalog/Table.java M fe/src/main/java/org/apache/impala/catalog/TableLoader.java M fe/src/main/java/org/apache/impala/catalog/Type.java D fe/src/main/java/org/apache/impala/catalog/delegates/DdlDelegate.java D fe/src/main/java/org/apache/impala/catalog/delegates/KuduDdlDelegate.java D fe/src/main/java/org/apache/impala/catalog/delegates/UnsupportedOpDelegate.java M fe/src/main/java/org/apache/impala/planner/HdfsPartitionFilter.java M fe/src/main/java/org/apache/impala/planner/KuduScanNode.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.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 A fe/src/main/java/org/apache/impala/
[Impala-ASF-CR] IMPALA-784: Use `-s in SHOW CREATE TABLE output
Henry Robinson has posted comments on this change. Change subject: IMPALA-784: Use `-s in SHOW CREATE TABLE output .. Patch Set 2: Zoltan - if nothing's going to happen on this patch for a while, could you 'abandon' it so that it doesn't show up in the pending review list? We can always reinstate it later. -- To view, visit http://gerrit.cloudera.org:8080/4527 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8e66439daa35d2e3725e7277c16f3cc5fdb6f128 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zoltan Ivanfi Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Ivanfi Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3823: Add timer to measure Parquet footer reads
Henry Robinson has posted comments on this change. Change subject: IMPALA-3823: Add timer to measure Parquet footer reads .. Patch Set 8: Code-Review+1 (11 comments) http://gerrit.cloudera.org:8080/#/c/4371/8/be/src/util/runtime-profile-counters.h File be/src/util/runtime-profile-counters.h: PS8, Line 204: Contrary to "Unlike" PS8, Line 204: a remove PS8, Line 206: The average is stored in the 'value_' member of the base class. Referring to member variables in a class description is usually a sign the text is leaking implementation details. Do you mean to say that value() returns the average? Line 209: SummaryStatsCounter(TUnit::type unit, int32_t total_num_values, Comment what this constructor is used for (is it when merging two SSCounters together?) PS8, Line 216: total_num_values can total_num_values be 0? PS8, Line 247: This keeps track of t Remove - just "The total..." is sufficient. PS8, Line 255: SpinLock counter_lock_; any reason not to call this lock_ ? http://gerrit.cloudera.org:8080/#/c/4371/8/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: PS8, Line 664: /* Print all SummaryStatsCounters as following: : : (Avg: ; Min: ; Max: ; : Number of samples: ) */ Comment style? PS8, Line 669: are is PS8, Line 1019: { min_ = new_value; } remove parentheses for single-line if statements. http://gerrit.cloudera.org:8080/#/c/4371/8/tests/query_test/test_scanners.py File tests/query_test/test_scanners.py: PS8, Line 317: \ remove -- To view, visit http://gerrit.cloudera.org:8080/4371 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Icf87bad90037dd0cea63b10c537382ec0f980cbf Gerrit-PatchSet: 8 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2916: Add warning to query profile if debug build
Henry Robinson has posted comments on this change. Change subject: IMPALA-2916: Add warning to query profile if debug build .. Patch Set 1: (2 comments) This will be useful to have. http://gerrit.cloudera.org:8080/#/c/4588/1/be/src/service/query-exec-state.cc File be/src/service/query-exec-state.cc: PS1, Line 88: WARNING Let's not use such a generic key. How about "Debug Mode Warning" ? PS1, Line 88: "WARNING", "This profile has been created by a DEBUG build. If " : "you are looking for performance related information, we suggest to use a RELEASE " : "build instead." Just a suggestion to make it snappier: "Query profile created while running a DEBUG build of Impala. Use RELEASE builds to measure query performance." -- To view, visit http://gerrit.cloudera.org:8080/4588 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I85ce4d4a5624382203e6b2c8f5b96d04c4482f37 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Lars Volker Gerrit-Reviewer: Henry Robinson Gerrit-HasComments: Yes
[Impala-ASF-CR] Remove Llama dependency
Alex Behm has posted comments on this change. Change subject: Remove Llama dependency .. Patch Set 2: Code-Review+2 (4 comments) http://gerrit.cloudera.org:8080/#/c/4739/2/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: Line 92: 21: optional i16 v_cpu_cores add some TODO here Line 108: 25: optional i64 rm_initial_mem = 0 add same TODO here http://gerrit.cloudera.org:8080/#/c/4739/2/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: Line 137: V_CPU_CORES, add same TODO Line 153: RM_INITIAL_MEM, add same TODO -- To view, visit http://gerrit.cloudera.org:8080/4739 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If2e5e21d8047097d56062ded11b0832a1d397fe0 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4050: Support starting webserver specified by hostname
Henry Robinson has posted comments on this change. Change subject: IMPALA-4050: Support starting webserver specified by hostname .. Patch Set 2: Hi - will you have some time to work on this patch, or should we close it out? -- To view, visit http://gerrit.cloudera.org:8080/4185 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibe7424a1c27f24560360219a5a6822b23dcbdce5 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: hewenting Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: hewenting Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4024: Add "system" database and expose Impala metrics as a table
Henry Robinson has posted comments on this change. Change subject: IMPALA-4024: Add "system" database and expose Impala metrics as a table .. Patch Set 19: What do you think we should do with this? Is it close to commit, or should we abandon and track finishing it off in a JIRA? Would be great to have this feature. -- To view, visit http://gerrit.cloudera.org:8080/3863 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7adbeb45220c468e43b424d70c30b952f6cec2cd Gerrit-PatchSet: 19 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Kathy Sun Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Kathy Sun Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4277: allow overriding of Hive/Hadoop versions/locations
Sailesh Mukil has posted comments on this change. Change subject: IMPALA-4277: allow overriding of Hive/Hadoop versions/locations .. Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/4720/2/bin/impala-config.sh File bin/impala-config.sh: PS2, Line 333: exanded "expanded" PS2, Line 335: ${HADOOP_HOME}/share/hadoop/tools/lib/* Just checking, but does this need updating too? PS2, Line 404: ${HADOOP_HOME}/lib/native/ There are more here. PS2, Line 424: ${HADOOP_HOME}/lib/native And here. -- To view, visit http://gerrit.cloudera.org:8080/4720 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7da10e38f9c4309f2d193dc25f14a6ea308c9639 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Charlie Helin Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] Not for review: IR for IMPALA-4231: fix codegen time regression
Tim Armstrong has abandoned this change. Change subject: Not for review: IR for IMPALA-4231: fix codegen time regression .. Abandoned -- To view, visit http://gerrit.cloudera.org:8080/4649 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: abandon Gerrit-Change-Id: I1900e7605b6c7eeb6f2f14c7660887137f9e4d16 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong
[Impala-ASF-CR] Remove Llama dependency
Tim Armstrong has posted comments on this change. Change subject: Remove Llama dependency .. Patch Set 2: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/4739 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If2e5e21d8047097d56062ded11b0832a1d397fe0 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] Remove Llama dependency
Tim Armstrong has posted comments on this change. Change subject: Remove Llama dependency .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/4739/1/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: Line 141: // TODO: retire at compatibility-breaking version > maybe we should create a JIRA for these things and then reference the jira Done -- To view, visit http://gerrit.cloudera.org:8080/4739 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If2e5e21d8047097d56062ded11b0832a1d397fe0 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] Remove Llama dependency
Hello Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4739 to look at the new patch set (#2). Change subject: Remove Llama dependency .. Remove Llama dependency This change prevents us from depending on LLAMA to build. Note that the LLAMA MiniKDC is left in - it is a test utility that does not depend on LLAMA itself. IMPALA-4292 tracks cleaning this up. Testing: Ran a private build to verify that all tests pass. Change-Id: If2e5e21d8047097d56062ded11b0832a1d397fe0 --- M bin/bootstrap_toolchain.py M bin/generate_minidump_collection_testdata.py M bin/impala-config.sh M bin/start-impala-cluster.py M common/thrift/ImpalaInternalService.thrift M common/thrift/ImpalaService.thrift M common/thrift/generate_metrics.py M common/thrift/metrics.json M infra/deploy/deploy.py 9 files changed, 6 insertions(+), 43 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/39/4739/2 -- To view, visit http://gerrit.cloudera.org:8080/4739 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: If2e5e21d8047097d56062ded11b0832a1d397fe0 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht
[Impala-ASF-CR](asf-site) Follow Apache Project Branding Requirements
Henry Robinson has posted comments on this change. Change subject: Follow Apache Project Branding Requirements .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/4683/1/bylaws.html File bylaws.html: PS1, Line 134: : : : I don't quite understand this change - is all the content going in the masthead or navbar div now? http://gerrit.cloudera.org:8080/#/c/4683/1/downloads.html File downloads.html: PS1, Line 176: : : : Apache Impala is an effort undergoing incubation at the Apache Software Foundation : (ASF), sponsored by the Apache Incubator PMC. Incubation is required of all newly : accepted projects until a further review indicates that the infrastructure, : communications, and decision making process have stabilized in a manner consistent : with other successful ASF projects. While incubation status is not necessarily a : reflection of the completeness or stability of the code, it does indicate that the : project has yet to be fully endorsed by the ASF. : : Apache Impala, Impala, Apache, the Apache feather logo, and the Apache Impala : project logo are either registered trademarks or trademarks of The Apache Software : Foundation in the United States and other countries. : : Can you try and keep the indentation consistent for s? It's hard to see where this lines up relative to the navbar div above. -- To view, visit http://gerrit.cloudera.org:8080/4683 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I263790d7c9aba7bfc57de7c16a367fe5204c49ff Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: asf-site Gerrit-Owner: Jim Apple Gerrit-Reviewer: Henry Robinson Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4277: allow overriding of Hive/Hadoop versions/locations
Tim Armstrong has posted comments on this change. Change subject: IMPALA-4277: allow overriding of Hive/Hadoop versions/locations .. Patch Set 1: I updated the HADOOP_HOME/lib reference I think HADOOP_LZO shouldn't need updating at this stage since it's a test-only dependency, and for IMPALA_LZO it should always be pointing at the IMPALA_LZO source repo, so there isn't really any variation in layout to configure. -- To view, visit http://gerrit.cloudera.org:8080/4720 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7da10e38f9c4309f2d193dc25f14a6ea308c9639 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Charlie Helin Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4277: allow overriding of Hive/Hadoop versions/locations
Tim Armstrong has uploaded a new patch set (#2). Change subject: IMPALA-4277: allow overriding of Hive/Hadoop versions/locations .. IMPALA-4277: allow overriding of Hive/Hadoop versions/locations This is to help with IMPALA-4277 to make it easier to build against Hadoop/Hive distributions where the directory layout doesn't exactly match our current CDH dependencies, or where we may want to temporarily override a version without making a source change. Change-Id: I7da10e38f9c4309f2d193dc25f14a6ea308c9639 --- M bin/impala-config.sh M buildall.sh M cmake_modules/FindHDFS.cmake M common/thrift/CMakeLists.txt 4 files changed, 24 insertions(+), 18 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/4720/2 -- To view, visit http://gerrit.cloudera.org:8080/4720 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7da10e38f9c4309f2d193dc25f14a6ea308c9639 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Charlie Helin Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] Remove Llama dependency
Dan Hecht has posted comments on this change. Change subject: Remove Llama dependency .. Patch Set 1: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/4739/1/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: Line 141: // TODO: retire at compatibility-breaking version maybe we should create a JIRA for these things and then reference the jira here, so that we don't forget to do it. -- To view, visit http://gerrit.cloudera.org:8080/4739 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If2e5e21d8047097d56062ded11b0832a1d397fe0 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4080, IMPALA-3638: Introduce ExecNode::Codegen()
Dan Hecht has posted comments on this change. Change subject: IMPALA-4080, IMPALA-3638: Introduce ExecNode::Codegen() .. Patch Set 4: Do we still plan to make the ExecNodes' Codegen() be static (in a future patch), or has that plan changed? -- To view, visit http://gerrit.cloudera.org:8080/4651 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I207566bc9f4c6a159271ecdbc4bbdba3d78c6651 Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4277: allow overriding of Hive/Hadoop versions/locations
Charlie Helin has posted comments on this change. Change subject: IMPALA-4277: allow overriding of Hive/Hadoop versions/locations .. Patch Set 1: -Code-Review > Did a grep for HADOOP_HOME, and this might need to be changed too: > https://github.com/apache/incubator-impala/blob/75a857c0ceb6a58691ff45f0df8d88586edc8acd/buildall.sh#L357 Yes here it seems like ${HADOOP_HOME}\lib should categorically be substituted by ${HADOOP_LIB_DIR}. But regarding this line it also looks like we need to parameterize HADOOP LZO? -- To view, visit http://gerrit.cloudera.org:8080/4720 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7da10e38f9c4309f2d193dc25f14a6ea308c9639 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Charlie Helin Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3920: TotalStorageWaitTime counter not populated for fragments with Kudu scan node
Dan Hecht has posted comments on this change. Change subject: IMPALA-3920: TotalStorageWaitTime counter not populated for fragments with Kudu scan node .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/4639 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If0c793930799fdcaff53e705f94b52cadac2f53a Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: anujphadke Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4277: allow overriding of Hive/Hadoop versions/locations
Sailesh Mukil has posted comments on this change. Change subject: IMPALA-4277: allow overriding of Hive/Hadoop versions/locations .. Patch Set 1: Did a grep for HADOOP_HOME, and this might need to be changed too: https://github.com/apache/incubator-impala/blob/75a857c0ceb6a58691ff45f0df8d88586edc8acd/buildall.sh#L357 -- To view, visit http://gerrit.cloudera.org:8080/4720 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7da10e38f9c4309f2d193dc25f14a6ea308c9639 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Charlie Helin Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3504: UDF for current timestamp in UTC
Dan Hecht has posted comments on this change. Change subject: IMPALA-3504: UDF for current timestamp in UTC .. Patch Set 8: (1 comment) http://gerrit.cloudera.org:8080/#/c/4490/8/be/src/service/impala-server.cc File be/src/service/impala-server.cc: PS8, Line 842: query_ctx->__set_utc_timestamp_string(TimestampValue::UtcTime().DebugString()); : query_ctx->__set_now_string(TimestampValue::LocalTime().DebugString()); it would be better if NOW() and UTC_TIMESTAMP() return the same instance in time (represented in different timezones). The current implementation will return slightly different instances in time. -- To view, visit http://gerrit.cloudera.org:8080/4490 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5ee6ee192aa469f77c711f27ad324696a42004d1 Gerrit-PatchSet: 8 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Youwei Wang Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Youwei Wang Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4216: Test became flaky: TestTpchMemLimitError.test low mem limit q20
Dan Hecht has posted comments on this change. Change subject: IMPALA-4216: Test became flaky: TestTpchMemLimitError.test_low_mem_limit_q20 .. Patch Set 1: But why do we no longer get the MEM_LIMIT_EXCEEDED error status returned by this query? -- To view, visit http://gerrit.cloudera.org:8080/4572 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I471a90c8e3bdecb4fed08fbae3436c50b8618471 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Attila Jeges Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4277: allow overriding of Hive/Hadoop versions/locations
Tim Armstrong has posted comments on this change. Change subject: IMPALA-4277: allow overriding of Hive/Hadoop versions/locations .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/4720/1/bin/impala-config.sh File bin/impala-config.sh: Line 305: export IMPALA_LLAMA_MINIKDC_VERSION=${IMPALA_LLAMA_MINIKDC_VERSION:-1.0.0} > This is actually separate from Llama - a test utility we stole from an old I'm also going to post a separate patch to remove Llama (needed to do some testing there to make sure it didn't break anything). PS1, Line 341: $HADOOP_HOME/bin > If HADOOP_INCLUDE_DIR and HADOOP_LIB_DIR point to a different path than HAD I think if they were set to incompatible versions it might cause a problem since we'd be building against the wrong version of Hadoop (e.g Hadoop 2 API versus Hadoop 3 API). I think it's reasonable to assume that it won't be set up to point to incompatible versions. -- To view, visit http://gerrit.cloudera.org:8080/4720 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7da10e38f9c4309f2d193dc25f14a6ea308c9639 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Charlie Helin Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4253: impala-server.backends.client-cache.total-clients shows negative value
Dan Hecht has posted comments on this change. Change subject: IMPALA-4253: impala-server.backends.client-cache.total-clients shows negative value .. Patch Set 1: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/4668/1/be/src/runtime/client-cache.cc File be/src/runtime/client-cache.cc: PS1, Line 95: // Only need to decrement it here. the "if succeeded" explains this, so you could remove this sentence, which I think is a bit confusing (not clear what "here" means -- this function, this if-stmt, etc?). -- To view, visit http://gerrit.cloudera.org:8080/4668 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic9e28055cb232cdb543c4c9f05a558ab0f73f777 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Juan Yu Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4277: allow overriding of Hive/Hadoop versions/locations
Sailesh Mukil has posted comments on this change. Change subject: IMPALA-4277: allow overriding of Hive/Hadoop versions/locations .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/4720/1/bin/impala-config.sh File bin/impala-config.sh: PS1, Line 341: $HADOOP_HOME/bin If HADOOP_INCLUDE_DIR and HADOOP_LIB_DIR point to a different path than HADOOP_HOME/(include)|(lib)/ , wouldn't this cause an issue? -- To view, visit http://gerrit.cloudera.org:8080/4720 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7da10e38f9c4309f2d193dc25f14a6ea308c9639 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Charlie Helin Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR](hadoop-next) IMPALA-4277: bump Hadoop component versions except for Hadoop itself
Sailesh Mukil has posted comments on this change. Change subject: IMPALA-4277: bump Hadoop component versions except for Hadoop itself .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/4698 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia0be6bbe76d929ceaeb2fa9ac4ebba1820c4dab7 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: hadoop-next Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Charlie Helin Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No