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 <[email protected]> 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 <[email protected]> Authored: Mon Jun 5 10:18:29 2017 -0700 Committer: Impala Public Jenkins <[email protected]> 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<PlanFragment> { 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<PlanNode> { } /** - * 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<PlanNode> { * 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<PlanNode> { 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
