Return-Path: X-Original-To: archive-asf-public-internal@cust-asf2.ponee.io Delivered-To: archive-asf-public-internal@cust-asf2.ponee.io Received: from cust-asf.ponee.io (cust-asf.ponee.io [163.172.22.183]) by cust-asf2.ponee.io (Postfix) with ESMTP id C6138200C8C for ; Tue, 6 Jun 2017 20:41:34 +0200 (CEST) Received: by cust-asf.ponee.io (Postfix) id C4C52160BD3; Tue, 6 Jun 2017 18:41:34 +0000 (UTC) Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by cust-asf.ponee.io (Postfix) with SMTP id C72E4160BB7 for ; Tue, 6 Jun 2017 20:41:33 +0200 (CEST) Received: (qmail 86688 invoked by uid 500); 6 Jun 2017 18:41:33 -0000 Mailing-List: contact commits-help@impala.incubator.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@impala.incubator.apache.org Delivered-To: mailing list commits@impala.incubator.apache.org Received: (qmail 86679 invoked by uid 99); 6 Jun 2017 18:41:33 -0000 Received: from pnap-us-west-generic-nat.apache.org (HELO spamd1-us-west.apache.org) (209.188.14.142) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 06 Jun 2017 18:41:33 +0000 Received: from localhost (localhost [127.0.0.1]) by spamd1-us-west.apache.org (ASF Mail Server at spamd1-us-west.apache.org) with ESMTP id 8FAB9CFF03 for ; Tue, 6 Jun 2017 18:41:32 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd1-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: -4.23 X-Spam-Level: X-Spam-Status: No, score=-4.23 tagged_above=-999 required=6.31 tests=[KAM_ASCII_DIVIDERS=0.8, RCVD_IN_DNSWL_HI=-5, RCVD_IN_MSPIKE_H3=-0.01, RCVD_IN_MSPIKE_WL=-0.01, SPF_PASS=-0.001, T_RP_MATCHES_RCVD=-0.01, URIBL_BLOCKED=0.001] autolearn=disabled Received: from mx1-lw-us.apache.org ([10.40.0.8]) by localhost (spamd1-us-west.apache.org [10.40.0.7]) (amavisd-new, port 10024) with ESMTP id aeXmH3NHIhNk for ; Tue, 6 Jun 2017 18:41:31 +0000 (UTC) Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by mx1-lw-us.apache.org (ASF Mail Server at mx1-lw-us.apache.org) with SMTP id 00A0C5FB5C for ; Tue, 6 Jun 2017 18:41:30 +0000 (UTC) Received: (qmail 86667 invoked by uid 99); 6 Jun 2017 18:41:30 -0000 Received: from git1-us-west.apache.org (HELO git1-us-west.apache.org) (140.211.11.23) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 06 Jun 2017 18:41:30 +0000 Received: by git1-us-west.apache.org (ASF Mail Server at git1-us-west.apache.org, from userid 33) id 34C45DFEF3; Tue, 6 Jun 2017 18:41:30 +0000 (UTC) Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit From: jrussell@apache.org To: commits@impala.incubator.apache.org Date: Tue, 06 Jun 2017 18:41:32 -0000 Message-Id: <239764c509e24bd3a9ee25cf10cf5e74@git.apache.org> In-Reply-To: <2af7ea4fc29b43a483880a60be2929e0@git.apache.org> References: <2af7ea4fc29b43a483880a60be2929e0@git.apache.org> X-Mailer: ASF-Git Admin Mailer Subject: [3/3] incubator-impala git commit: IMPALA-5282: Handle overflows in computeResourceProfile(). archived-at: Tue, 06 Jun 2017 18:41:34 -0000 IMPALA-5282: Handle overflows in computeResourceProfile(). An overflow in computeResourceProfile() could lead to negative resource estimates which later lead to a failed Preconditions check. I went through the computeResourceProfile() implementation of all data sinks and plan nodes and changed the code to handle overflows. Testing: - Reproduced the issue locally with the DDL provided in the JIRA. I manually set huge NDV stats to trigger overflow. Change-Id: I8a83a76141853d3274f812e5a531f456e1b110b1 Reviewed-on: http://gerrit.cloudera.org:8080/7084 Reviewed-by: Alex Behm Tested-by: Impala Public Jenkins Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/227a180a Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/227a180a Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/227a180a Branch: refs/heads/master Commit: 227a180ab927c485f24663600114af0157d69450 Parents: 02eb18b Author: Alex Behm Authored: Mon Jun 5 10:18:29 2017 -0700 Committer: Impala Public Jenkins Committed: Tue Jun 6 18:20:41 2017 +0000 ---------------------------------------------------------------------- .../java/org/apache/impala/planner/ExchangeNode.java | 2 +- .../java/org/apache/impala/planner/HdfsScanNode.java | 5 +++-- .../java/org/apache/impala/planner/HdfsTableSink.java | 5 +++-- .../main/java/org/apache/impala/planner/JoinNode.java | 6 +++--- .../java/org/apache/impala/planner/PlanFragment.java | 2 +- .../main/java/org/apache/impala/planner/PlanNode.java | 12 ++++++------ .../java/org/apache/impala/planner/SubplanNode.java | 2 +- .../main/java/org/apache/impala/planner/UnionNode.java | 2 +- 8 files changed, 19 insertions(+), 17 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/227a180a/fe/src/main/java/org/apache/impala/planner/ExchangeNode.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/planner/ExchangeNode.java b/fe/src/main/java/org/apache/impala/planner/ExchangeNode.java index d3997b8..3f4ea1e 100644 --- a/fe/src/main/java/org/apache/impala/planner/ExchangeNode.java +++ b/fe/src/main/java/org/apache/impala/planner/ExchangeNode.java @@ -93,7 +93,7 @@ public class ExchangeNode extends PlanNode { cardinality_ = -1; break; } - cardinality_ = addCardinalities(cardinality_, child.getCardinality()); + cardinality_ = checkedAdd(cardinality_, child.getCardinality()); } if (hasLimit()) { http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/227a180a/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java b/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java index 9fb87e0..a08f0e8 100644 --- a/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java +++ b/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java @@ -696,7 +696,7 @@ public class HdfsScanNode extends ScanNode { // enough to change the planning outcome if (p.getNumRows() > -1) { if (statsNumRows_ == -1) statsNumRows_ = 0; - statsNumRows_ = addCardinalities(statsNumRows_, p.getNumRows()); + statsNumRows_ = checkedAdd(statsNumRows_, p.getNumRows()); } else { ++numPartitionsMissingStats_; } @@ -988,7 +988,8 @@ public class HdfsScanNode extends ScanNode { long perThreadIoBuffers = Math.min((long) Math.ceil(avgScanRangeBytes / (double) readSize), MAX_IO_BUFFERS_PER_THREAD) + 1; - long perInstanceMemEstimate = maxScannerThreads * perThreadIoBuffers * readSize; + long perInstanceMemEstimate = checkedMultiply( + checkedMultiply(maxScannerThreads, perThreadIoBuffers), readSize); // Sanity check: the tighter estimation should not exceed the per-host maximum. long perHostUpperBound = getPerHostMemUpperBound(); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/227a180a/fe/src/main/java/org/apache/impala/planner/HdfsTableSink.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/planner/HdfsTableSink.java b/fe/src/main/java/org/apache/impala/planner/HdfsTableSink.java index 5ff8a1b..fed4ffd 100644 --- a/fe/src/main/java/org/apache/impala/planner/HdfsTableSink.java +++ b/fe/src/main/java/org/apache/impala/planner/HdfsTableSink.java @@ -95,8 +95,9 @@ public class HdfsTableSink extends TableSink { Math.max(1L, inputNode.getCardinality() / numInstances); long perInstanceInputBytes = (long) Math.ceil(perInstanceInputCardinality * inputNode.getAvgRowSize()); - perInstanceMemEstimate = - Math.min(perInstanceInputBytes, numPartitionsPerInstance * perPartitionMemReq); + long perInstanceMemReq = + PlanNode.checkedMultiply(numPartitionsPerInstance, perPartitionMemReq); + perInstanceMemEstimate = Math.min(perInstanceInputBytes, perInstanceMemReq); } resourceProfile_ = new ResourceProfile(perInstanceMemEstimate, 0); } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/227a180a/fe/src/main/java/org/apache/impala/planner/JoinNode.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/planner/JoinNode.java b/fe/src/main/java/org/apache/impala/planner/JoinNode.java index 3bd0899..0c983d9 100644 --- a/fe/src/main/java/org/apache/impala/planner/JoinNode.java +++ b/fe/src/main/java/org/apache/impala/planner/JoinNode.java @@ -419,7 +419,7 @@ public abstract class JoinNode extends PlanNode { long leftCard = getChild(0).cardinality_; long rightCard = getChild(1).cardinality_; if (leftCard != -1 && rightCard != -1) { - cardinality_ = multiplyCardinalities(leftCard, rightCard); + cardinality_ = checkedMultiply(leftCard, rightCard); } } @@ -453,7 +453,7 @@ public abstract class JoinNode extends PlanNode { } case FULL_OUTER_JOIN: { if (leftCard != -1 && rightCard != -1) { - long cardinalitySum = addCardinalities(leftCard, rightCard); + long cardinalitySum = checkedAdd(leftCard, rightCard); cardinality_ = Math.max(cardinalitySum, cardinality_); } break; @@ -475,7 +475,7 @@ public abstract class JoinNode extends PlanNode { if (getChild(0).cardinality_ == -1 || getChild(1).cardinality_ == -1) { cardinality_ = -1; } else { - cardinality_ = multiplyCardinalities(getChild(0).cardinality_, + cardinality_ = checkedMultiply(getChild(0).cardinality_, getChild(1).cardinality_); } break; http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/227a180a/fe/src/main/java/org/apache/impala/planner/PlanFragment.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/planner/PlanFragment.java b/fe/src/main/java/org/apache/impala/planner/PlanFragment.java index 3e89137..79c953a 100644 --- a/fe/src/main/java/org/apache/impala/planner/PlanFragment.java +++ b/fe/src/main/java/org/apache/impala/planner/PlanFragment.java @@ -265,7 +265,7 @@ public class PlanFragment extends TreeNode { if (dataPartition_.getPartitionExprs().contains(expr)) { numDistinct = (long)Math.max((double) numDistinct / (double) numInstances, 1L); } - result = PlanNode.multiplyCardinalities(result, numDistinct); + result = PlanNode.checkedMultiply(result, numDistinct); } return result; } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/227a180a/fe/src/main/java/org/apache/impala/planner/PlanNode.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/planner/PlanNode.java b/fe/src/main/java/org/apache/impala/planner/PlanNode.java index b6c3763..67d9e44 100644 --- a/fe/src/main/java/org/apache/impala/planner/PlanNode.java +++ b/fe/src/main/java/org/apache/impala/planner/PlanNode.java @@ -586,14 +586,14 @@ abstract public class PlanNode extends TreeNode { } /** - * Computes and returns the sum of two cardinalities. If an overflow occurs, + * Computes and returns the sum of two long values. If an overflow occurs, * the maximum Long value is returned (Long.MAX_VALUE). */ - public static long addCardinalities(long a, long b) { + public static long checkedAdd(long a, long b) { try { return LongMath.checkedAdd(a, b); } catch (ArithmeticException e) { - LOG.warn("overflow when adding cardinalities: " + a + ", " + b); + LOG.warn("overflow when adding longs: " + a + ", " + b); return Long.MAX_VALUE; } } @@ -602,11 +602,11 @@ abstract public class PlanNode extends TreeNode { * Computes and returns the product of two cardinalities. If an overflow * occurs, the maximum Long value is returned (Long.MAX_VALUE). */ - public static long multiplyCardinalities(long a, long b) { + public static long checkedMultiply(long a, long b) { try { return LongMath.checkedMultiply(a, b); } catch (ArithmeticException e) { - LOG.warn("overflow when multiplying cardinalities: " + a + ", " + b); + LOG.warn("overflow when multiplying longs: " + a + ", " + b); return Long.MAX_VALUE; } } @@ -635,7 +635,7 @@ abstract public class PlanNode extends TreeNode { for(PlanNode p : children_) { long tmp = p.getCardinality(); if (tmp == -1) return -1; - sum = addCardinalities(sum, tmp); + sum = checkedAdd(sum, tmp); } return sum; } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/227a180a/fe/src/main/java/org/apache/impala/planner/SubplanNode.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/planner/SubplanNode.java b/fe/src/main/java/org/apache/impala/planner/SubplanNode.java index cbe7087..169abc1 100644 --- a/fe/src/main/java/org/apache/impala/planner/SubplanNode.java +++ b/fe/src/main/java/org/apache/impala/planner/SubplanNode.java @@ -85,7 +85,7 @@ public class SubplanNode extends PlanNode { super.computeStats(analyzer); if (getChild(0).cardinality_ != -1 && getChild(1).cardinality_ != -1) { cardinality_ = - multiplyCardinalities(getChild(0).cardinality_, getChild(1).cardinality_); + checkedMultiply(getChild(0).cardinality_, getChild(1).cardinality_); } else { cardinality_ = -1; } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/227a180a/fe/src/main/java/org/apache/impala/planner/UnionNode.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/planner/UnionNode.java b/fe/src/main/java/org/apache/impala/planner/UnionNode.java index 6b8d331..2c72c63 100644 --- a/fe/src/main/java/org/apache/impala/planner/UnionNode.java +++ b/fe/src/main/java/org/apache/impala/planner/UnionNode.java @@ -115,7 +115,7 @@ public class UnionNode extends PlanNode { // ignore missing child cardinality info in the hope it won't matter enough // to change the planning outcome if (child.cardinality_ > 0) { - cardinality_ = addCardinalities(cardinality_, child.cardinality_); + cardinality_ = checkedAdd(cardinality_, child.cardinality_); } } // The number of nodes of a union node is -1 (invalid) if all the referenced tables