impala-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From jruss...@apache.org
Subject [3/3] incubator-impala git commit: IMPALA-5282: Handle overflows in computeResourceProfile().
Date Tue, 06 Jun 2017 18:41:32 GMT
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 <alex.behm@cloudera.com>
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 <alex.behm@cloudera.com>
Authored: Mon Jun 5 10:18:29 2017 -0700
Committer: Impala Public Jenkins <impala-public-jenkins@gerrit.cloudera.org>
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


Mime
View raw message