[Impala-ASF-CR] IMPALA-7694: Add host resource usage metrics to profile
Michael Ho has posted comments on this change. (
http://gerrit.cloudera.org:8080/12069 )
Change subject: IMPALA-7694: Add host resource usage metrics to profile
..
Patch Set 11:
(12 comments)
Still wrapping my head around this big change. Some initial comments for now.
http://gerrit.cloudera.org:8080/#/c/12069/11/be/src/runtime/coordinator-backend-state.h
File be/src/runtime/coordinator-backend-state.h:
http://gerrit.cloudera.org:8080/#/c/12069/11/be/src/runtime/coordinator-backend-state.h@67
PS11, Line 67: RuntimeProfile* host_profiles
nit: Please add a comment on the role of this parameter.
http://gerrit.cloudera.org:8080/#/c/12069/11/be/src/runtime/coordinator-backend-state.h@249
PS11, Line 249: host_profile_ = nullptr;
nit: A brief statement of what this contains.
http://gerrit.cloudera.org:8080/#/c/12069/11/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:
http://gerrit.cloudera.org:8080/#/c/12069/11/be/src/runtime/query-state.cc@296
PS11, Line 296: // Add profile to report
: host_profile_->ToThrift(_forest->host_profile);
: profiles_forest->__isset.host_profile = true;
: // Free resources in chunked counters in the profile
: host_profile_->ClearChunkedTimeSeriesCounters();
I wonder how this may reconcile with https://gerrit.cloudera.org/#/c/12049/ ?
In particular, with the mentioned patch, on a failure to send the profile
report, the query state thread will back off, and retry sending a new profile.
So, the host profile is potentially another non-idempotent state added to the
report. That said, how tolerant is the coordinator's side logic on handling
missing chunks in this time series ? In other words, if we fail to send a
report, how bad is it to let the host profile of this epoch to be dropped ?
http://gerrit.cloudera.org:8080/#/c/12069/11/be/src/runtime/runtime-state.cc
File be/src/runtime/runtime-state.cc:
http://gerrit.cloudera.org:8080/#/c/12069/11/be/src/runtime/runtime-state.cc@76
PS11, Line 76: profile_(RuntimeProfile::Create(obj_pool(), "")),
Why this change ?
http://gerrit.cloudera.org:8080/#/c/12069/11/be/src/util/runtime-profile-counters.h
File be/src/util/runtime-profile-counters.h:
http://gerrit.cloudera.org:8080/#/c/12069/11/be/src/util/runtime-profile-counters.h@443
PS11, Line 443: previous_sample_count_ = 0;
This deserves a comment.
http://gerrit.cloudera.org:8080/#/c/12069/11/be/src/util/runtime-profile.h
File be/src/util/runtime-profile.h:
http://gerrit.cloudera.org:8080/#/c/12069/11/be/src/util/runtime-profile.h@419
PS11, Line 419: Samples
: /// are not re-sampled into larger intervals, instead owners
of this profile can call
: /// ClearChunkedTimeSeriesCounters() to reset the sample
buffers of all chunked time
: /// series counters
So, misuse of this kind of counter (e.g. never calling
ClearChunkedTimeSeriesCounters) may lead to large untracked memory usage, right
?
http://gerrit.cloudera.org:8080/#/c/12069/11/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:
http://gerrit.cloudera.org:8080/#/c/12069/11/be/src/util/runtime-profile.cc@1216
PS11, Line 1216:
nit: blank space
http://gerrit.cloudera.org:8080/#/c/12069/11/be/src/util/runtime-profile.cc@1231
PS11, Line 1231:
nit: blank space
http://gerrit.cloudera.org:8080/#/c/12069/11/be/src/util/runtime-profile.cc@1279
PS11, Line 1279: DCHECK(
DCHECK_EQ
http://gerrit.cloudera.org:8080/#/c/12069/11/be/src/util/system-state-info.h
File be/src/util/system-state-info.h:
http://gerrit.cloudera.org:8080/#/c/12069/11/be/src/util/system-state-info.h@66
PS11, Line 66: /// CaptureSystemStateSnapshot().
'cpu_ratios_' is updated after returning from this function.
http://gerrit.cloudera.org:8080/#/c/12069/11/be/src/util/system-state-info.h@69
PS11, Line 69: NUM_CPU_VALUES = 5;
Seems more future proof to have this as the last value in the enum below like:
enum PROC_STAT_CPU_VALUES {
CPU_USER = 0,
CPU_NICE,
CPU_SYSTEM,
CPU_IDLE,
CPU_IOWAIT,
NUM_CPU_VALUES
};
http://gerrit.cloudera.org:8080/#/c/12069/11/be/src/util/system-state-info.cc
File be/src/util/system-state-info.cc:
http://gerrit.cloudera.org:8080/#/c/12069/11/be/src/util/system-state-info.cc@44
PS11, Line 44: int64_t SystemStateInfo::ParseInt64(const string& val) const {
: StringParser::ParseResult result;
: int64_t parsed =
StringParser::StringToInt(val.c_str(), val.size(), );
: if (result == StringParser::PARSE_SUCCESS) return parsed;
: return -1;
: }
Wonder if this fits better as a utility function in StringParser ?
--
To view, visit http://gerrit.cloudera.org:8080/12069
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
[Impala-ASF-CR] IMPALA-7941: part 2/2: use cgroups memory limit
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12262 ) Change subject: IMPALA-7941: part 2/2: use cgroups memory limit .. Patch Set 1: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/12262 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie9fb4fb936a46fc194a204391d03c07c8c7fba21 Gerrit-Change-Number: 12262 Gerrit-PatchSet: 1 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 24 Jan 2019 05:09:28 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8041, Part 1: Move rewrite rules into expr nodes
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12143 ) Change subject: IMPALA-8041, Part 1: Move rewrite rules into expr nodes .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1874/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifda7082abea4f6448e6cfef0d9ef7b8565d73fce Gerrit-Change-Number: 12143 Gerrit-PatchSet: 3 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Thu, 24 Jan 2019 04:43:20 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8041, Part 1: Move rewrite rules into expr nodes
Hello Bharath Vissapragada, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12143 to look at the new patch set (#3). Change subject: IMPALA-8041, Part 1: Move rewrite rules into expr nodes .. IMPALA-8041, Part 1: Move rewrite rules into expr nodes This patch is part of the task to integrate expression rewrites into the analysis process to avoid the need for two analysis passes. The analyzer provides a rewrite engine which traverses the expression tree and compares each node against a list of rewrite rules, typically by performing an "instanceof" comparison of the node with the target node type. This project seeks to apply the rules as we analyze each node. The simplest way to do so is for each node to hold the code for its own rewrites, callable via a virtual method: rewrite(). This patch is a first step: the rewrite code moves from the various rewrite rules into the expression node that is to be rewritten. In some cases nodes have a single rule which is moved from the rewrite rule directly into the rewrite() method. In other cases, a node has multiple rules. The different rules for the same node are moved into distinct methods. Though these methods are all called by the main rewrite() method, nothing calls that rewrite() method in this patch. Finally, this patch introduces a "stub" expression analyzer. In a later patch this class will drive the combined analysis/rewrite logic. For now, it is mostly a placeholder. The rewrite rules themselves are now just stubs retained to minimze change. A future patch will retire them once the analysis/rewrite integration is complete. This patch is designed to have no functional change: code merely moves from one location (the rewrite rules) to another (the expression nodes). Some "shim" code is added, but the functionality is unchanged. Tests: reran all FE tests to verify no change in behavior. Change-Id: Ifda7082abea4f6448e6cfef0d9ef7b8565d73fce --- M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java M fe/src/main/java/org/apache/impala/analysis/Expr.java A fe/src/main/java/org/apache/impala/analysis/ExprAnalyzer.java M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java M fe/src/main/java/org/apache/impala/common/TreeNode.java M fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java M fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java M fe/src/main/java/org/apache/impala/rewrite/ExprRewriter.java M fe/src/main/java/org/apache/impala/rewrite/ExtractCommonConjunctRule.java M fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java M fe/src/main/java/org/apache/impala/rewrite/NormalizeCountStarRule.java M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java M fe/src/main/java/org/apache/impala/rewrite/SimplifyDistinctFromRule.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java 18 files changed, 686 insertions(+), 378 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/12143/3 -- To view, visit http://gerrit.cloudera.org:8080/12143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ifda7082abea4f6448e6cfef0d9ef7b8565d73fce Gerrit-Change-Number: 12143 Gerrit-PatchSet: 3 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers
[Impala-ASF-CR] IMPALA-8041, Part 1: Move rewrite rules into expr nodes
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12143 ) Change subject: IMPALA-8041, Part 1: Move rewrite rules into expr nodes .. Patch Set 2: Rebased on latest master -- To view, visit http://gerrit.cloudera.org:8080/12143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifda7082abea4f6448e6cfef0d9ef7b8565d73fce Gerrit-Change-Number: 12143 Gerrit-PatchSet: 2 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Thu, 24 Jan 2019 03:44:03 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7832: Support for IF NOT EXISTS in ALTER TABLE ADD COLUMN(S)
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12181 ) Change subject: IMPALA-7832: Support for IF NOT EXISTS in ALTER TABLE ADD COLUMN(S) .. Patch Set 9: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/12181 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I60ed22c8a8eefa10e94ad3dedf32fe67c16642d9 Gerrit-Change-Number: 12181 Gerrit-PatchSet: 9 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Thu, 24 Jan 2019 03:14:33 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7832: Support for IF NOT EXISTS in ALTER TABLE ADD COLUMN(S)
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12181 ) Change subject: IMPALA-7832: Support for IF NOT EXISTS in ALTER TABLE ADD COLUMN(S) .. IMPALA-7832: Support for IF NOT EXISTS in ALTER TABLE ADD COLUMN(S) This patch adds IF NOT EXISTS support in ALTER TABLE ADD COLUMN and ALTER TABLE ADD COLUMNS. If IF NOT EXISTS is specified and a column already exists with this name, no error is thrown. If IF NOT EXISTS is specified for multiple columns and a column already exists, no error is thrown and a new column that does not exist will be added. Syntax: ALTER TABLE tbl ADD COLUMN [IF NOT EXISTS] i int ALTER TABLE tbl ADD [IF NOT EXISTS] COLUMNS (i int, j int) Testing: - Added new FE tests - Ran all FE tests - Updated E2E DDL tests - Ran all E2E DDL tests Change-Id: I60ed22c8a8eefa10e94ad3dedf32fe67c16642d9 Reviewed-on: http://gerrit.cloudera.org:8080/12181 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M common/thrift/JniCatalog.thrift M fe/src/main/cup/sql-parser.cup R fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java C fe/src/main/java/org/apache/impala/analysis/AlterTableReplaceColsStmt.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java M fe/src/test/java/org/apache/impala/analysis/ParserTest.java M testdata/workloads/functional-query/queries/QueryTest/alter-table.test M testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test M testdata/workloads/functional-query/queries/QueryTest/kudu_insert.test 11 files changed, 398 insertions(+), 182 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/12181 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I60ed22c8a8eefa10e94ad3dedf32fe67c16642d9 Gerrit-Change-Number: 12181 Gerrit-PatchSet: 10 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers
[Impala-ASF-CR] IMPALA-7941: part 2/2: use cgroups memory limit
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12262 ) Change subject: IMPALA-7941: part 2/2: use cgroups memory limit .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1873/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12262 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie9fb4fb936a46fc194a204391d03c07c8c7fba21 Gerrit-Change-Number: 12262 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 24 Jan 2019 02:09:20 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7941: part 2/2: use cgroups memory limit
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12262 ) Change subject: IMPALA-7941: part 2/2: use cgroups memory limit .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1872/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12262 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie9fb4fb936a46fc194a204391d03c07c8c7fba21 Gerrit-Change-Number: 12262 Gerrit-PatchSet: 1 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 24 Jan 2019 01:56:54 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8095: Detailed expression cardinality tests
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12248 ) Change subject: IMPALA-8095: Detailed expression cardinality tests .. Patch Set 5: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1871/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12248 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3da58ee9b0beebeffb170b9430bd36d20dcd2401 Gerrit-Change-Number: 12248 Gerrit-PatchSet: 5 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 24 Jan 2019 01:40:48 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7905: Hive keywords not quoted for identifiers
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12009 ) Change subject: IMPALA-7905: Hive keywords not quoted for identifiers .. IMPALA-7905: Hive keywords not quoted for identifiers Impala often generates SQL for statements using the toSql() call. Generated SQL is often used during testing or when writing the query plan. Impala keywords such as "create", when used as identifiers, must be quoted: SELECT `select`, `from` FROM `order` ... The code in ToSqlUtils.getIdentSql() quotes the identifier if it is an Impala or Hive keyword, or if it does not follow the identifier pattern. The code uses the Hive lexer to detect a keyword. But, the code contained a flaw: the lexer expects a case-insensitive input. We provide a case sensitive input. As a result, "MONTH" is caught as a Hive keyword and quoted, but "month" is not. This patch fixes that flaw. This patch also fixes: IMPALA-8051: Compute stats fails on a column with comment character in name The code uses the Hive lexical analyzer to check names. Since "#" and "--" are comment characters, a name like "foo#" is parsed as "foo" which does not need quotes, hence we don't quote "foo#", which causes issues. Added a special check for "#" and "--" to resolve this issue. Testing: * Refactored getIdentSql() easier testing. * Added a tests to the recently added ToSqlUtilsTest for this case and several others. * Making this change caused the columns `month`, `year`, and `key` to be quoted when before they were not. Updated many tests as a result. * Added a new identSql() function, for use in tests, to match the quoting that Impala uses, and to handle the wildcard, and multi-part names. Used this in ToSqlTest to handle the quoted names. * PlannerTest emits statement SQL to the output file wrapped to 80 columns and sometimes leaves trailing spaces at the end of the line. Some tools remove that trailing space, resulting in trivial file differences. Fixed this to remove trailing spaces in order to simplify file comparisons. * Tweaked the "In pipelines" output to avoid trailing spaces when no pipelines are listed. * Reran all FE tests. Change-Id: I06cc20b052a3a66535a171c36b4b31477c0ba6d0 Reviewed-on: http://gerrit.cloudera.org:8080/12009 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java M fe/src/main/java/org/apache/impala/common/PrintUtils.java M fe/src/main/java/org/apache/impala/planner/PlanNode.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlUtilsTest.java M fe/src/test/java/org/apache/impala/planner/PlannerTest.java M fe/src/test/java/org/apache/impala/util/PrintUtilsTest.java M testdata/workloads/functional-planner/queries/PlannerTest/aggregation.test M testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test M testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test M testdata/workloads/functional-planner/queries/PlannerTest/ddl.test M testdata/workloads/functional-planner/queries/PlannerTest/hbase.test M testdata/workloads/functional-planner/queries/PlannerTest/hdfs.test M testdata/workloads/functional-planner/queries/PlannerTest/implicit-joins.test M testdata/workloads/functional-planner/queries/PlannerTest/inline-view.test M testdata/workloads/functional-planner/queries/PlannerTest/insert-sort-by.test M testdata/workloads/functional-planner/queries/PlannerTest/insert.test M testdata/workloads/functional-planner/queries/PlannerTest/join-order.test M testdata/workloads/functional-planner/queries/PlannerTest/joins.test M testdata/workloads/functional-planner/queries/PlannerTest/kudu-upsert.test M testdata/workloads/functional-planner/queries/PlannerTest/lineage.test M testdata/workloads/functional-planner/queries/PlannerTest/nested-collections.test M testdata/workloads/functional-planner/queries/PlannerTest/order.test M testdata/workloads/functional-planner/queries/PlannerTest/parquet-filtering-disabled.test M testdata/workloads/functional-planner/queries/PlannerTest/parquet-filtering.test M testdata/workloads/functional-planner/queries/PlannerTest/parquet-stats-agg.test M testdata/workloads/functional-planner/queries/PlannerTest/partition-key-scans.test M testdata/workloads/functional-planner/queries/PlannerTest/predicate-propagation.test M
[Impala-ASF-CR] IMPALA-7905: Hive keywords not quoted for identifiers
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12009 ) Change subject: IMPALA-7905: Hive keywords not quoted for identifiers .. Patch Set 13: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/12009 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I06cc20b052a3a66535a171c36b4b31477c0ba6d0 Gerrit-Change-Number: 12009 Gerrit-PatchSet: 13 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Thu, 24 Jan 2019 01:26:07 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7841 (Part 1): Refactor SelectStmt for easier debugging
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11915 ) Change subject: IMPALA-7841 (Part 1): Refactor SelectStmt for easier debugging .. IMPALA-7841 (Part 1): Refactor SelectStmt for easier debugging Builds on IMPALA-7808 with several additional refactorings: * IMPALA-7808 minimized code changes. This change cleans up the new functions, removing if's and merging the "aggregation" function with the body of the new analyze() function. * Removed an unneeded analyzer argument. This is all refactoring: there is no functional change. Testing: Reran existing tests to ensure that functionality remained unchanged. Change-Id: I4f4fe3d3a1ab3170e294714dab066d40d169eff1 Reviewed-on: http://gerrit.cloudera.org:8080/11915 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/analysis/CollectionTableRef.java M fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java M fe/src/main/java/org/apache/impala/analysis/QueryStmt.java M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java M fe/src/main/java/org/apache/impala/analysis/Subquery.java M fe/src/main/java/org/apache/impala/analysis/WithClause.java 7 files changed, 93 insertions(+), 99 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/11915 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I4f4fe3d3a1ab3170e294714dab066d40d169eff1 Gerrit-Change-Number: 11915 Gerrit-PatchSet: 5 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers
[Impala-ASF-CR] IMPALA-7841 (Part 1): Refactor SelectStmt for easier debugging
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11915 ) Change subject: IMPALA-7841 (Part 1): Refactor SelectStmt for easier debugging .. Patch Set 4: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/11915 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4f4fe3d3a1ab3170e294714dab066d40d169eff1 Gerrit-Change-Number: 11915 Gerrit-PatchSet: 4 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Thu, 24 Jan 2019 01:12:55 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7941: part 2/2: use cgroups memory limit
Tim Armstrong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12262 Change subject: IMPALA-7941: part 2/2: use cgroups memory limit .. IMPALA-7941: part 2/2: use cgroups memory limit This uses the functionality from part 1 to detect the CGroups memory limit and use it to set a lower process memory limit if needed. min(system memory, cgroups memory limit) is used instead of system memory to determine the memory limit. Behaviour of processes without a memory limit set via CGroups is unchanged. The default behaviour of using 80% of the memory limit detected is still in effect. This seems like an OK default, but may lead to some amount of wasted memory. Modify containers to have a default JVM heap size of 2GB and --mem_limit_includes_jvm, so that the automatically configured memory limit makes more sense. start-impala-cluster.py is modified to exercise all of this. Testing: Tested a containerised cluster manually on my system, which has 32GB of RAM. Here's the breakdown from the memz/ page showing the JVM heap and auto-configured memory limit. Process: Limit=7.31 GB Total=1.94 GB Peak=1.94 GB JVM: max heap size: Total=1.78 GB JVM: non-heap committed: Total=35.56 MB Buffer Pool: Free Buffers: Total=0 Buffer Pool: Clean Pages: Total=0 Buffer Pool: Unused Reservation: Total=0 Control Service Queue: Limit=50.00 MB Total=0 Peak=0 Data Stream Service Queue: Limit=374.27 MB Total=0 Peak=0 Data Stream Manager Early RPCs: Total=0 Peak=0 TCMalloc Overhead: Total=12.20 MB Untracked Memory: Total=121.31 MB Change-Id: Ie9fb4fb936a46fc194a204391d03c07c8c7fba21 --- M be/src/runtime/exec-env.cc M bin/start-impala-cluster.py M docker/coord_exec/Dockerfile M docker/coordinator/Dockerfile M docker/daemon_entrypoint.sh M docker/executor/Dockerfile 6 files changed, 58 insertions(+), 35 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/62/12262/1 -- To view, visit http://gerrit.cloudera.org:8080/12262 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ie9fb4fb936a46fc194a204391d03c07c8c7fba21 Gerrit-Change-Number: 12262 Gerrit-PatchSet: 1 Gerrit-Owner: Tim Armstrong
[Impala-ASF-CR] IMPALA-7941: part 2/2: use cgroups memory limit
Hello Joe McDonnell, Bikramjeet Vig, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12262 to look at the new patch set (#2). Change subject: IMPALA-7941: part 2/2: use cgroups memory limit .. IMPALA-7941: part 2/2: use cgroups memory limit This uses the functionality from part 1 to detect the CGroups memory limit and use it to set a lower process memory limit if needed. min(system memory, cgroups memory limit) is used instead of system memory to determine the memory limit. Behaviour of processes without a memory limit set via CGroups is unchanged. The default behaviour of using 80% of the memory limit detected is still in effect. This seems like an OK default, but may lead to some amount of wasted memory. Modify containers to have a default JVM heap size of 2GB and --mem_limit_includes_jvm, so that the automatically configured memory limit makes more sense. start-impala-cluster.py is modified to exercise all of this. Testing: Tested a containerised cluster manually on my system, which has 32GB of RAM. Here's the breakdown from the memz/ page showing the JVM heap and auto-configured memory limit. Process: Limit=7.31 GB Total=1.94 GB Peak=1.94 GB JVM: max heap size: Total=1.78 GB JVM: non-heap committed: Total=35.56 MB Buffer Pool: Free Buffers: Total=0 Buffer Pool: Clean Pages: Total=0 Buffer Pool: Unused Reservation: Total=0 Control Service Queue: Limit=50.00 MB Total=0 Peak=0 Data Stream Service Queue: Limit=374.27 MB Total=0 Peak=0 Data Stream Manager Early RPCs: Total=0 Peak=0 TCMalloc Overhead: Total=12.20 MB Untracked Memory: Total=121.31 MB Change-Id: Ie9fb4fb936a46fc194a204391d03c07c8c7fba21 --- M be/src/runtime/exec-env.cc M bin/start-impala-cluster.py M docker/coord_exec/Dockerfile M docker/coordinator/Dockerfile M docker/daemon_entrypoint.sh M docker/executor/Dockerfile 6 files changed, 59 insertions(+), 36 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/62/12262/2 -- To view, visit http://gerrit.cloudera.org:8080/12262 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie9fb4fb936a46fc194a204391d03c07c8c7fba21 Gerrit-Change-Number: 12262 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell
[Impala-ASF-CR] IMPALA-7941: part 2/2: use cgroups memory limit
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12262 ) Change subject: IMPALA-7941: part 2/2: use cgroups memory limit .. Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/12262/1/bin/start-impala-cluster.py File bin/start-impala-cluster.py: http://gerrit.cloudera.org:8080/#/c/12262/1/bin/start-impala-cluster.py@298 PS1, Line 298: .format(compute_impalad_mem_limit(cluster_size) Need to remove this .format(), it's a noop http://gerrit.cloudera.org:8080/#/c/12262/1/bin/start-impala-cluster.py@497 PS1, Line 497: mem_limit = compute_impalad_mem_limit(cluster_size) Can move this outside loop http://gerrit.cloudera.org:8080/#/c/12262/1/bin/start-impala-cluster.py@535 PS1, Line 535: # TODO: docker memory limit remove -- To view, visit http://gerrit.cloudera.org:8080/12262 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie9fb4fb936a46fc194a204391d03c07c8c7fba21 Gerrit-Change-Number: 12262 Gerrit-PatchSet: 1 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 24 Jan 2019 00:55:15 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7941: part 2/2: use cgroups memory limit
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12262 ) Change subject: IMPALA-7941: part 2/2: use cgroups memory limit .. Patch Set 1: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3669/ DRY_RUN=true -- To view, visit http://gerrit.cloudera.org:8080/12262 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie9fb4fb936a46fc194a204391d03c07c8c7fba21 Gerrit-Change-Number: 12262 Gerrit-PatchSet: 1 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 24 Jan 2019 00:51:17 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8095: Detailed expression cardinality tests
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12248 ) Change subject: IMPALA-8095: Detailed expression cardinality tests .. Patch Set 5: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/12248 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3da58ee9b0beebeffb170b9430bd36d20dcd2401 Gerrit-Change-Number: 12248 Gerrit-PatchSet: 5 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 24 Jan 2019 00:39:46 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8095: Detailed expression cardinality tests
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12248 ) Change subject: IMPALA-8095: Detailed expression cardinality tests .. Patch Set 5: (4 comments) Thanks for the review Tim. Addressed review comments. http://gerrit.cloudera.org:8080/#/c/12248/4/fe/src/test/java/org/apache/impala/analysis/ExprCardinalityTest.java File fe/src/test/java/org/apache/impala/analysis/ExprCardinalityTest.java: http://gerrit.cloudera.org:8080/#/c/12248/4/fe/src/test/java/org/apache/impala/analysis/ExprCardinalityTest.java@168 PS4, Line 168: // Stats, no null values > nit: blank line at function start doesn't help readability Done http://gerrit.cloudera.org:8080/#/c/12248/4/fe/src/test/java/org/apache/impala/analysis/ExprNdvTest.java File fe/src/test/java/org/apache/impala/analysis/ExprNdvTest.java: http://gerrit.cloudera.org:8080/#/c/12248/4/fe/src/test/java/org/apache/impala/analysis/ExprNdvTest.java@162 PS4, Line 162: > Doesn't Impala set it with compute stats after IMPALA-7659 was merged? Thanks. This was a batch of tests from way back I'm merging. The comment was obsolete. http://gerrit.cloudera.org:8080/#/c/12248/4/fe/src/test/java/org/apache/impala/common/AnalysisSessionFixture.java File fe/src/test/java/org/apache/impala/common/AnalysisSessionFixture.java: http://gerrit.cloudera.org:8080/#/c/12248/4/fe/src/test/java/org/apache/impala/common/AnalysisSessionFixture.java@45 PS4, Line 45: private final FrontendFixture feFixture_ = FrontendFixture.instance(); > private? I didn't see a case where this was accessed from a different class Done http://gerrit.cloudera.org:8080/#/c/12248/4/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java File fe/src/test/java/org/apache/impala/common/FrontendTestBase.java: http://gerrit.cloudera.org:8080/#/c/12248/4/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java@62 PS4, Line 62: protected static ImpaladTestCatalog catalog_ = feFixture_.catalog(); > Delete commented-out code? Done -- To view, visit http://gerrit.cloudera.org:8080/12248 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3da58ee9b0beebeffb170b9430bd36d20dcd2401 Gerrit-Change-Number: 12248 Gerrit-PatchSet: 5 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 24 Jan 2019 00:38:49 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8095: Detailed expression cardinality tests
Hello Bharath Vissapragada, Fredy Wijaya, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12248 to look at the new patch set (#5). Change subject: IMPALA-8095: Detailed expression cardinality tests .. IMPALA-8095: Detailed expression cardinality tests Cardinality is a critical input to the query planning process, especially join planning. Impala has many high-level end-to-end tests that implicitly test cardinality at the "wholesale" level: A test will produce a wrong result if the cardinality is badly wrong. Until this patch, no detailed unit tests exist for this topic. Tests here include: * Table cardinality, NDV values and null count in metadata retrieved from HMS. * Table cardinality, NDV values and null counts in metadata presented to the query. * Expression NDV and selectivity values (which derive from table cardinality and column NDV.) The test illustrate a number of bugs. This patch simply identifies the bugs, comments out the tests that fail because of the bugs, and substitutes tests that pass with the current, incorrect, behavior. Future patches will fix the bugs. Reviewers can note the difference between the original, incorrect behavior shown here, and the revised behavior in those additional patches. Since none of the existing "functional" tables provide the level of detail needed for these tests, added a new test table specifically for this task. This set of tests was a good time to extend the test "fixture" framework created earlier. The FrontendTestBase class was refactored to use a new FrontendFixture which represents a (simulated) Impala and HMS cluster. The previoius SessionFixture represents a single user session (with session options) and the QueryFixture represents a single query. As part of this refactoring, the fixture classes moved into "common" alongside FrontendTestBase. Testing: This patch includes only tests: no "production" code was changed. Change-Id: I3da58ee9b0beebeffb170b9430bd36d20dcd2401 --- M fe/src/main/java/org/apache/impala/analysis/Expr.java M fe/src/main/java/org/apache/impala/analysis/SelectListItem.java M fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java D fe/src/test/java/org/apache/impala/analysis/AnalysisSessionFixture.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java A fe/src/test/java/org/apache/impala/analysis/ExprCardinalityTest.java M fe/src/test/java/org/apache/impala/analysis/ExprNdvTest.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java A fe/src/test/java/org/apache/impala/common/AbstractFrontendTest.java A fe/src/test/java/org/apache/impala/common/AnalysisSessionFixture.java A fe/src/test/java/org/apache/impala/common/FrontendFixture.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java A fe/src/test/java/org/apache/impala/common/QueryFixture.java M fe/src/test/java/org/apache/impala/planner/CardinalityTest.java A testdata/NullRows/data.csv M testdata/bin/compute-table-stats.sh M testdata/datasets/functional/functional_schema_template.sql M testdata/datasets/functional/schema_constraints.csv 18 files changed, 1,689 insertions(+), 536 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/12248/5 -- To view, visit http://gerrit.cloudera.org:8080/12248 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3da58ee9b0beebeffb170b9430bd36d20dcd2401 Gerrit-Change-Number: 12248 Gerrit-PatchSet: 5 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8095: Detailed expression cardinality tests
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12248 ) Change subject: IMPALA-8095: Detailed expression cardinality tests .. Patch Set 4: Code-Review+1 (4 comments) It's hard to argue with adding more tests for the current behaviour. Had a few minor comments. Thought about +2ing but would be good if someone who knows the frontend better looked at the fixtures to make sure they make sense (they do to me). http://gerrit.cloudera.org:8080/#/c/12248/4/fe/src/test/java/org/apache/impala/analysis/ExprCardinalityTest.java File fe/src/test/java/org/apache/impala/analysis/ExprCardinalityTest.java: http://gerrit.cloudera.org:8080/#/c/12248/4/fe/src/test/java/org/apache/impala/analysis/ExprCardinalityTest.java@168 PS4, Line 168: nit: blank line at function start doesn't help readability http://gerrit.cloudera.org:8080/#/c/12248/4/fe/src/test/java/org/apache/impala/analysis/ExprNdvTest.java File fe/src/test/java/org/apache/impala/analysis/ExprNdvTest.java: http://gerrit.cloudera.org:8080/#/c/12248/4/fe/src/test/java/org/apache/impala/analysis/ExprNdvTest.java@162 PS4, Line 162:* Test the NDV-with-nulls adjustment. At present, cannot find Doesn't Impala set it with compute stats after IMPALA-7659 was merged? http://gerrit.cloudera.org:8080/#/c/12248/4/fe/src/test/java/org/apache/impala/common/AnalysisSessionFixture.java File fe/src/test/java/org/apache/impala/common/AnalysisSessionFixture.java: http://gerrit.cloudera.org:8080/#/c/12248/4/fe/src/test/java/org/apache/impala/common/AnalysisSessionFixture.java@45 PS4, Line 45: final FrontendFixture feFixture_ = FrontendFixture.instance(); private? I didn't see a case where this was accessed from a different class. http://gerrit.cloudera.org:8080/#/c/12248/4/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java File fe/src/test/java/org/apache/impala/common/FrontendTestBase.java: http://gerrit.cloudera.org:8080/#/c/12248/4/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java@62 PS4, Line 62: // protected static ImpaladTestCatalog catalog_ = new ImpaladTestCatalog(); Delete commented-out code? -- To view, visit http://gerrit.cloudera.org:8080/12248 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3da58ee9b0beebeffb170b9430bd36d20dcd2401 Gerrit-Change-Number: 12248 Gerrit-PatchSet: 4 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 24 Jan 2019 00:16:52 + Gerrit-HasComments: Yes
[Impala-ASF-CR] Support centos:7 for test-with-docker.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12139 ) Change subject: Support centos:7 for test-with-docker. .. Support centos:7 for test-with-docker. As a follow-on to IMPALA-7698, adds various incantations so that centos:7 can build under test-with-docker. The core issue is that the centos:7 image doesn't let you start sshd (necessary for the HBase startup scripts, and probably could be worked around) or postgresql (harder to work around) with systemctl, because systemd isn't "running." To avoid this, we start them manually with /usr/sbin/sshd and pg_ctl. Change-Id: I7577949b6eaaa2239bcf0fadf64e1490c2106b08 Reviewed-on: http://gerrit.cloudera.org:8080/12139 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M bin/bootstrap_system.sh M docker/entrypoint.sh 2 files changed, 90 insertions(+), 12 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/12139 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I7577949b6eaaa2239bcf0fadf64e1490c2106b08 Gerrit-Change-Number: 12139 Gerrit-PatchSet: 4 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Laszlo Gaal Gerrit-Reviewer: Philip Zeyliger
[Impala-ASF-CR] Support centos:7 for test-with-docker.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12139 ) Change subject: Support centos:7 for test-with-docker. .. Patch Set 3: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/12139 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7577949b6eaaa2239bcf0fadf64e1490c2106b08 Gerrit-Change-Number: 12139 Gerrit-PatchSet: 3 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Laszlo Gaal Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Thu, 24 Jan 2019 00:09:08 + Gerrit-HasComments: No
[Impala-ASF-CR] Using 'master' branch of Impala-lzo and allowing test-with-docker to configure it.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12259 ) Change subject: Using 'master' branch of Impala-lzo and allowing test-with-docker to configure it. .. Using 'master' branch of Impala-lzo and allowing test-with-docker to configure it. This updates bootstrap_system.sh to check out the 'master' branch of Impala-lzo. (I've separately updated the 'master' branch to be identical to today's cdh5-trunk branch; it had grown a few years stale.) I've also added support to teasing the configuration through test-with-docker. This allows for Impala 2.x and 3.x to diverge here, and it allows for testing changes to Impala-lzo. Change-Id: Ieba45fc18d9e490f75d16c477cdc1cce26f41ce9 Reviewed-on: http://gerrit.cloudera.org:8080/12259 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M bin/bootstrap_system.sh M docker/entrypoint.sh M docker/test-with-docker.py 3 files changed, 22 insertions(+), 2 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/12259 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ieba45fc18d9e490f75d16c477cdc1cce26f41ce9 Gerrit-Change-Number: 12259 Gerrit-PatchSet: 3 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8041, Part 2: Refactor SELECT list
Fredy Wijaya has posted comments on this change. (
http://gerrit.cloudera.org:8080/12144 )
Change subject: IMPALA-8041, Part 2: Refactor SELECT list
..
Patch Set 3: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12144/2/fe/src/main/java/org/apache/impala/analysis/SelectListItem.java
File fe/src/main/java/org/apache/impala/analysis/SelectListItem.java:
http://gerrit.cloudera.org:8080/#/c/12144/2/fe/src/main/java/org/apache/impala/analysis/SelectListItem.java@63
PS2, Line 63: public String getAlias() { return alias_; }
> Great question; one that touches on the key issue we're trying to address.
Thanks for the explanation.
--
To view, visit http://gerrit.cloudera.org:8080/12144
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5e173dad717e739c097f1f0cd86a0352648ff886
Gerrit-Change-Number: 12144
Gerrit-PatchSet: 3
Gerrit-Owner: Paul Rogers
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Paul Rogers
Gerrit-Comment-Date: Wed, 23 Jan 2019 23:22:51 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] Using 'master' branch of Impala-lzo and allowing test-with-docker to configure it.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12259 ) Change subject: Using 'master' branch of Impala-lzo and allowing test-with-docker to configure it. .. Patch Set 2: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/12259 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ieba45fc18d9e490f75d16c477cdc1cce26f41ce9 Gerrit-Change-Number: 12259 Gerrit-PatchSet: 2 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 23 Jan 2019 23:28:09 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7832: Support for IF NOT EXISTS in ALTER TABLE ADD COLUMN(S)
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12181 ) Change subject: IMPALA-7832: Support for IF NOT EXISTS in ALTER TABLE ADD COLUMN(S) .. Patch Set 9: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3668/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/12181 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I60ed22c8a8eefa10e94ad3dedf32fe67c16642d9 Gerrit-Change-Number: 12181 Gerrit-PatchSet: 9 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Wed, 23 Jan 2019 23:24:05 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7640: RENAME on managed Kudu table should rename Kudu table
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12179 ) Change subject: IMPALA-7640: RENAME on managed Kudu table should rename Kudu table .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1870/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12179 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I77e7583ce93cba8f6e743c4bedd9900ae1fae081 Gerrit-Change-Number: 12179 Gerrit-PatchSet: 2 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Sahil Takiar Gerrit-Comment-Date: Wed, 23 Jan 2019 23:24:18 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7832: Support for IF NOT EXISTS in ALTER TABLE ADD COLUMN(S)
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12181 ) Change subject: IMPALA-7832: Support for IF NOT EXISTS in ALTER TABLE ADD COLUMN(S) .. Patch Set 8: Code-Review+2 Promoting it to +2 after +1 from Bharath and +1 from Paul. -- To view, visit http://gerrit.cloudera.org:8080/12181 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I60ed22c8a8eefa10e94ad3dedf32fe67c16642d9 Gerrit-Change-Number: 12181 Gerrit-PatchSet: 8 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Wed, 23 Jan 2019 23:23:44 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7832: Support for IF NOT EXISTS in ALTER TABLE ADD COLUMN(S)
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12181 ) Change subject: IMPALA-7832: Support for IF NOT EXISTS in ALTER TABLE ADD COLUMN(S) .. Patch Set 9: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12181 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I60ed22c8a8eefa10e94ad3dedf32fe67c16642d9 Gerrit-Change-Number: 12181 Gerrit-PatchSet: 9 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Wed, 23 Jan 2019 23:24:04 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8058: Fallback for HBase key scan range estimation
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12192 ) Change subject: IMPALA-8058: Fallback for HBase key scan range estimation .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1869/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic01147abcb6b184071ba28b55aedc3bc49b322ce Gerrit-Change-Number: 12192 Gerrit-PatchSet: 3 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Zoram Thanga Gerrit-Comment-Date: Wed, 23 Jan 2019 23:21:09 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8041, Part 2: Refactor SELECT list
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12144 ) Change subject: IMPALA-8041, Part 2: Refactor SELECT list .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1868/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12144 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5e173dad717e739c097f1f0cd86a0352648ff886 Gerrit-Change-Number: 12144 Gerrit-PatchSet: 3 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Wed, 23 Jan 2019 23:11:19 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7640: RENAME on managed Kudu table should rename Kudu table
Sahil Takiar has posted comments on this change. (
http://gerrit.cloudera.org:8080/12179 )
Change subject: IMPALA-7640: RENAME on managed Kudu table should rename Kudu
table
..
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/12179/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/12179/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2378
PS1, Line 2378: if (oldTbl instanceof KuduTable &&
!Table.isExternalTable(msTbl)) {
> Can you add a precondition check for (KuduTable.isKuduTable(msTbl))?
Done
http://gerrit.cloudera.org:8080/#/c/12179/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2586
PS1, Line 2586: if (KuduTable.isKuduTable(msTbl)) {
> Can this code now use renameKuduTable()?
I considered that, but doing so would essentially make renameKuduTable() a
wrapper around KuduCatalogOpExecutor.renameTable()
The way KuduCatalogOpExecutor.renameTable is executed here vs. in
renameKuduTable() is different enough that making them use a common method
doesn't help much. The differences are that (1) we use
properties.get(KuduTable.KEY_TABLE_NAME) to get the table name vs
KuduUtil.getDefaultCreateKuduTableName, (2) all the altered properties have to
be added to the msTbl before KuduCatalogOpExecutor.validateKuduTblExists is
invoked - it seems that the call to validateKuduTblExists validates the table
is accessible *and* that the new properties are valid.
http://gerrit.cloudera.org:8080/#/c/12179/1/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test
File testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test:
http://gerrit.cloudera.org:8080/#/c/12179/1/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test@370
PS1, Line 370: alter table tbl_to_alter rename to kudu_tbl_to_alter
> Can you add a test that makes sure that the underlying table got renamed su
This query + the "create external table external_tbl stored as kudu..." one
below already cover this. Together they validate the the table was renamed. I
added another test to make sure that the original table does not exist.
--
To view, visit http://gerrit.cloudera.org:8080/12179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I77e7583ce93cba8f6e743c4bedd9900ae1fae081
Gerrit-Change-Number: 12179
Gerrit-PatchSet: 1
Gerrit-Owner: Sahil Takiar
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Sahil Takiar
Gerrit-Comment-Date: Wed, 23 Jan 2019 23:07:23 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8095: Detailed expression cardinality tests
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12248 ) Change subject: IMPALA-8095: Detailed expression cardinality tests .. Patch Set 4: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1867/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12248 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3da58ee9b0beebeffb170b9430bd36d20dcd2401 Gerrit-Change-Number: 12248 Gerrit-PatchSet: 4 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Wed, 23 Jan 2019 23:06:52 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7640: RENAME on managed Kudu table should rename Kudu table
Hello Lars Volker, Mike Percy, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12179 to look at the new patch set (#2). Change subject: IMPALA-7640: RENAME on managed Kudu table should rename Kudu table .. IMPALA-7640: RENAME on managed Kudu table should rename Kudu table `ALTER TABLE managed_kudu_tbl RENAME TO new_managed_kudu_tbl` will now rename the underlying Kudu table from `managed_kudu_tbl` to `new_managed_kudu_tbl`. This is only triggered for managed tables, for external tables renames do not modify the underlying Kudu table. Testing: * Ran core tests * Updated kudu_alter.test Change-Id: I77e7583ce93cba8f6e743c4bedd9900ae1fae081 --- M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test 2 files changed, 53 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/12179/2 -- To view, visit http://gerrit.cloudera.org:8080/12179 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I77e7583ce93cba8f6e743c4bedd9900ae1fae081 Gerrit-Change-Number: 12179 Gerrit-PatchSet: 2 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Mike Percy
[Impala-ASF-CR] IMPALA-8058: Fallback for HBase key scan range estimation
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12192 ) Change subject: IMPALA-8058: Fallback for HBase key scan range estimation .. Patch Set 2: (2 comments) Hi Bharath, Thanks much for the review. Addressed your two review comments. http://gerrit.cloudera.org:8080/#/c/12192/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12192/2//COMMIT_MSG@22 PS2, Line 22: exploints > typo Done http://gerrit.cloudera.org:8080/#/c/12192/2/fe/src/main/java/org/apache/impala/catalog/FeHBaseTable.java File fe/src/main/java/org/apache/impala/catalog/FeHBaseTable.java: http://gerrit.cloudera.org:8080/#/c/12192/2/fe/src/main/java/org/apache/impala/catalog/FeHBaseTable.java@412 PS2, Line 412: getTableName().toSql(), > you can use tbl.getFullName() Done -- To view, visit http://gerrit.cloudera.org:8080/12192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic01147abcb6b184071ba28b55aedc3bc49b322ce Gerrit-Change-Number: 12192 Gerrit-PatchSet: 2 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Zoram Thanga Gerrit-Comment-Date: Wed, 23 Jan 2019 22:47:24 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8058: Fallback for HBase key scan range estimation
Hello Bharath Vissapragada, Zoram Thanga, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12192 to look at the new patch set (#3). Change subject: IMPALA-8058: Fallback for HBase key scan range estimation .. IMPALA-8058: Fallback for HBase key scan range estimation Impala supports "pushing" of HBase key range predicates to HBase so that Impala reads only rows within the target key range. The planner estimates the cardinality of such scans by sampling the rows within the range. However, we have seen cases where sampling returns rows for unknown reasons. The planner then ends up without a good cardinality estimate. (Specifically, the code does a division by zero and produces a huge estimate. See the ticket for details.) Impala appears to use the sampling strategy to compute cardinality because HBase uses generally do not gather table stats. The resulting estimates are often off by 2x or more. This is a problem in tests as it causes cardinality numbers to vary greatly from the expected values. Fortunately, tests do gather HMS stats. There may be cases where users do as well. This fix exploits that fact. This fix: * Creates a fall-back strategy that uses table cardinality from HMS and the selectivity of the key predicates to estimate cardinality when the sampling approach fails. * The fall-back strategy requires tracking the predicates used for HBase keys so that their selectivity can be applied during fall-back calculations. * Moved HBase key calculation out of the SingleNodePlanner into the HBase scan node as suggested by a "TO DO" in the code. Doing so simplified the new code. * In the spirit of IMPALA-7919, adds the key predicates to the HBase scan node in the EXPLAIN output. Testing: * Adds a query context option to disable the normal key sampling to force the use of the fall-back. Used for testing. * Adds a new set of HBase test cases that use the new feature to check plans with the fall-back approach. * Reran all existing tests. * Compared cardinality numbers for the two modes: sampling and HMS using the cardinality features of IMPALA-8021. The two approaches provide different results, but this is mostly due to the missing selectivity estimates for inequality operators. (That's a fix for another time.) Change-Id: Ic01147abcb6b184071ba28b55aedc3bc49b322ce --- M common/thrift/ImpalaInternalService.thrift M fe/src/main/java/org/apache/impala/catalog/FeHBaseTable.java M fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java M fe/src/test/java/org/apache/impala/planner/PlannerTest.java M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java M testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test A testdata/workloads/functional-planner/queries/PlannerTest/hbase-no-key-est.test M testdata/workloads/functional-planner/queries/PlannerTest/hbase.test M testdata/workloads/functional-planner/queries/PlannerTest/joins.test 10 files changed, 511 insertions(+), 116 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/12192/3 -- To view, visit http://gerrit.cloudera.org:8080/12192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic01147abcb6b184071ba28b55aedc3bc49b322ce Gerrit-Change-Number: 12192 Gerrit-PatchSet: 3 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Zoram Thanga
[Impala-ASF-CR] IMPALA-7832: Support for IF NOT EXISTS in ALTER TABLE ADD COLUMN(S)
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12181 ) Change subject: IMPALA-7832: Support for IF NOT EXISTS in ALTER TABLE ADD COLUMN(S) .. Patch Set 8: Code-Review+1 LGTM -- To view, visit http://gerrit.cloudera.org:8080/12181 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I60ed22c8a8eefa10e94ad3dedf32fe67c16642d9 Gerrit-Change-Number: 12181 Gerrit-PatchSet: 8 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Wed, 23 Jan 2019 22:40:46 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8041, Part 2: Refactor SELECT list
Paul Rogers has posted comments on this change. (
http://gerrit.cloudera.org:8080/12144 )
Change subject: IMPALA-8041, Part 2: Refactor SELECT list
..
Patch Set 2:
(6 comments)
Thanks Fredy for the review. Addressed your comments. Had a question about two
of them.
http://gerrit.cloudera.org:8080/#/c/12144/2/fe/src/main/java/org/apache/impala/analysis/AbstractExpression.java
File fe/src/main/java/org/apache/impala/analysis/AbstractExpression.java:
http://gerrit.cloudera.org:8080/#/c/12144/2/fe/src/main/java/org/apache/impala/analysis/AbstractExpression.java@23
PS2, Line 23: Represents an expression a while
> confusing comment
Done
http://gerrit.cloudera.org:8080/#/c/12144/2/fe/src/main/java/org/apache/impala/analysis/SelectListItem.java
File fe/src/main/java/org/apache/impala/analysis/SelectListItem.java:
http://gerrit.cloudera.org:8080/#/c/12144/2/fe/src/main/java/org/apache/impala/analysis/SelectListItem.java@28
PS2, Line 28: import com.google.common.base.Predicates;
> nit: empty line after import
Done
http://gerrit.cloudera.org:8080/#/c/12144/2/fe/src/main/java/org/apache/impala/analysis/SelectListItem.java@40
PS2, Line 40: alias_
> can be final
Done
http://gerrit.cloudera.org:8080/#/c/12144/2/fe/src/main/java/org/apache/impala/analysis/SelectListItem.java@63
PS2, Line 63: public void setExpr(Expr expr) { expr_ = expr; }
> since we already have clone() method, is it necessary to make expr_ mutable
Great question; one that touches on the key issue we're trying to address.
The select list is a list of (abstract) expressions. Once we form the list (in
this new version), the abstract expressions within the list will never change.
This is different than the current master version in which the list of
expressions do change.
The analyzer does rewrites. During that time, in the base version, the analyzer
replaces one select list item with the new, rewritten expression. In this
version, the abstract expression is unchanged. However, the Expr node does
change after rewrite, and that new version is set user using the setExpr()
method.
In the old version, we had ambiguity about whether the select list contained
the original, unwritten expressions or the post-analysis, rewritten versions.
(And we had code to try to reset them, which didn't quite work.)
In this version, this class holds onto both versions making it clear which is
the user's "source" expression and which is rewritten.
All of this is moving toward integrating rewrites into the analyzer rather than
as a separate bolt-on step.
http://gerrit.cloudera.org:8080/#/c/12144/2/fe/src/main/java/org/apache/impala/analysis/SelectListItem.java@220
PS2, Line 220: return (SelectWildcard) item;
> Add Preconditions.checkArgument(item instanceof SelectWildcard)
Certainly could do so. But all that would do is the very same check that the
cast does. Since any error thrown from the cast would clearly indicate the
issue, the additional check didn't seem to add much additional checking or
information. What do you think?
http://gerrit.cloudera.org:8080/#/c/12144/2/fe/src/main/java/org/apache/impala/analysis/SelectListItem.java@223
PS2, Line 223: return (SelectExpr) item;
> Add Preconditions.checkArgument(item instanceof SelectExpr)
Seme comment/question as above.
--
To view, visit http://gerrit.cloudera.org:8080/12144
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5e173dad717e739c097f1f0cd86a0352648ff886
Gerrit-Change-Number: 12144
Gerrit-PatchSet: 2
Gerrit-Owner: Paul Rogers
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Paul Rogers
Gerrit-Comment-Date: Wed, 23 Jan 2019 22:39:12 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8041, Part 2: Refactor SELECT list
Hello Bharath Vissapragada, Fredy Wijaya, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12144 to look at the new patch set (#3). Change subject: IMPALA-8041, Part 2: Refactor SELECT list .. IMPALA-8041, Part 2: Refactor SELECT list Another step toward combining analysis and rewrites. This step refactors the SELECT list to: * Enable each select list item to hold its "source" (before analysis and rewrite) SQL. * Split the select list item into two classes: one for the wildcard (which holds no expresion) and the other for actual select expressions. * Moves some analysis steps into the select list item itself. * Cleans up a few other minor items. Tests: No functional change, just refactoring. Reran all FE tests. Change-Id: I5e173dad717e739c097f1f0cd86a0352648ff886 --- M fe/src/main/cup/sql-parser.cup A fe/src/main/java/org/apache/impala/analysis/AbstractExpression.java M fe/src/main/java/org/apache/impala/analysis/Expr.java M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java M fe/src/main/java/org/apache/impala/analysis/SelectList.java M fe/src/main/java/org/apache/impala/analysis/SelectListItem.java M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java M fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java 12 files changed, 302 insertions(+), 132 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/44/12144/3 -- To view, visit http://gerrit.cloudera.org:8080/12144 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5e173dad717e739c097f1f0cd86a0352648ff886 Gerrit-Change-Number: 12144 Gerrit-PatchSet: 3 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers
[Impala-ASF-CR] IMPALA-8095: Detailed expression cardinality tests
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12248 ) Change subject: IMPALA-8095: Detailed expression cardinality tests .. Patch Set 4: Fixed check style issues. -- To view, visit http://gerrit.cloudera.org:8080/12248 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3da58ee9b0beebeffb170b9430bd36d20dcd2401 Gerrit-Change-Number: 12248 Gerrit-PatchSet: 4 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Wed, 23 Jan 2019 22:24:41 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8095: Detailed expression cardinality tests
Hello Bharath Vissapragada, Fredy Wijaya, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12248 to look at the new patch set (#4). Change subject: IMPALA-8095: Detailed expression cardinality tests .. IMPALA-8095: Detailed expression cardinality tests Cardinality is a critical input to the query planning process, especially join planning. Impala has many high-level end-to-end tests that implicitly test cardinality at the "wholesale" level: A test will produce a wrong result if the cardinality is badly wrong. Until this patch, no detailed unit tests exist for this topic. Tests here include: * Table cardinality, NDV values and null count in metadata retrieved from HMS. * Table cardinality, NDV values and null counts in metadata presented to the query. * Expression NDV and selectivity values (which derive from table cardinality and column NDV.) The test illustrate a number of bugs. This patch simply identifies the bugs, comments out the tests that fail because of the bugs, and substitutes tests that pass with the current, incorrect, behavior. Future patches will fix the bugs. Reviewers can note the difference between the original, incorrect behavior shown here, and the revised behavior in those additional patches. Since none of the existing "functional" tables provide the level of detail needed for these tests, added a new test table specifically for this task. This set of tests was a good time to extend the test "fixture" framework created earlier. The FrontendTestBase class was refactored to use a new FrontendFixture which represents a (simulated) Impala and HMS cluster. The previoius SessionFixture represents a single user session (with session options) and the QueryFixture represents a single query. As part of this refactoring, the fixture classes moved into "common" alongside FrontendTestBase. Testing: This patch includes only tests: no "production" code was changed. Change-Id: I3da58ee9b0beebeffb170b9430bd36d20dcd2401 --- M fe/src/main/java/org/apache/impala/analysis/Expr.java M fe/src/main/java/org/apache/impala/analysis/SelectListItem.java M fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java D fe/src/test/java/org/apache/impala/analysis/AnalysisSessionFixture.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java A fe/src/test/java/org/apache/impala/analysis/ExprCardinalityTest.java M fe/src/test/java/org/apache/impala/analysis/ExprNdvTest.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java A fe/src/test/java/org/apache/impala/common/AbstractFrontendTest.java A fe/src/test/java/org/apache/impala/common/AnalysisSessionFixture.java A fe/src/test/java/org/apache/impala/common/FrontendFixture.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java A fe/src/test/java/org/apache/impala/common/QueryFixture.java M fe/src/test/java/org/apache/impala/planner/CardinalityTest.java A testdata/NullRows/data.csv M testdata/bin/compute-table-stats.sh M testdata/datasets/functional/functional_schema_template.sql M testdata/datasets/functional/schema_constraints.csv 18 files changed, 1,688 insertions(+), 535 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/12248/4 -- To view, visit http://gerrit.cloudera.org:8080/12248 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3da58ee9b0beebeffb170b9430bd36d20dcd2401 Gerrit-Change-Number: 12248 Gerrit-PatchSet: 4 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers
[Impala-ASF-CR] IMPALA-7929. Impala query on HBASE table failing with InternalException.
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12213 ) Change subject: IMPALA-7929. Impala query on HBASE table failing with InternalException. .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/12213/3/fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java File fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java: http://gerrit.cloudera.org:8080/#/c/12213/3/fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java@285 PS3, Line 285: Thus qualifier is changed from Required to Optional in thrift, : // and the code here is changed accordingly to use the new constructor that : // doesn't require qualifier. > A couple notes on this comment: Well said. -- To view, visit http://gerrit.cloudera.org:8080/12213 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I378c2249604481067b5b1c3a3bbb28c30ad4d751 Gerrit-Change-Number: 12213 Gerrit-PatchSet: 3 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Yongjun Zhang Gerrit-Comment-Date: Wed, 23 Jan 2019 22:15:54 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5872: Testcase builder for query planner
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12221 ) Change subject: IMPALA-5872: Testcase builder for query planner .. Patch Set 2: (3 comments) Minor suggestions, otherwise looks pretty good. Since this code won't affect production use cases, my thought is that we should merge it early so we can all play with and extend it. I'm anxious to convert my cobbled-together text metadata parser to use your load framework, which will be easier to do if the code is in master. http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java File fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java: http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java@129 PS2, Line 129: queryStmt_.collectTableRefs(allReferencedTables); Will this filter out duplicate table references: SELECT ... FROM foo a, foo b? Should this collection be a Set or Map instead? http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java@164 PS2, Line 164: result.setDbs(new ArrayList<>(uniqueDbs.values())); The values of a map or set are in non-deterministic order. Will it help tests, etc. to sort these entries by name (say) so that the same operation will product the same result? http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/common/JniUtil.java File fe/src/main/java/org/apache/impala/common/JniUtil.java: http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/common/JniUtil.java@120 PS2, Line 120: return serializer.toString(input); If this is for the test case, the resulting string could be very large. Should this take a Writer instead? If you do want a string, you can pass a StringWriter. -- To view, visit http://gerrit.cloudera.org:8080/12221 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iec83eeb2dc5136768b70ed581fb8d3ed0335cb52 Gerrit-Change-Number: 12221 Gerrit-PatchSet: 2 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Balazs Jeszenszky Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Wed, 23 Jan 2019 22:14:08 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7841 (Part 1): Refactor SelectStmt for easier debugging
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11915 ) Change subject: IMPALA-7841 (Part 1): Refactor SelectStmt for easier debugging .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1866/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/11915 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4f4fe3d3a1ab3170e294714dab066d40d169eff1 Gerrit-Change-Number: 11915 Gerrit-PatchSet: 3 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Wed, 23 Jan 2019 21:56:10 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7929. Impala query on HBASE table failing with InternalException.
Joe McDonnell has posted comments on this change. (
http://gerrit.cloudera.org:8080/12213 )
Change subject: IMPALA-7929. Impala query on HBASE table failing with
InternalException.
..
Patch Set 3:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/12213/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12213/3//COMMIT_MSG@7
PS3, Line 7: IMPALA-7929.
We always use ":" after the JIRA in our commit titles. i.e.
IMPALA-7929: blah
http://gerrit.cloudera.org:8080/#/c/12213/3//COMMIT_MSG@7
PS3, Line 7: Impala query on HBASE table failing with InternalException.
Minor comment about titles:
Usually describing what you are changing is more useful down the road than
describing the symptom of the JIRA. There are often multiple JIRAs that have
similar symptoms but different fixes. There are also times when we merge
multiple things for one JIRA, and keeping those separate is useful. It is worth
reading through some commits on "git log" to get a sense of the varieties of
titles that people use.
http://gerrit.cloudera.org:8080/#/c/12213/3//COMMIT_MSG@9
PS3, Line 9: Impala query failed with "IntenalException: Required field
'qualifier' was not present!"
: on table created via hive and mapped to hbase, because the
qualifier of the hbase key
: key column is null in the mapped table, and Impala requires
non-null qualifier. The fix
: here relaxes this requirement.
Please wrap commit messages at about 70-75 characters. "git log" adds about 4
characters before a message body, so it is nice if this fits in a standard 80
character terminal. Take a look at "git log" and follow what other developers
are doing. (An exception to this is when there are long names or formatted
lines like stack traces.)
Separately, a couple notes:
I would like the comment to be more specific. What is changing and why does it
fix the issue? If someone looked up IMPALA-7929, would they be able to figure
out what was wrong and how it was fixed? To me, "relaxes this requirement"
doesn't tell me what specifically changed.
http://gerrit.cloudera.org:8080/#/c/12213/3/common/thrift/PlanNodes.thrift
File common/thrift/PlanNodes.thrift:
http://gerrit.cloudera.org:8080/#/c/12213/3/common/thrift/PlanNodes.thrift@287
PS3, Line 287: 2: optional string qualifier
Please add a comment describing why this is optional.
http://gerrit.cloudera.org:8080/#/c/12213/3/fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java
File fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java:
http://gerrit.cloudera.org:8080/#/c/12213/3/fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java@285
PS3, Line 285: Thus qualifier is changed from Required to Optional in thrift,
: // and the code here is changed accordingly to use the
new constructor that
: // doesn't require qualifier.
A couple notes on this comment:
1. To me, when I read "is changed from X to Y", that tense only really makes
sense in the context of this code change / code review. Once this is merged, it
doesn't make sense because it is already done and completed. (This tense might
be permissible in a commit message, because it is describing what is changing
in this commit.)
Most code comments describe the existing code in the present tense. "X is
optional because blah." Sometimes comments describe what changed, and this uses
the past tense. "JIRA-Y changed this to do Z."
2. Comments exist to answer a question. If the code is very clear, developers
are not usually asking "What is the code doing?" They are more likely to ask
"Why is the code this way?" It is pretty clear that we are constructing the
THBaseFilter and setting the qualifier separately. The comment here should
answer the question "Why is the qualifier special and set separately?"
I would write this comment as:
IMPALA-7929: Since the qualifier can be null (e.g. for the key column of an
HBase table), the qualifier field must be optional in order to express the null
value. Constructors in Thrift do not set optional fields, so the qualifier must
be set separately.
http://gerrit.cloudera.org:8080/#/c/12213/3/tests/query_test/test_hbase_queries.py
File tests/query_test/test_hbase_queries.py:
http://gerrit.cloudera.org:8080/#/c/12213/3/tests/query_test/test_hbase_queries.py@94
PS3, Line 94: result=self.client.execute("select * from {0} where k !=
'row1'".format(table_name))
: assert(len(result.data) == 3)
: result=self.client.execute("select * from {0} where k =
'row1'".format(table_name))
: assert(len(result.data) == 1)
: result=self.client.execute("select * from {0} where c !=
'c2'".format(table_name))
: assert(len(result.data) == 2)
: result=self.client.execute("select * from {0} where c =
'c4'".format(table_name))
[Impala-ASF-CR] IMPALA-8041, Part 2: Refactor SELECT list
Fredy Wijaya has posted comments on this change. (
http://gerrit.cloudera.org:8080/12144 )
Change subject: IMPALA-8041, Part 2: Refactor SELECT list
..
Patch Set 2:
(6 comments)
Couple nits, but LGTM.
http://gerrit.cloudera.org:8080/#/c/12144/2/fe/src/main/java/org/apache/impala/analysis/AbstractExpression.java
File fe/src/main/java/org/apache/impala/analysis/AbstractExpression.java:
http://gerrit.cloudera.org:8080/#/c/12144/2/fe/src/main/java/org/apache/impala/analysis/AbstractExpression.java@23
PS2, Line 23: Represents an expression a while
confusing comment
http://gerrit.cloudera.org:8080/#/c/12144/2/fe/src/main/java/org/apache/impala/analysis/SelectListItem.java
File fe/src/main/java/org/apache/impala/analysis/SelectListItem.java:
http://gerrit.cloudera.org:8080/#/c/12144/2/fe/src/main/java/org/apache/impala/analysis/SelectListItem.java@28
PS2, Line 28: import com.google.common.base.Predicates;
nit: empty line after import
http://gerrit.cloudera.org:8080/#/c/12144/2/fe/src/main/java/org/apache/impala/analysis/SelectListItem.java@40
PS2, Line 40: alias_
can be final
http://gerrit.cloudera.org:8080/#/c/12144/2/fe/src/main/java/org/apache/impala/analysis/SelectListItem.java@63
PS2, Line 63: public void setExpr(Expr expr) { expr_ = expr; }
since we already have clone() method, is it necessary to make expr_ mutable?
http://gerrit.cloudera.org:8080/#/c/12144/2/fe/src/main/java/org/apache/impala/analysis/SelectListItem.java@220
PS2, Line 220: return (SelectWildcard) item;
Add Preconditions.checkArgument(item instanceof SelectWildcard)
http://gerrit.cloudera.org:8080/#/c/12144/2/fe/src/main/java/org/apache/impala/analysis/SelectListItem.java@223
PS2, Line 223: return (SelectExpr) item;
Add Preconditions.checkArgument(item instanceof SelectExpr)
--
To view, visit http://gerrit.cloudera.org:8080/12144
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5e173dad717e739c097f1f0cd86a0352648ff886
Gerrit-Change-Number: 12144
Gerrit-PatchSet: 2
Gerrit-Owner: Paul Rogers
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Paul Rogers
Gerrit-Comment-Date: Wed, 23 Jan 2019 21:38:13 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7832: Support for IF NOT EXISTS in ALTER TABLE ADD COLUMN(S)
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12181 ) Change subject: IMPALA-7832: Support for IF NOT EXISTS in ALTER TABLE ADD COLUMN(S) .. Patch Set 8: Paul, Bharath gave a +1, can you take another look at it? -- To view, visit http://gerrit.cloudera.org:8080/12181 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I60ed22c8a8eefa10e94ad3dedf32fe67c16642d9 Gerrit-Change-Number: 12181 Gerrit-PatchSet: 8 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Wed, 23 Jan 2019 21:39:37 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7866: Predicates, helpers for implicit casts, slot refs
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11953 ) Change subject: IMPALA-7866: Predicates, helpers for implicit casts, slot refs .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1863/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/11953 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ieaa0aee1b9015e0aed521f2038bf44513d7f8613 Gerrit-Change-Number: 11953 Gerrit-PatchSet: 3 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Wed, 23 Jan 2019 21:28:14 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8041, Part 1: Move rewrite rules into expr nodes
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12143 ) Change subject: IMPALA-8041, Part 1: Move rewrite rules into expr nodes .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1865/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifda7082abea4f6448e6cfef0d9ef7b8565d73fce Gerrit-Change-Number: 12143 Gerrit-PatchSet: 2 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Wed, 23 Jan 2019 21:38:19 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7934: Switch to java.util.Base64 implementation
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12250 ) Change subject: IMPALA-7934: Switch to java.util.Base64 implementation .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/12250/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12250/2//COMMIT_MSG@7 PS2, Line 7: IMPALA-7934: Switch to java.util.Base64 implementation I'm pretty sure it is compatible with commons' base64 but could you please double check manually (if you haven't already done so)? Basically by creating a table with incremental stats (or persistent functions) without the patch, then trying to read them after applying this patch. http://gerrit.cloudera.org:8080/#/c/12250/2//COMMIT_MSG@14 PS2, Line 14: Did you see any perf improvements in the loading time of large tables with incremental stats? -- To view, visit http://gerrit.cloudera.org:8080/12250 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2d43d4a4f073a800d963ce4c77f21c9efa8471ac Gerrit-Change-Number: 12250 Gerrit-PatchSet: 2 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 23 Jan 2019 21:21:54 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8095: Detailed expression cardinality tests
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12248 ) Change subject: IMPALA-8095: Detailed expression cardinality tests .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1864/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12248 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3da58ee9b0beebeffb170b9430bd36d20dcd2401 Gerrit-Change-Number: 12248 Gerrit-PatchSet: 3 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Wed, 23 Jan 2019 21:21:18 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7970 : Add support for metastore event based automatic invalidate
Paul Rogers has posted comments on this change. (
http://gerrit.cloudera.org:8080/12118 )
Change subject: IMPALA-7970 : Add support for metastore event based automatic
invalidate
..
Patch Set 22:
(3 comments)
Thanks much for your great work in getting this in and helping us anticipate
all the ways that things might go wrong in a production system. Thanks much
also for discussing the code review comments in person.
This review has just a few minor follow-on comments. After that, I suspect
we'll be in good shape to merge the code: we are reaching the point of
diminishing returns in reviews; the next step is to try the feature out in a
live cluster, and for that you'll need the code merged. A feature flag lets us
turn off the feature if we encounter anything missed during reviews.
http://gerrit.cloudera.org:8080/#/c/12118/22/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java:
http://gerrit.cloudera.org:8080/#/c/12118/22/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@360
PS22, Line 360: throw new MetastoreNotificationException(debugString(
To follow up on previous comments about errors. We may have the user busily
hammering the system with INVALIDATEs and queries, each of which may mutate the
cache ahead of the event processing here. If we encounter such a case, we
should soldier on rather than stopping event processing.
So, the case that the old table does not exist, is that an expected error if
the user issues an INVALIDATE? Or, does it mean that we have a likely bug and
we should stop event processing?
The case of the missing new table is different: it would seem that there would
be no good reason not to add the new table.
But, I wonder, does the renameTable() method handle the case in which the
rename was already done? Something like:
* Rename table in HMS.
* Issue a query against the new name, causing the catalog to load metadata for
the new table.
* Get the rename event.
* Find the old table and remove it, but be unable to add a new table since a
table of that name was already loaded.
http://gerrit.cloudera.org:8080/#/c/12118/22/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@515
PS22, Line 515: LOG.info(debugString("Successfully removed database
%s", dbName_));
Briefly looked at these other error handling blocks. The exception/logging
decisions look right on each of them.
http://gerrit.cloudera.org:8080/#/c/12118/22/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
File
fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java:
http://gerrit.cloudera.org:8080/#/c/12118/22/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@258
PS22, Line 258: public synchronized void start(long fromEventId) {
This is synchronized, which suggests that we might get multiple concurrent
start/stop calls from different threads.
Should the start() (and stop()) methods check if they are in the proper state
before proceeding? That is, should this method check if we are already in the
ACTIVE state and simply return if so?
--
To view, visit http://gerrit.cloudera.org:8080/12118
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic70b27780560b7ac9b33418d132b36cd0ca4abf7
Gerrit-Change-Number: 12118
Gerrit-PatchSet: 22
Gerrit-Owner: Vihang Karajgaonkar
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Paul Rogers
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Vihang Karajgaonkar
Gerrit-Comment-Date: Wed, 23 Jan 2019 21:18:50 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5051: Add INT64 timestamp write support in Parquet
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12247 ) Change subject: IMPALA-5051: Add INT64 timestamp write support in Parquet .. Patch Set 5: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1862/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12247 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib41ad532ec902ed5a9a1528513726eac1c11441f Gerrit-Change-Number: 12247 Gerrit-PatchSet: 5 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Reviewer: Zoltan Ivanfi Gerrit-Comment-Date: Wed, 23 Jan 2019 21:18:46 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8058: Fallback for HBase key scan range estimation
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12192 ) Change subject: IMPALA-8058: Fallback for HBase key scan range estimation .. Patch Set 2: Code-Review+2 (2 comments) http://gerrit.cloudera.org:8080/#/c/12192/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12192/2//COMMIT_MSG@22 PS2, Line 22: exploints typo http://gerrit.cloudera.org:8080/#/c/12192/2/fe/src/main/java/org/apache/impala/catalog/FeHBaseTable.java File fe/src/main/java/org/apache/impala/catalog/FeHBaseTable.java: http://gerrit.cloudera.org:8080/#/c/12192/2/fe/src/main/java/org/apache/impala/catalog/FeHBaseTable.java@412 PS2, Line 412: getTableName().toSql(), you can use tbl.getFullName() -- To view, visit http://gerrit.cloudera.org:8080/12192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic01147abcb6b184071ba28b55aedc3bc49b322ce Gerrit-Change-Number: 12192 Gerrit-PatchSet: 2 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Zoram Thanga Gerrit-Comment-Date: Wed, 23 Jan 2019 21:12:46 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7841 (Part 1): Refactor SelectStmt for easier debugging
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/11915 ) Change subject: IMPALA-7841 (Part 1): Refactor SelectStmt for easier debugging .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11915 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4f4fe3d3a1ab3170e294714dab066d40d169eff1 Gerrit-Change-Number: 11915 Gerrit-PatchSet: 3 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Wed, 23 Jan 2019 21:03:04 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7905: Hive keywords not quoted for identifiers
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12009 ) Change subject: IMPALA-7905: Hive keywords not quoted for identifiers .. Patch Set 13: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3667/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/12009 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I06cc20b052a3a66535a171c36b4b31477c0ba6d0 Gerrit-Change-Number: 12009 Gerrit-PatchSet: 13 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Wed, 23 Jan 2019 21:05:01 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7905: Hive keywords not quoted for identifiers
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12009 ) Change subject: IMPALA-7905: Hive keywords not quoted for identifiers .. Patch Set 13: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12009 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I06cc20b052a3a66535a171c36b4b31477c0ba6d0 Gerrit-Change-Number: 12009 Gerrit-PatchSet: 13 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Wed, 23 Jan 2019 21:05:00 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7905: Hive keywords not quoted for identifiers
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12009 ) Change subject: IMPALA-7905: Hive keywords not quoted for identifiers .. Patch Set 12: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12009 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I06cc20b052a3a66535a171c36b4b31477c0ba6d0 Gerrit-Change-Number: 12009 Gerrit-PatchSet: 12 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Wed, 23 Jan 2019 21:04:51 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7927: Enhance Rewritten SQL in test files
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12033 ) Change subject: IMPALA-7927: Enhance Rewritten SQL in test files .. Patch Set 1: Hi Bharath, this one should be a simple one; please take a look when you get a chance. Thanks! -- To view, visit http://gerrit.cloudera.org:8080/12033 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I09ee971a7797c4154bafee5c080ac3c6a1057fc7 Gerrit-Change-Number: 12033 Gerrit-PatchSet: 1 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Wed, 23 Jan 2019 21:04:22 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7841 (Part 1): Refactor SelectStmt for easier debugging
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11915 ) Change subject: IMPALA-7841 (Part 1): Refactor SelectStmt for easier debugging .. Patch Set 4: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3666/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/11915 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4f4fe3d3a1ab3170e294714dab066d40d169eff1 Gerrit-Change-Number: 11915 Gerrit-PatchSet: 4 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Wed, 23 Jan 2019 21:03:17 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12079 ) Change subject: IMPALA-7968, Part 1: JSON serialization framework .. Patch Set 8: Hi Bharath, Fredy gave this a +1. Can you take a quick look and give it the final +2 if it looks good to you? -- To view, visit http://gerrit.cloudera.org:8080/12079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c Gerrit-Change-Number: 12079 Gerrit-PatchSet: 8 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Wed, 23 Jan 2019 21:03:24 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7841 (Part 1): Refactor SelectStmt for easier debugging
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11915 ) Change subject: IMPALA-7841 (Part 1): Refactor SelectStmt for easier debugging .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11915 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4f4fe3d3a1ab3170e294714dab066d40d169eff1 Gerrit-Change-Number: 11915 Gerrit-PatchSet: 4 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Wed, 23 Jan 2019 21:03:16 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7841 (Part 1): Refactor SelectStmt for easier debugging
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/11915 ) Change subject: IMPALA-7841 (Part 1): Refactor SelectStmt for easier debugging .. Patch Set 3: (1 comment) Addressed review comment. Rebased on latest master. http://gerrit.cloudera.org:8080/#/c/11915/2/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java File fe/src/main/java/org/apache/impala/analysis/SelectStmt.java: http://gerrit.cloudera.org:8080/#/c/11915/2/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@923 PS2, Line 923: } > nit: newline. Done -- To view, visit http://gerrit.cloudera.org:8080/11915 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4f4fe3d3a1ab3170e294714dab066d40d169eff1 Gerrit-Change-Number: 11915 Gerrit-PatchSet: 3 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Wed, 23 Jan 2019 21:02:11 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7905: Hive keywords not quoted for identifiers
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12009 ) Change subject: IMPALA-7905: Hive keywords not quoted for identifiers .. Patch Set 12: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1861/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12009 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I06cc20b052a3a66535a171c36b4b31477c0ba6d0 Gerrit-Change-Number: 12009 Gerrit-PatchSet: 12 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Wed, 23 Jan 2019 21:02:29 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7841 (Part 1): Refactor SelectStmt for easier debugging
Hello Bharath Vissapragada, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11915 to look at the new patch set (#3). Change subject: IMPALA-7841 (Part 1): Refactor SelectStmt for easier debugging .. IMPALA-7841 (Part 1): Refactor SelectStmt for easier debugging Builds on IMPALA-7808 with several additional refactorings: * IMPALA-7808 minimized code changes. This change cleans up the new functions, removing if's and merging the "aggregation" function with the body of the new analyze() function. * Removed an unneeded analyzer argument. This is all refactoring: there is no functional change. Testing: Reran existing tests to ensure that functionality remained unchanged. Change-Id: I4f4fe3d3a1ab3170e294714dab066d40d169eff1 --- M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/analysis/CollectionTableRef.java M fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java M fe/src/main/java/org/apache/impala/analysis/QueryStmt.java M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java M fe/src/main/java/org/apache/impala/analysis/Subquery.java M fe/src/main/java/org/apache/impala/analysis/WithClause.java 7 files changed, 93 insertions(+), 99 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/15/11915/3 -- To view, visit http://gerrit.cloudera.org:8080/11915 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4f4fe3d3a1ab3170e294714dab066d40d169eff1 Gerrit-Change-Number: 11915 Gerrit-PatchSet: 3 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers
[Impala-ASF-CR] IMPALA-8041, Part 1: Move rewrite rules into expr nodes
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12143 ) Change subject: IMPALA-8041, Part 1: Move rewrite rules into expr nodes .. Patch Set 2: Rebased on latest master. Resolved check style issues. -- To view, visit http://gerrit.cloudera.org:8080/12143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifda7082abea4f6448e6cfef0d9ef7b8565d73fce Gerrit-Change-Number: 12143 Gerrit-PatchSet: 2 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Wed, 23 Jan 2019 20:59:31 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8041, Part 1: Move rewrite rules into expr nodes
Hello Bharath Vissapragada, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12143 to look at the new patch set (#2). Change subject: IMPALA-8041, Part 1: Move rewrite rules into expr nodes .. IMPALA-8041, Part 1: Move rewrite rules into expr nodes This patch is part of the task to integrate expression rewrites into the analysis process to avoid the need for two analysis passes. The analyzer provides a rewrite engine which traverses the expression tree and compares each node against a list of rewrite rules, typically by performing an "instanceof" comparison of the node with the target node type. This project seeks to apply the rules as we analyze each node. The simplest way to do so is for each node to hold the code for its own rewrites, callable via a virtual method: rewrite(). This patch is a first step: the rewrite code moves from the various rewrite rules into the expression node that is to be rewritten. In some cases nodes have a single rule which is moved from the rewrite rule directly into the rewrite() method. In other cases, a node has multiple rules. The different rules for the same node are moved into distinct methods. Though these methods are all called by the main rewrite() method, nothing calls that rewrite() method in this patch. Finally, this patch introduces a "stub" expression analyzer. In a later patch this class will drive the combined analysis/rewrite logic. For now, it is mostly a placeholder. The rewrite rules themselves are now just stubs retained to minimze change. A future patch will retire them once the analysis/rewrite integration is complete. This patch is designed to have no functional change: code merely moves from one location (the rewrite rules) to another (the expression nodes). Some "shim" code is added, but the functionality is unchanged. Tests: reran all FE tests to verify no change in behavior. Change-Id: Ifda7082abea4f6448e6cfef0d9ef7b8565d73fce --- M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java M fe/src/main/java/org/apache/impala/analysis/Expr.java A fe/src/main/java/org/apache/impala/analysis/ExprAnalyzer.java M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java M fe/src/main/java/org/apache/impala/common/TreeNode.java M fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java M fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java M fe/src/main/java/org/apache/impala/rewrite/ExprRewriter.java M fe/src/main/java/org/apache/impala/rewrite/ExtractCommonConjunctRule.java M fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java M fe/src/main/java/org/apache/impala/rewrite/NormalizeCountStarRule.java M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java M fe/src/main/java/org/apache/impala/rewrite/SimplifyDistinctFromRule.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java 18 files changed, 686 insertions(+), 378 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/12143/2 -- To view, visit http://gerrit.cloudera.org:8080/12143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ifda7082abea4f6448e6cfef0d9ef7b8565d73fce Gerrit-Change-Number: 12143 Gerrit-PatchSet: 2 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers
[Impala-ASF-CR] IMPALA-8095: Detailed expression cardinality tests
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12248 ) Change subject: IMPALA-8095: Detailed expression cardinality tests .. Patch Set 2: Rebased on latest master. A catalog_.close() line was added to the CleanUp method in FrontendTestBase. Moved this line to CleanUp in the new FrontendFixture class. -- To view, visit http://gerrit.cloudera.org:8080/12248 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3da58ee9b0beebeffb170b9430bd36d20dcd2401 Gerrit-Change-Number: 12248 Gerrit-PatchSet: 2 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Wed, 23 Jan 2019 20:49:05 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7800: Reject new connections after --fe service threads
Zoram Thanga has posted comments on this change. ( http://gerrit.cloudera.org:8080/12226 ) Change subject: IMPALA-7800: Reject new connections after --fe_service_threads .. Patch Set 6: > I meant "the timeout can also be turned off by setting it to 0 or > something by user". We will have a non-zero timeout by default. Given the feedback so far, I wonder if we need to go back to the drawing board and re-think the approach? -- To view, visit http://gerrit.cloudera.org:8080/12226 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4b9d48aaff9e3b5854b5121798211c641c58a559 Gerrit-Change-Number: 12226 Gerrit-PatchSet: 6 Gerrit-Owner: Zoram Thanga Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Zoram Thanga Gerrit-Comment-Date: Wed, 23 Jan 2019 20:54:53 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7866: Predicates, helpers for implicit casts, slot refs
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11953 ) Change subject: IMPALA-7866: Predicates, helpers for implicit casts, slot refs .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/11953/3/fe/src/main/java/org/apache/impala/analysis/Expr.java File fe/src/main/java/org/apache/impala/analysis/Expr.java: http://gerrit.cloudera.org:8080/#/c/11953/3/fe/src/main/java/org/apache/impala/analysis/Expr.java@304 PS3, Line 304: line has trailing whitespace -- To view, visit http://gerrit.cloudera.org:8080/11953 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ieaa0aee1b9015e0aed521f2038bf44513d7f8613 Gerrit-Change-Number: 11953 Gerrit-PatchSet: 3 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Wed, 23 Jan 2019 20:44:45 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5051: Add INT64 timestamp write support in Parquet
Csaba Ringhofer has posted comments on this change. (
http://gerrit.cloudera.org:8080/12247 )
Change subject: IMPALA-5051: Add INT64 timestamp write support in Parquet
..
Patch Set 3:
(7 comments)
About 'parquet-tools dump': it would be great to use it during Impala EE tests,
but adding it as a dependency would also mean +1 way Impala builds can break,
so I am not sure at the moment.
http://gerrit.cloudera.org:8080/#/c/12247/3/be/src/exec/parquet/hdfs-parquet-table-writer.cc
File be/src/exec/parquet/hdfs-parquet-table-writer.cc:
http://gerrit.cloudera.org:8080/#/c/12247/3/be/src/exec/parquet/hdfs-parquet-table-writer.cc@400
PS3, Line 400: !=
> Shouldn't it be '=='?
It is intentionally !=. -1 means variable length, and plain_encoded_value_size_
should remain -1 in that case.
Actually this code led to DCHECK error during the writing of some decimals,
which was not caught by the tests I ran locally. After running to issues on
jenkins, I moved this logic Int64TimestampColumnWriterBase.
http://gerrit.cloudera.org:8080/#/c/12247/3/be/src/exec/parquet/hdfs-parquet-table-writer.cc@583
PS3, Line 583: Overrides of this function are expected to set 'result_'
> I think an output parameter would be more conventional.
I prefer the current solution - my guess is that adding an additional argument
to a non-inline functions will make it slower, while writing to result_ is
cheaper as 'this' is used during the function call anyway.
http://gerrit.cloudera.org:8080/#/c/12247/3/be/src/exec/parquet/parquet-metadata-utils.cc
File be/src/exec/parquet/parquet-metadata-utils.cc:
http://gerrit.cloudera.org:8080/#/c/12247/3/be/src/exec/parquet/parquet-metadata-utils.cc@158
PS3, Line 158: // converted_type is not set because older readers that do not
use logical types
> Ah, yes, good point, I haven't noticed we are only dealing with LocalDateTi
Done
http://gerrit.cloudera.org:8080/#/c/12247/3/be/src/runtime/timestamp-value.inline.h
File be/src/runtime/timestamp-value.inline.h:
http://gerrit.cloudera.org:8080/#/c/12247/3/be/src/runtime/timestamp-value.inline.h@135
PS3, Line 135: (NANOS_PER_MILLI / 2)
> Might be worth to add a short comment about that we do the rounding here.
The whole rounding logic is under discussion - Impala uses rounding to microsec
when writing Kudu, but I think that truncating towards negative infinity would
be better. I sent an email to d...@impala.apache.org about this topic, I plan
to wait for an agreement there before changing these functions.
http://gerrit.cloudera.org:8080/#/c/12247/3/be/src/runtime/timestamp-value.inline.h@151
PS3, Line 151: kudu::int128_t nanos128 =
: static_cast(unixtime_seconds) * NANOS_PER_SEC
: + time_.fractional_seconds();
:
: if (nanos128 < std::numeric_limits::min()
: || nanos128 > std::numeric_limits::max()) return
false;
> I think we can avoid using int128_t here. We now what are the min and max u
I agree, but I am waiting for a decision in rounding vs truncating, see line
135.
http://gerrit.cloudera.org:8080/#/c/12247/3/be/src/service/query-options.cc
File be/src/service/query-options.cc:
http://gerrit.cloudera.org:8080/#/c/12247/3/be/src/service/query-options.cc@728
PS3, Line 728: if (iequals(value, "INT96_NANO")) {
> You are right, I think that I messed something up like running the tests wi
Done
http://gerrit.cloudera.org:8080/#/c/12247/3/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:
http://gerrit.cloudera.org:8080/#/c/12247/3/common/thrift/ImpalaInternalService.thrift@1
PS3, Line 1:
> nit: accidental new line
Done
--
To view, visit http://gerrit.cloudera.org:8080/12247
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib41ad532ec902ed5a9a1528513726eac1c11441f
Gerrit-Change-Number: 12247
Gerrit-PatchSet: 3
Gerrit-Owner: Csaba Ringhofer
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Zoltan Borok-Nagy
Gerrit-Reviewer: Zoltan Ivanfi
Gerrit-Comment-Date: Wed, 23 Jan 2019 20:50:38 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8095: Detailed expression cardinality tests
Impala Public Jenkins has posted comments on this change. (
http://gerrit.cloudera.org:8080/12248 )
Change subject: IMPALA-8095: Detailed expression cardinality tests
..
Patch Set 3:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/12248/3/fe/src/test/java/org/apache/impala/analysis/ExprCardinalityTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprCardinalityTest.java:
http://gerrit.cloudera.org:8080/#/c/12248/3/fe/src/test/java/org/apache/impala/analysis/ExprCardinalityTest.java@356
PS3, Line 356: private void verifyInequalitySel(String table, String col,
String value) throws ImpalaException {
line too long (99 > 90)
http://gerrit.cloudera.org:8080/#/c/12248/3/fe/src/test/java/org/apache/impala/analysis/ExprCardinalityTest.java@462
PS3, Line 462: verifySelectExpr("alltypes", "int_col in (1, 2, 3, 4, 5, 6,
7, 8, 9, 10, 11, 12)", 3, 1);
line too long (93 > 90)
http://gerrit.cloudera.org:8080/#/c/12248/3/fe/src/test/java/org/apache/impala/analysis/ExprCardinalityTest.java@490
PS3, Line 490: verifySelectExpr("alltypes", "int_col not in (1, 2, 3, 4, 5,
6, 7, 8, 9, 10, 11, 12)", 3, 0);
line too long (97 > 90)
http://gerrit.cloudera.org:8080/#/c/12248/3/fe/src/test/java/org/apache/impala/common/FrontendFixture.java
File fe/src/test/java/org/apache/impala/common/FrontendFixture.java:
http://gerrit.cloudera.org:8080/#/c/12248/3/fe/src/test/java/org/apache/impala/common/FrontendFixture.java@341
PS3, Line 341: return ctx.analyzeAndAuthorize(parsedStmt, stmtTableCache,
frontend_.getAuthzChecker());
line too long (92 > 90)
http://gerrit.cloudera.org:8080/#/c/12248/3/fe/src/test/java/org/apache/impala/planner/CardinalityTest.java
File fe/src/test/java/org/apache/impala/planner/CardinalityTest.java:
http://gerrit.cloudera.org:8080/#/c/12248/3/fe/src/test/java/org/apache/impala/planner/CardinalityTest.java@106
PS3, Line 106: verifyCardinality("SELECT null_int FROM functional.nullrows
WHERE group_str = 'x'", 4);
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/12248/3/fe/src/test/java/org/apache/impala/planner/CardinalityTest.java@110
PS3, Line 110: //verifyCardinality("SELECT null_int FROM
functional.nullrows WHERE null_str = 'x'", 26);
line too long (93 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/12248
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3da58ee9b0beebeffb170b9430bd36d20dcd2401
Gerrit-Change-Number: 12248
Gerrit-PatchSet: 3
Gerrit-Owner: Paul Rogers
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Paul Rogers
Gerrit-Comment-Date: Wed, 23 Jan 2019 20:48:51 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8095: Detailed expression cardinality tests
Hello Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12248 to look at the new patch set (#3). Change subject: IMPALA-8095: Detailed expression cardinality tests .. IMPALA-8095: Detailed expression cardinality tests Cardinality is a critical input to the query planning process, especially join planning. Impala has many high-level end-to-end tests that implicitly test cardinality at the "wholesale" level: A test will produce a wrong result if the cardinality is badly wrong. Until this patch, no detailed unit tests exist for this topic. Tests here include: * Table cardinality, NDV values and null count in metadata retrieved from HMS. * Table cardinality, NDV values and null counts in metadata presented to the query. * Expression NDV and selectivity values (which derive from table cardinality and column NDV.) The test illustrate a number of bugs. This patch simply identifies the bugs, comments out the tests that fail because of the bugs, and substitutes tests that pass with the current, incorrect, behavior. Future patches will fix the bugs. Reviewers can note the difference between the original, incorrect behavior shown here, and the revised behavior in those additional patches. Since none of the existing "functional" tables provide the level of detail needed for these tests, added a new test table specifically for this task. This set of tests was a good time to extend the test "fixture" framework created earlier. The FrontendTestBase class was refactored to use a new FrontendFixture which represents a (simulated) Impala and HMS cluster. The previoius SessionFixture represents a single user session (with session options) and the QueryFixture represents a single query. As part of this refactoring, the fixture classes moved into "common" alongside FrontendTestBase. Testing: This patch includes only tests: no "production" code was changed. Change-Id: I3da58ee9b0beebeffb170b9430bd36d20dcd2401 --- M fe/src/main/java/org/apache/impala/analysis/Expr.java M fe/src/main/java/org/apache/impala/analysis/SelectListItem.java M fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java D fe/src/test/java/org/apache/impala/analysis/AnalysisSessionFixture.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java A fe/src/test/java/org/apache/impala/analysis/ExprCardinalityTest.java M fe/src/test/java/org/apache/impala/analysis/ExprNdvTest.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java A fe/src/test/java/org/apache/impala/common/AbstractFrontendTest.java A fe/src/test/java/org/apache/impala/common/AnalysisSessionFixture.java A fe/src/test/java/org/apache/impala/common/FrontendFixture.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java A fe/src/test/java/org/apache/impala/common/QueryFixture.java M fe/src/test/java/org/apache/impala/planner/CardinalityTest.java A testdata/NullRows/data.csv M testdata/bin/compute-table-stats.sh M testdata/datasets/functional/functional_schema_template.sql M testdata/datasets/functional/schema_constraints.csv 18 files changed, 1,680 insertions(+), 535 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/12248/3 -- To view, visit http://gerrit.cloudera.org:8080/12248 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3da58ee9b0beebeffb170b9430bd36d20dcd2401 Gerrit-Change-Number: 12248 Gerrit-PatchSet: 3 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers
[Impala-ASF-CR] IMPALA-8091: incremental improvements to NTP sync
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12253 ) Change subject: IMPALA-8091: incremental improvements to NTP sync .. Patch Set 2: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/12253 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifc8cacf81765d132dd574993f8149231bdbb7bc6 Gerrit-Change-Number: 12253 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Brown Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Laszlo Gaal Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Wed, 23 Jan 2019 20:47:06 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8091: incremental improvements to NTP sync
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12253 ) Change subject: IMPALA-8091: incremental improvements to NTP sync .. IMPALA-8091: incremental improvements to NTP sync - Warn when ntp-wait is not available. - Install ntp-perl for Redhat-based systems, which provides ntp-wait. Debian-based systems already have ntp-wait as provided by ntp. Change-Id: Ifc8cacf81765d132dd574993f8149231bdbb7bc6 Reviewed-on: http://gerrit.cloudera.org:8080/12253 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M bin/bootstrap_system.sh M testdata/cluster/admin 2 files changed, 4 insertions(+), 2 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/12253 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ifc8cacf81765d132dd574993f8149231bdbb7bc6 Gerrit-Change-Number: 12253 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Brown Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Laszlo Gaal Gerrit-Reviewer: Philip Zeyliger
[Impala-ASF-CR] IMPALA-7866: Predicates, helpers for implicit casts, slot refs
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/11953 ) Change subject: IMPALA-7866: Predicates, helpers for implicit casts, slot refs .. Patch Set 2: Rebased on latest master to resolve merge conflict. Reran FE tests to validate the rebase. -- To view, visit http://gerrit.cloudera.org:8080/11953 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ieaa0aee1b9015e0aed521f2038bf44513d7f8613 Gerrit-Change-Number: 11953 Gerrit-PatchSet: 2 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Wed, 23 Jan 2019 20:44:30 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7866: Predicates, helpers for implicit casts, slot refs
Hello Bharath Vissapragada, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11953 to look at the new patch set (#3). Change subject: IMPALA-7866: Predicates, helpers for implicit casts, slot refs .. IMPALA-7866: Predicates, helpers for implicit casts, slot refs * Refactors implicit cast functions into a predicate and a static helper rather than the methods on the Expr base class. * Adds an IS_SLOT_REF predicate and replaces explcit instanceof checks with uses of the predicate. * Other minor clean-up. Tests: No functional changes, just refactoring. Reran existing tests. Change-Id: Ieaa0aee1b9015e0aed521f2038bf44513d7f8613 --- M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/analysis/CastExpr.java M fe/src/main/java/org/apache/impala/analysis/Expr.java M fe/src/main/java/org/apache/impala/analysis/PartitionSet.java M fe/src/main/java/org/apache/impala/analysis/QueryStmt.java M fe/src/main/java/org/apache/impala/analysis/SelectListItem.java M fe/src/main/java/org/apache/impala/analysis/SlotRef.java M fe/src/main/java/org/apache/impala/analysis/SortInfo.java M fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java M fe/src/main/java/org/apache/impala/analysis/UnionStmt.java M fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java M fe/src/main/java/org/apache/impala/common/TreeNode.java M fe/src/main/java/org/apache/impala/planner/DataSourceScanNode.java M fe/src/main/java/org/apache/impala/planner/HdfsPartitionPruner.java M fe/src/main/java/org/apache/impala/planner/KuduScanNode.java M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java M fe/src/main/java/org/apache/impala/planner/SortNode.java M fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java 19 files changed, 97 insertions(+), 93 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/53/11953/3 -- To view, visit http://gerrit.cloudera.org:8080/11953 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ieaa0aee1b9015e0aed521f2038bf44513d7f8613 Gerrit-Change-Number: 11953 Gerrit-PatchSet: 3 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers
[Impala-ASF-CR] IMPALA-8092: Add an admission controller debug page
Tim Armstrong has posted comments on this change. (
http://gerrit.cloudera.org:8080/12244 )
Change subject: IMPALA-8092: Add an admission controller debug page
..
Patch Set 1:
(19 comments)
This is cool - I checked it out and played around with it a bit. I think we can
improve the ergonomics a bit, but I really liked just being able to see what's
going on in the admission controller in real time.
http://gerrit.cloudera.org:8080/#/c/12244/1/be/src/scheduling/admission-controller.h
File be/src/scheduling/admission-controller.h:
http://gerrit.cloudera.org:8080/#/c/12244/1/be/src/scheduling/admission-controller.h@403
PS1, Line 403: void ResetStats();
We might want to rename this - it seems like this only resets informational
statistics, not statistics that are used by the admission control algorithms.
Not sure if there's a better name than "Informational"
http://gerrit.cloudera.org:8080/#/c/12244/1/be/src/scheduling/admission-controller.h@449
PS1, Line 449: std::vector peak_mem_histogram_;
> just looked at HdrHistogram, it seems like a full blown implementation of a
Yeah I took a look too and it seemed non-trivial to use. I'm fine with leaving
as-is for now.
http://gerrit.cloudera.org:8080/#/c/12244/1/be/src/scheduling/admission-controller.h@455
PS1, Line 455: EMA
I think lower-case ema is more consistent with elsewhere.
http://gerrit.cloudera.org:8080/#/c/12244/1/be/src/scheduling/admission-controller.h@593
PS1, Line 593: Query
lower case?
http://gerrit.cloudera.org:8080/#/c/12244/1/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:
http://gerrit.cloudera.org:8080/#/c/12244/1/be/src/scheduling/admission-controller.cc@1098
PS1, Line 1098: bind(QueueNodeToJsonHelperCallback,
_queries, document, _1));
Use a lambda?
http://gerrit.cloudera.org:8080/#/c/12244/1/be/src/service/impala-http-handler.cc
File be/src/service/impala-http-handler.cc:
http://gerrit.cloudera.org:8080/#/c/12244/1/be/src/service/impala-http-handler.cc@145
PS1, Line 145: admission_controller
maybe just /admission to be more concise?
http://gerrit.cloudera.org:8080/#/c/12244/1/be/src/service/impala-http-handler.cc@148
PS1, Line 148: resource_pool_reset
This shows up at the top of the debug page now. I think you need to pass in
false like some of the above cases. This might be a good time to make the
is_on_nav_bar argument to RegisterUrlCallback non-default.
http://gerrit.cloudera.org:8080/#/c/12244/1/be/src/service/impala-http-handler.cc@873
PS1, Line 873: QueryInfo(const TUniqueId& q_id, int64_t memory_limit,
int64_t memory_limit_for_ac,
We might be able to use a struct initializer {} and avoid this constructor.
http://gerrit.cloudera.org:8080/#/c/12244/1/be/src/service/impala-http-handler.cc@886
PS1, Line 886: &
Consider just passing in the variables that need to be accessed, i.e.
running_queries
http://gerrit.cloudera.org:8080/#/c/12244/1/be/src/service/impala-http-handler.cc@889
PS1, Line 889: if (request_state->operation_state() !=
TOperationState::INITIALIZED_STATE
I think we probably want to read operation_state() once and put it in a local
rather than on two branches of the condition. It's hard to otherwise reason
about what might happen if the state changes in-between the conditions being
evaluated.
http://gerrit.cloudera.org:8080/#/c/12244/1/be/src/service/impala-http-handler.cc@923
PS1, Line 923: // In order to embed a plain json inside the webpage generated
by mustache, we need
This is a little unfortunate but I think we can live with it.
http://gerrit.cloudera.org:8080/#/c/12244/1/www/admission_controller.tmpl
File www/admission_controller.tmpl:
PS1:
It would be nice if we had a way to show only a single resource pool on a page
(maybe just with an argument to filter them). Then if the pool headings linked
to those pages.
I'm thinking it would be convenient to be able to look at a single page and F5
it.
http://gerrit.cloudera.org:8080/#/c/12244/1/www/admission_controller.tmpl@194
PS1, Line 194: This page lists all resource pools are their
corresponding stats.
It actually only lists resource pools that queries have been submitted to
(since they are lazily created).
http://gerrit.cloudera.org:8080/#/c/12244/1/www/admission_controller.tmpl@210
PS1, Line 210: Requests
I think "Max Concurrent Queries" would be more intuitive (we use "Queries"
elsewhere in the page.
http://gerrit.cloudera.org:8080/#/c/12244/1/www/admission_controller.tmpl@223
PS1, Line 223: min_query_mem_limit
should be max
http://gerrit.cloudera.org:8080/#/c/12244/1/www/admission_controller.tmpl@231
PS1, Line 231: local
maybe "submitted to this coordinator" instead of "local to this coordinator" -
might match user-facing concepts better.
http://gerrit.cloudera.org:8080/#/c/12244/1/www/admission_controller.tmpl@283
PS1, Line 283: Queries Currently Running
Is is reasonably
[Impala-ASF-CR] IMPALA-5051: Add INT64 timestamp write support in Parquet
Hello Zoltan Borok-Nagy, Zoltan Ivanfi, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12247 to look at the new patch set (#5). Change subject: IMPALA-5051: Add INT64 timestamp write support in Parquet .. IMPALA-5051: Add INT64 timestamp write support in Parquet Add query option "parquet_timestamp_type" that chooses the Parquet type used when writing TIMESTAMP columns. This is an experimental feature at the moment, because these types are not widely adopted in other Hadoop components yet. For this reason the query option is added as "development" level, and the default behavior is not changed. The following options can be used: INT96_NANOS (default): This is the same as the old behavior, can represent any timestamp that can be handled by Impala. INT64_MILLIS, INT64_MICROS: Can encode the whole [1400..1) range handled by Impala at the cost of reduced precision. Values are rounded to the nearest ms/us during writing. Values that would be rounded to year 1 are rounded down to avoid converting valid values to invalid ones. INT64_NANOS: Can encode a reduced range without losing nanosecond precision: [1677-09-21 00:12:43.145224192 .. 2262-04-11 23:47:16.854775807] Values outside this range are converted to NULLs without warning. The change was done completely in the backend and all TIMESTAMP columns are written using the type set in the query option. An alternative design would have been to implement some parts in the fronted by adding TIMESTAMP->BIGINT conversion functions to the query plan, which would make it easier to add the possibility of per-column setting in the future. I choose the current design because it seemed much simpler and there are no clear plans for the per-column setting. Most of the code will be still useful if we decide to go the other way in the future. All types are written without conversion to UTC (as Impala always wrote timestamps), and this information is expressed in the new Parquet logical types by setting isAdjustedToUTC to false. The old logical type (converted_type) is net set, because old readers do not read isAdjustedToUTC, and assume that TIMESTAMP_MILLIS and TIMESTAMP_MICROS are written in UTC. These readers can still read int64 timestamp columns as INT_64. Testing: - added unit tests for new TimestampValue->int64 functions - add EE tests for checking values / min-max stats / metadata written for int64 Parquet timestamps - ran core tests Change-Id: Ib41ad532ec902ed5a9a1528513726eac1c11441f --- M be/src/exec/parquet/hdfs-parquet-table-writer.cc M be/src/exec/parquet/hdfs-parquet-table-writer.h M be/src/exec/parquet/parquet-common.cc M be/src/exec/parquet/parquet-common.h M be/src/exec/parquet/parquet-metadata-utils.cc M be/src/exec/parquet/parquet-metadata-utils.h M be/src/runtime/timestamp-test.cc M be/src/runtime/timestamp-value.h M be/src/runtime/timestamp-value.inline.h M be/src/service/query-options.cc M be/src/service/query-options.h M be/src/util/debug-util.cc M be/src/util/debug-util.h M common/thrift/ImpalaInternalService.thrift M common/thrift/ImpalaService.thrift M testdata/workloads/functional-query/queries/QueryTest/parquet-int64-timestamps.test M tests/query_test/test_insert_parquet.py M tests/util/get_parquet_metadata.py 18 files changed, 515 insertions(+), 66 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/12247/5 -- To view, visit http://gerrit.cloudera.org:8080/12247 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib41ad532ec902ed5a9a1528513726eac1c11441f Gerrit-Change-Number: 12247 Gerrit-PatchSet: 5 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Reviewer: Zoltan Ivanfi
[Impala-ASF-CR] IMPALA-7905: Hive keywords not quoted for identifiers
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12009 ) Change subject: IMPALA-7905: Hive keywords not quoted for identifiers .. Patch Set 12: (1 comment) Addressed the code review comment. http://gerrit.cloudera.org:8080/#/c/12009/11/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java File fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java: http://gerrit.cloudera.org:8080/#/c/12009/11/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java@147 PS11, Line 147: true if the identifier is an Impala keyword, or if starts :* with a digit > update. Done -- To view, visit http://gerrit.cloudera.org:8080/12009 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I06cc20b052a3a66535a171c36b4b31477c0ba6d0 Gerrit-Change-Number: 12009 Gerrit-PatchSet: 12 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Wed, 23 Jan 2019 20:31:41 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7905: Hive keywords not quoted for identifiers
Hello Bharath Vissapragada, Fredy Wijaya, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12009 to look at the new patch set (#12). Change subject: IMPALA-7905: Hive keywords not quoted for identifiers .. IMPALA-7905: Hive keywords not quoted for identifiers Impala often generates SQL for statements using the toSql() call. Generated SQL is often used during testing or when writing the query plan. Impala keywords such as "create", when used as identifiers, must be quoted: SELECT `select`, `from` FROM `order` ... The code in ToSqlUtils.getIdentSql() quotes the identifier if it is an Impala or Hive keyword, or if it does not follow the identifier pattern. The code uses the Hive lexer to detect a keyword. But, the code contained a flaw: the lexer expects a case-insensitive input. We provide a case sensitive input. As a result, "MONTH" is caught as a Hive keyword and quoted, but "month" is not. This patch fixes that flaw. This patch also fixes: IMPALA-8051: Compute stats fails on a column with comment character in name The code uses the Hive lexical analyzer to check names. Since "#" and "--" are comment characters, a name like "foo#" is parsed as "foo" which does not need quotes, hence we don't quote "foo#", which causes issues. Added a special check for "#" and "--" to resolve this issue. Testing: * Refactored getIdentSql() easier testing. * Added a tests to the recently added ToSqlUtilsTest for this case and several others. * Making this change caused the columns `month`, `year`, and `key` to be quoted when before they were not. Updated many tests as a result. * Added a new identSql() function, for use in tests, to match the quoting that Impala uses, and to handle the wildcard, and multi-part names. Used this in ToSqlTest to handle the quoted names. * PlannerTest emits statement SQL to the output file wrapped to 80 columns and sometimes leaves trailing spaces at the end of the line. Some tools remove that trailing space, resulting in trivial file differences. Fixed this to remove trailing spaces in order to simplify file comparisons. * Tweaked the "In pipelines" output to avoid trailing spaces when no pipelines are listed. * Reran all FE tests. Change-Id: I06cc20b052a3a66535a171c36b4b31477c0ba6d0 --- M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java M fe/src/main/java/org/apache/impala/common/PrintUtils.java M fe/src/main/java/org/apache/impala/planner/PlanNode.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlUtilsTest.java M fe/src/test/java/org/apache/impala/planner/PlannerTest.java M fe/src/test/java/org/apache/impala/util/PrintUtilsTest.java M testdata/workloads/functional-planner/queries/PlannerTest/aggregation.test M testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test M testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test M testdata/workloads/functional-planner/queries/PlannerTest/ddl.test M testdata/workloads/functional-planner/queries/PlannerTest/hbase.test M testdata/workloads/functional-planner/queries/PlannerTest/hdfs.test M testdata/workloads/functional-planner/queries/PlannerTest/implicit-joins.test M testdata/workloads/functional-planner/queries/PlannerTest/inline-view.test M testdata/workloads/functional-planner/queries/PlannerTest/insert-sort-by.test M testdata/workloads/functional-planner/queries/PlannerTest/insert.test M testdata/workloads/functional-planner/queries/PlannerTest/join-order.test M testdata/workloads/functional-planner/queries/PlannerTest/joins.test M testdata/workloads/functional-planner/queries/PlannerTest/kudu-upsert.test M testdata/workloads/functional-planner/queries/PlannerTest/lineage.test M testdata/workloads/functional-planner/queries/PlannerTest/nested-collections.test M testdata/workloads/functional-planner/queries/PlannerTest/order.test M testdata/workloads/functional-planner/queries/PlannerTest/parquet-filtering-disabled.test M testdata/workloads/functional-planner/queries/PlannerTest/parquet-filtering.test M testdata/workloads/functional-planner/queries/PlannerTest/parquet-stats-agg.test M testdata/workloads/functional-planner/queries/PlannerTest/partition-key-scans.test M testdata/workloads/functional-planner/queries/PlannerTest/predicate-propagation.test M testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test M
[Impala-ASF-CR] IMPALA-8092: Add an admission controller debug page
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/12244 ) Change subject: IMPALA-8092: Add an admission controller debug page .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/12244/1/be/src/scheduling/admission-controller.h File be/src/scheduling/admission-controller.h: http://gerrit.cloudera.org:8080/#/c/12244/1/be/src/scheduling/admission-controller.h@449 PS1, Line 449: std::vector peak_mem_histogram_; > Quick initial question, can't remember if we discussed this offline - why n just looked at HdrHistogram, it seems like a full blown implementation of a histogram, not sure it its an overkill for our use case. I just went with fixed bucket for easy lookup of appropriate bucket and to avoid keeping track of the bucket-to-value mapping. Definitely a lot of ways we can do this and something more clever to allow variable range and buckets. open to suggestions. -- To view, visit http://gerrit.cloudera.org:8080/12244 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iff055d9709ea1bcc2f492adcde92241b6149f766 Gerrit-Change-Number: 12244 Gerrit-PatchSet: 1 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 23 Jan 2019 19:53:43 + Gerrit-HasComments: Yes
[Impala-ASF-CR] Support centos:7 for test-with-docker.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12139 ) Change subject: Support centos:7 for test-with-docker. .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1860/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12139 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7577949b6eaaa2239bcf0fadf64e1490c2106b08 Gerrit-Change-Number: 12139 Gerrit-PatchSet: 2 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Laszlo Gaal Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Wed, 23 Jan 2019 19:47:16 + Gerrit-HasComments: No
[Impala-ASF-CR] Using 'master' branch of Impala-lzo and allowing test-with-docker to configure it.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12259 ) Change subject: Using 'master' branch of Impala-lzo and allowing test-with-docker to configure it. .. Patch Set 2: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3664/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/12259 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ieba45fc18d9e490f75d16c477cdc1cce26f41ce9 Gerrit-Change-Number: 12259 Gerrit-PatchSet: 2 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 23 Jan 2019 18:56:28 + Gerrit-HasComments: No
[Impala-ASF-CR] Support centos:7 for test-with-docker.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12139 ) Change subject: Support centos:7 for test-with-docker. .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12139 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7577949b6eaaa2239bcf0fadf64e1490c2106b08 Gerrit-Change-Number: 12139 Gerrit-PatchSet: 3 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Laszlo Gaal Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Wed, 23 Jan 2019 18:58:42 + Gerrit-HasComments: No
[Impala-ASF-CR] Support centos:7 for test-with-docker.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12139 ) Change subject: Support centos:7 for test-with-docker. .. Patch Set 3: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3665/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/12139 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7577949b6eaaa2239bcf0fadf64e1490c2106b08 Gerrit-Change-Number: 12139 Gerrit-PatchSet: 3 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Laszlo Gaal Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Wed, 23 Jan 2019 18:58:43 + Gerrit-HasComments: No
[Impala-ASF-CR] Using 'master' branch of Impala-lzo and allowing test-with-docker to configure it.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12259 ) Change subject: Using 'master' branch of Impala-lzo and allowing test-with-docker to configure it. .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12259 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ieba45fc18d9e490f75d16c477cdc1cce26f41ce9 Gerrit-Change-Number: 12259 Gerrit-PatchSet: 2 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 23 Jan 2019 18:56:27 + Gerrit-HasComments: No
[Impala-ASF-CR] Support centos:7 for test-with-docker.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/12139 ) Change subject: Support centos:7 for test-with-docker. .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12139 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7577949b6eaaa2239bcf0fadf64e1490c2106b08 Gerrit-Change-Number: 12139 Gerrit-PatchSet: 2 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Laszlo Gaal Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Wed, 23 Jan 2019 18:56:17 + Gerrit-HasComments: No
[Impala-ASF-CR] Support centos:7 for test-with-docker.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12139 ) Change subject: Support centos:7 for test-with-docker. .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/12139/2/docker/entrypoint.sh File docker/entrypoint.sh: http://gerrit.cloudera.org:8080/#/c/12139/2/docker/entrypoint.sh@51 PS2, Line 51: sudo -u postgres PGDATA=/var/lib/pgsql/data bash -c "pg_ctl $1 -w --timeout=120 >> /var/lib/pgsql/pg.log 2>&1" line too long (116 > 90) -- To view, visit http://gerrit.cloudera.org:8080/12139 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7577949b6eaaa2239bcf0fadf64e1490c2106b08 Gerrit-Change-Number: 12139 Gerrit-PatchSet: 2 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Laszlo Gaal Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Wed, 23 Jan 2019 18:54:51 + Gerrit-HasComments: Yes
[Impala-ASF-CR] Support centos:7 for test-with-docker.
Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/12139 ) Change subject: Support centos:7 for test-with-docker. .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/12139/1/bin/bootstrap_system.sh File bin/bootstrap_system.sh: http://gerrit.cloudera.org:8080/#/c/12139/1/bin/bootstrap_system.sh@253 PS1, Line 253: redhat6 sudo service ntpd start || grep docker /proc/1/cgroup > Ignoring the nit, we stop ntpd on redhat7 notindocker. Don't we need to sta Ah, I understand now. Adding. -- To view, visit http://gerrit.cloudera.org:8080/12139 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7577949b6eaaa2239bcf0fadf64e1490c2106b08 Gerrit-Change-Number: 12139 Gerrit-PatchSet: 1 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Laszlo Gaal Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Wed, 23 Jan 2019 18:54:41 + Gerrit-HasComments: Yes
[Impala-ASF-CR] Support centos:7 for test-with-docker.
Hello Laszlo Gaal, Joe McDonnell, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12139 to look at the new patch set (#2). Change subject: Support centos:7 for test-with-docker. .. Support centos:7 for test-with-docker. As a follow-on to IMPALA-7698, adds various incantations so that centos:7 can build under test-with-docker. The core issue is that the centos:7 image doesn't let you start sshd (necessary for the HBase startup scripts, and probably could be worked around) or postgresql (harder to work around) with systemctl, because systemd isn't "running." To avoid this, we start them manually with /usr/sbin/sshd and pg_ctl. Change-Id: I7577949b6eaaa2239bcf0fadf64e1490c2106b08 --- M bin/bootstrap_system.sh M docker/entrypoint.sh 2 files changed, 90 insertions(+), 12 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/39/12139/2 -- To view, visit http://gerrit.cloudera.org:8080/12139 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7577949b6eaaa2239bcf0fadf64e1490c2106b08 Gerrit-Change-Number: 12139 Gerrit-PatchSet: 2 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Laszlo Gaal Gerrit-Reviewer: Philip Zeyliger
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12260 ) Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC. .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1859/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12260 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e Gerrit-Change-Number: 12260 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 23 Jan 2019 18:38:27 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7980: Fix spinning because of buggy num unqueued files .
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12097 ) Change subject: IMPALA-7980: Fix spinning because of buggy num_unqueued_files_. .. Patch Set 3: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/1858/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/12097 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I133de13238d3d05c510e2ff771d48979125735b1 Gerrit-Change-Number: 12097 Gerrit-PatchSet: 3 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 23 Jan 2019 18:26:55 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5872: Testcase builder for query planner
Fredy Wijaya has posted comments on this change. (
http://gerrit.cloudera.org:8080/12221 )
Change subject: IMPALA-5872: Testcase builder for query planner
..
Patch Set 2:
(19 comments)
http://gerrit.cloudera.org:8080/#/c/12221/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12221/2//COMMIT_MSG@41
PS2, Line 41:
nit: remove new line
http://gerrit.cloudera.org:8080/#/c/12221/2/common/thrift/Frontend.thrift
File common/thrift/Frontend.thrift:
http://gerrit.cloudera.org:8080/#/c/12221/2/common/thrift/Frontend.thrift@915
PS2, Line 915: // Query statemnt for which this test case data is generated.
: 1: required string query_stmt
: // All referenced table and view defs.
: 2: optional list tables_and_views
: // All databases referenced in the query.
: 3: optional list dbs
: // Output path
: 4: required string testcase_data_path
: // Impala version that was used to generate this testcase.
: // TODO: How to deal with version incompatibilities? E.g: A
testcase collected on
: // Impala version v1 may or may not be compatible to Impala
version v2 if the
: // underlying thrift layout changes.
: 5: required string impala_version
nit: Adding a new line after each field makes it a bit more readable.
http://gerrit.cloudera.org:8080/#/c/12221/2/common/thrift/JniCatalog.thrift
File common/thrift/JniCatalog.thrift:
http://gerrit.cloudera.org:8080/#/c/12221/2/common/thrift/JniCatalog.thrift@747
PS2, Line 747:
nit: remove extra new lines.
http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/cup/sql-parser.cup
File fe/src/main/cup/sql-parser.cup:
http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/cup/sql-parser.cup@769
PS2, Line 769: KW_TESTCASE
I don't think "testcase" is a reserved SQL keyword. So, instead of introducing
a new "testcase" keyword, should we use ident trick?
KW_COPY IDENT:ident KW_TO STRING_LITERAL:path query_sttmt:query
{:
if (!ident.toUppercase().equals("TESTCASE") { parser.parseError() }
...
:}
http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java
File fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java:
http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java@62
PS2, Line 62: testOutputFilePrefix_
nit: use upper case variable name since it's static final
http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java@82
PS2, Line 82: }
nit: add new line
http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java@107
PS2, Line 107: true, true
add inline comments for true, true
http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java@114
PS2, Line 114: true, true
add inline comments for true, true
http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java@133
PS2, Line 133: Table table = (Table) referencedTable.getTable();
Add Preconditions.checkState(referencedTable.getTable() instanceof Table)
http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java@151
PS2, Line 151: View view = (View) feView;
Add Preconditions.checkState()
http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java@181
PS2, Line 181: os.close();
Wrap in try/finally or use try resource.
http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/analysis/HdfsUri.java
File fe/src/main/java/org/apache/impala/analysis/HdfsUri.java:
http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/analysis/HdfsUri.java@55
PS2, Line 55: false
Add comment /* throwIfNotExists */ false to easily distinguish between
registerPrivReq vs throwIfNotExists.
http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1306
PS2, Line 1306: }
nit: add new line
http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/catalog/FeTable.java
File fe/src/main/java/org/apache/impala/catalog/FeTable.java:
http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/catalog/FeTable.java@34
PS2, Line 34: nameComparator_
Variables in an interface are essentially static final, so using upper case for
the variable name is more appropriate, i.e. NAME_COMPARATOR. We could also use
lambda for this:
Comparator
[Impala-ASF-CR] Support centos:7 for test-with-docker.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/12139 ) Change subject: Support centos:7 for test-with-docker. .. Patch Set 1: (1 comment) I just have a small question about ntpd on redhat7. Beyond that, I'm ready to +2 this. http://gerrit.cloudera.org:8080/#/c/12139/1/bin/bootstrap_system.sh File bin/bootstrap_system.sh: http://gerrit.cloudera.org:8080/#/c/12139/1/bin/bootstrap_system.sh@253 PS1, Line 253: redhat6 sudo service ntpd start || grep docker /proc/1/cgroup > In a "privileged" container this can actually run, so I didn't take this su Ignoring the nit, we stop ntpd on redhat7 notindocker. Don't we need to start it back up for redhat7 notindocker? -- To view, visit http://gerrit.cloudera.org:8080/12139 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7577949b6eaaa2239bcf0fadf64e1490c2106b08 Gerrit-Change-Number: 12139 Gerrit-PatchSet: 1 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Laszlo Gaal Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Wed, 23 Jan 2019 18:18:20 + Gerrit-HasComments: Yes
[Impala-ASF-CR] Using 'master' branch of Impala-lzo and allowing test-with-docker to configure it.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12259 ) Change subject: Using 'master' branch of Impala-lzo and allowing test-with-docker to configure it. .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1857/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12259 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ieba45fc18d9e490f75d16c477cdc1cce26f41ce9 Gerrit-Change-Number: 12259 Gerrit-PatchSet: 1 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 23 Jan 2019 18:13:19 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Andrew Sherman has uploaded this change for review. (
http://gerrit.cloudera.org:8080/12260
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC.
..
IMPALA-7985: Port RemoteShutdown() to KRPC.
The :shutdown command is used to shutdown a remote server. The common
case is that a user specifies the impalad to shutdown by specifying a
host e.g. :shutdown('host100'). If a user has more than one impalad on a
remote host then the form :shutdown(':') can be used to
specify the port by which the imapald can be contacted. Prior to
IMPALA-7985 this port was the backend port, e.g.
:shutdown('host100:22000'). With IMPALA-7985 the port to use is the KRPC
port, e.g. :shutdown('host100:27000').
Shutdown is implemented by making an rpc call to the target impalad.
This changes the implementation of this call to use KRPC.
To aid the user in finding the KRPC port, the KRPC address is added to
the /backends section of the debug web page.
We attempt to detect the case where :shutdown is pointed at a thrift
port (like the backend port) and print an informative message.
Documentation of this change will be done in IMPALA-8098.
For discussion of why it was chosen to implement this change in an
incompatible way, see comments in
https://issues.apache.org/jira/browse/IMPALA-7985.
TESTING
Ran all end-to-end tests.
Add a test for /backends to test_web_pages.py.
In test_restart_services.py add a call to the old backend port to the
test. Some expected error messages were changed in line with what KRPC
returns.
Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
---
M be/src/runtime/backend-client.h
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/service/client-request-state.cc
M be/src/service/control-service.cc
M be/src/service/control-service.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-internal-service.cc
M be/src/service/impala-internal-service.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M common/protobuf/control_service.proto
M common/thrift/ImpalaInternalService.thrift
M tests/custom_cluster/test_restart_services.py
M tests/webserver/test_web_pages.py
M www/backends.tmpl
16 files changed, 219 insertions(+), 144 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/12260/1
--
To view, visit http://gerrit.cloudera.org:8080/12260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Gerrit-Change-Number: 12260
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] IMPALA-7980: Fix spinning because of buggy num unqueued files .
Philip Zeyliger has posted comments on this change. (
http://gerrit.cloudera.org:8080/12097 )
Change subject: IMPALA-7980: Fix spinning because of buggy num_unqueued_files_.
..
Patch Set 3:
(12 comments)
Thanks for the reviews. Here's another iteration.
The LZO stuff also needed handling, it turns out, so that's in progress.
http://gerrit.cloudera.org:8080/#/c/12097/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12097/2//COMMIT_MSG@7
PS2, Line 7: IMPALA-7980: Fix spinning because of buggy num_unqueued_files_.
> In theory some of the lines in this commit message should be shorter
I tried to shorten some stuff. Didn't get all of it.
http://gerrit.cloudera.org:8080/#/c/12097/2//COMMIT_MSG@21
PS2, Line 21: base-sequence-scanner.cc.
> did you consider an approach like adding something like this to the top lev
So, an HdfsScanNode can have multiple files, of different file formats. And
then it uses static methods to call IssueInitialRanges(). So I wasn't able to
shoe-horn in some object-oriented stuff in there. I didn't want to take on a
bigger refactoring.
Here's the relevant snippet from hdfs-scan-node-base.cc.
for (const auto& entry : matching_per_type_files) {
if (entry.second.empty()) continue;
switch (entry.first) {
case THdfsFileFormat::PARQUET:
RETURN_IF_ERROR(HdfsParquetScanner::IssueInitialRanges(this,
entry.second));
break;
case THdfsFileFormat::TEXT:
RETURN_IF_ERROR(HdfsTextScanner::IssueInitialRanges(this,
entry.second));
break;
case THdfsFileFormat::SEQUENCE_FILE:
case THdfsFileFormat::RC_FILE:
case THdfsFileFormat::AVRO:
RETURN_IF_ERROR(BaseSequenceScanner::IssueInitialRanges(this,
entry.second));
break;
case THdfsFileFormat::ORC:
RETURN_IF_ERROR(HdfsOrcScanner::IssueInitialRanges(this, entry.second));
break;
default:
DCHECK(false) << "Unexpected file type " << entry.first;
}
}
http://gerrit.cloudera.org:8080/#/c/12097/2//COMMIT_MSG@38
PS2, Line 38: required ~30 threads to be spinning in the kernel. We were able
to use
> typo
Done
http://gerrit.cloudera.org:8080/#/c/12097/2//COMMIT_MSG@74
PS2, Line 74: I se
> typo
Done
http://gerrit.cloudera.org:8080/#/c/12097/2/be/src/exec/hdfs-scan-node-base.h
File be/src/exec/hdfs-scan-node-base.h:
http://gerrit.cloudera.org:8080/#/c/12097/2/be/src/exec/hdfs-scan-node-base.h@266
PS2, Line 266: Must n
> Must?
Done
http://gerrit.cloudera.org:8080/#/c/12097/2/be/src/exec/hdfs-scan-node-base.h@277
PS2, Line 277: threads
> impala has plugins?
This is related to the Impala lzo plugin. The task here is to change the
signature in that repo, and then delete this signature. There's no
compatibility guarantee that I have to keep around; I just wanted to avoid
having to teach some test infrastructure about multiple branches.
Anyway, I didn't see [[deprecated]] in use elsewhere, and this should be
short-lived, so I'm going to ignore it at the moment.
http://gerrit.cloudera.org:8080/#/c/12097/2/be/src/exec/hdfs-scan-node-base.h@287
PS2, Line 287: Tuple* InitTemplateTuple(const
std::vector& value_evals,
> can you DCHECK that the result doesn't go below zero?
Done
http://gerrit.cloudera.org:8080/#/c/12097/2/be/src/exec/hdfs-scan-node-base.h@471
PS2, Line 471: m a slot's path to its index into ma
> yea, I was also confused by the inverted naming here.
Sorry about the confusing naming. As you can imagine, I went back and forth
between booleans and ints and stuff here.
I went with "remaining_scan_range_issuances_".
http://gerrit.cloudera.org:8080/#/c/12097/2/be/src/exec/hdfs-scan-node-base.cc
File be/src/exec/hdfs-scan-node-base.cc:
http://gerrit.cloudera.org:8080/#/c/12097/2/be/src/exec/hdfs-scan-node-base.cc@453
PS2, Line 453:
> is it important to not decrement this in the error cases below on line 471-
It has to be decremented *after* IssueInitialRanges(), since, if you don't, all
the scanner threads might exit and you'll hang. I was trying to catch all the
error cases and always decrement in those.
I think the easiest way to do it is to wrap inside of another function, so I'll
do that.
http://gerrit.cloudera.org:8080/#/c/12097/2/be/src/exec/hdfs-scan-node.h
File be/src/exec/hdfs-scan-node.h:
http://gerrit.cloudera.org:8080/#/c/12097/2/be/src/exec/hdfs-scan-node.h@139
PS2, Line 139:
: /// Number of times scanner thread didn't find work to do.
: RuntimeProfile::Counter*
scanner_thread_workless_loops_counter_ = nullptr;
> hrm, I see the value of this for your test, but not entirely convinced it m
I'm curious whether or not this loop is hot in practice at our customers, and
this was the easiest way to tell. I've seen this be ~10 in the benchmarking I
was doing, and it'll be useful to check up on it in more cases.
I considered getting rid of this whole loop and using condition
