Merge pull request #1369 from citusdata/featurefix/better-range-pruning

Improve / Fix range pruning

src/backend/distributed/planner/shard_pruning.c

@@ -186,6 +186,9 @@ static List * PruneWithBoundaries(DistTableCacheEntry *cacheEntry,
 static List * ExhaustivePrune(DistTableCacheEntry *cacheEntry,
 							  ClauseWalkerContext *context,
 							  PruningInstance *prune);
+static bool ExhaustivePruneOne(ShardInterval *curInterval,
+							   ClauseWalkerContext *context,
+							   PruningInstance *prune);
 static int UpperShardBoundary(Datum partitionColumnValue,
 							  ShardInterval **shardIntervalCache,
 							  int shardCount, FunctionCallInfoData *compareFunction,
@@ -894,15 +897,27 @@ PruneOne(DistTableCacheEntry *cacheEntry, ClauseWalkerContext *context,
 	}
 
 	/*
-	 * If previous pruning method yielded a single shard, we could also
+	 * If previous pruning method yielded a single shard, and the table is not
-	 * attempt range based pruning to exclude it further.  But that seems
+	 * hash partitioned, attempt range based pruning to exclude it further.
-	 * rarely useful in practice, and thus likely a waste of runtime and code
+	 *
-	 * complexity.
+	 * That's particularly important in particular for subquery pushdown,
+	 * where it's very common to have a user specified equality restriction,
+	 * and a range based restriction for shard boundaries, added by the
+	 * subquery machinery.
 	 */
 	if (shardInterval)
 	{
+		if (context->partitionMethod != DISTRIBUTE_BY_HASH &&
+			ExhaustivePruneOne(shardInterval, context, prune))
+		{
+			return NIL;
+		}
+		else
+		{
+			/* no chance to prune further, return */
 			return list_make1(shardInterval);
 		}
+	}
 
 	/*
 	 * Should never get here for hashing, we've filtered down to either zero
@@ -1157,11 +1172,21 @@ PruneWithBoundaries(DistTableCacheEntry *cacheEntry, ClauseWalkerContext *contex
 		hasLowerBound = true;
 	}
 	if (prune->greaterConsts)
+	{
+		/*
+		 * Use the more restrictive one, if both greater and greaterEqual
+		 * constraints are specified.
+		 */
+		if (!hasLowerBound ||
+			PerformValueCompare(compareFunctionCall,
+								prune->greaterConsts->constvalue,
+								lowerBound) >= 0)
 		{
 			lowerBound = prune->greaterConsts->constvalue;
 			lowerBoundInclusive = false;
 			hasLowerBound = true;
 		}
+	}
 	if (prune->lessEqualConsts)
 	{
 		upperBound = prune->lessEqualConsts->constvalue;
@@ -1169,11 +1194,21 @@ PruneWithBoundaries(DistTableCacheEntry *cacheEntry, ClauseWalkerContext *contex
 		hasUpperBound = true;
 	}
 	if (prune->lessConsts)
+	{
+		/*
+		 * Use the more restrictive one, if both less and lessEqual
+		 * constraints are specified.
+		 */
+		if (!hasUpperBound ||
+			PerformValueCompare(compareFunctionCall,
+								prune->lessConsts->constvalue,
+								upperBound) <= 0)
 		{
 			upperBound = prune->lessConsts->constvalue;
 			upperBoundInclusive = false;
 			hasUpperBound = true;
 		}
+	}
 
 	Assert(hasLowerBound || hasUpperBound);
 
@@ -1232,21 +1267,40 @@ ExhaustivePrune(DistTableCacheEntry *cacheEntry, ClauseWalkerContext *context,
 				PruningInstance *prune)
 {
 	List *remainingShardList = NIL;
-	FunctionCallInfo compareFunctionCall = &context->compareIntervalFunctionCall;
 	int shardCount = cacheEntry->shardIntervalArrayLength;
 	ShardInterval **sortedShardIntervalArray = cacheEntry->sortedShardIntervalArray;
 	int curIdx = 0;
 
 	for (curIdx = 0; curIdx < shardCount; curIdx++)
 	{
-		Datum compareWith = 0;
 		ShardInterval *curInterval = sortedShardIntervalArray[curIdx];
 
+		if (!ExhaustivePruneOne(curInterval, context, prune))
+		{
+			remainingShardList = lappend(remainingShardList, curInterval);
+		}
+	}
+
+	return remainingShardList;
+}
+
+
+/*
+ * ExhaustivePruneOne returns true if curInterval is pruned away, false
+ * otherwise.
+ */
+static bool
+ExhaustivePruneOne(ShardInterval *curInterval,
+				   ClauseWalkerContext *context,
+				   PruningInstance *prune)
+{
+	FunctionCallInfo compareFunctionCall = &context->compareIntervalFunctionCall;
+	Datum compareWith = 0;
+
 	/* NULL boundaries can't be compared to */
 	if (!curInterval->minValueExists || !curInterval->maxValueExists)
 	{
-			remainingShardList = lappend(remainingShardList, curInterval);
+		return false;
-			continue;
 	}
 
 	if (prune->equalConsts)
@@ -1257,14 +1311,14 @@ ExhaustivePrune(DistTableCacheEntry *cacheEntry, ClauseWalkerContext *context,
 								compareWith,
 								curInterval->minValue) < 0)
 		{
-				continue;
+			return true;
 		}
 
 		if (PerformValueCompare(compareFunctionCall,
 								compareWith,
 								curInterval->maxValue) > 0)
 		{
-				continue;
+			return true;
 		}
 	}
 	if (prune->greaterEqualConsts)
@@ -1275,7 +1329,7 @@ ExhaustivePrune(DistTableCacheEntry *cacheEntry, ClauseWalkerContext *context,
 								curInterval->maxValue,
 								compareWith) < 0)
 		{
-				continue;
+			return true;
 		}
 	}
 	if (prune->greaterConsts)
@@ -1286,7 +1340,7 @@ ExhaustivePrune(DistTableCacheEntry *cacheEntry, ClauseWalkerContext *context,
 								curInterval->maxValue,
 								compareWith) <= 0)
 		{
-				continue;
+			return true;
 		}
 	}
 	if (prune->lessEqualConsts)
@@ -1297,7 +1351,7 @@ ExhaustivePrune(DistTableCacheEntry *cacheEntry, ClauseWalkerContext *context,
 								curInterval->minValue,
 								compareWith) > 0)
 		{
-				continue;
+			return true;
 		}
 	}
 	if (prune->lessConsts)
@@ -1308,12 +1362,9 @@ ExhaustivePrune(DistTableCacheEntry *cacheEntry, ClauseWalkerContext *context,
 								curInterval->minValue,
 								compareWith) >= 0)
 		{
-				continue;
+			return true;
 		}
 	}
 
-		remainingShardList = lappend(remainingShardList, curInterval);
+	return false;
-	}
-
-	return remainingShardList;
 }