Code review feedback

src/backend/distributed/planner/shard_pruning.c

@@ -174,7 +174,7 @@ static void AddPartitionKeyRestrictionToInstance(ClauseWalkerContext *context,
 												 OpExpr *opClause, Var *varClause,
 												 Const *constantClause);
 static Const * TransformPartitionRestrictionValue(Var *partitionColumn,
-												  Node *restrictionValue);
+												  Const *restrictionValue);
 static void AddSAOPartitionKeyRestrictionToInstance(ClauseWalkerContext *context,
 													ScalarArrayOpExpr *
 													arrayOperatorExpression);
@@ -724,15 +724,19 @@ AddPartitionKeyRestrictionToInstance(ClauseWalkerContext *context, OpExpr *opCla
 	ListCell *btreeInterpretationCell = NULL;
 	bool matchedOp = false;
 
-	/* we want our restriction value in terms of the type of the partition column */
+	/* only have extra work to do if const isn't same type as partition column */
-	constantClause = TransformPartitionRestrictionValue(partitionColumn,
+	if (constantClause->consttype != partitionColumn->vartype)
-														(Node *) constantClause);
-	if (constantClause == NULL)
 	{
-		/* couldn't coerce the value, so we note this as a restriction we don't grok */
+		/* we want our restriction value in terms of the type of the partition column */
-		prune->otherRestrictions = lappend(prune->otherRestrictions, opClause);
+		constantClause = TransformPartitionRestrictionValue(partitionColumn,
+															constantClause);
+		if (constantClause == NULL)
+		{
+			/* couldn't coerce value, so we note this as a restriction we don't grok */
+			prune->otherRestrictions = lappend(prune->otherRestrictions, opClause);
 
-		return;
+			return;
+		}
 	}
 
 	/* at this point, we'd better be able to pass binary Datums to comparison functions */
@@ -860,10 +864,10 @@ AddPartitionKeyRestrictionToInstance(ClauseWalkerContext *context, OpExpr *opCla
  * we will simply fail to prune partitions based on this clause.
  */
 static Const *
-TransformPartitionRestrictionValue(Var *partitionColumn, Node *restrictionValue)
+TransformPartitionRestrictionValue(Var *partitionColumn, Const *restrictionValue)
 {
-	Node *transformedValue = coerce_to_target_type(NULL, restrictionValue,
+	Node *transformedValue = coerce_to_target_type(NULL, (Node *) restrictionValue,
-												   exprType(restrictionValue),
+												   restrictionValue->consttype,
 												   partitionColumn->vartype,
 												   partitionColumn->vartypmod,
 												   COERCION_ASSIGNMENT,

src/test/regress/expected/multi_prune_shard_list.out

@@ -216,14 +216,38 @@ SELECT create_distributed_table('coerce_hash', 'id');
  
 (1 row)
 
+INSERT INTO coerce_hash VALUES (1, 'test value');
+-- All three of the following should return the same results...
+-- SELECT with same type as partition column
+SELECT * FROM coerce_hash WHERE id = 1::bigint;
+ id |   value    
+----+------------
+  1 | test value
+(1 row)
+
+-- SELECT with similar type to partition column
+SELECT * FROM coerce_hash WHERE id = 1;
+ id |   value    
+----+------------
+  1 | test value
+(1 row)
+
+-- SELECT with numeric type (original impetus for this change)
 -- PostgreSQL will cast id to numeric rather than 1.0 to bigint...
 -- We used to blindly pass the RHS' Datum to our comparison func.,
 -- resulting in inaccurate pruning. An Assert now verifies type-
 -- compatibility; the following would crash the server in an Assert-
 -- before the underlying issue was addressed. It looks like a boring
 -- test now, but if the old behavior is restored, it should crash again.
-SELECT * FROM coerce_hash WHERE id = 1.0::numeric;
+SELECT * FROM coerce_hash WHERE id = 1.0;
- id | value 
+ id |   value    
-----+-------
+----+------------
-(0 rows)
+  1 | test value
+(1 row)
+
+SELECT * FROM coerce_hash WHERE id = 1.0::numeric;
+ id |   value    
+----+------------
+  1 | test value
+(1 row)
 

src/test/regress/sql/multi_prune_shard_list.sql

@@ -130,10 +130,24 @@ CREATE TABLE coerce_hash (
 );
 SELECT create_distributed_table('coerce_hash', 'id');
 
+INSERT INTO coerce_hash VALUES (1, 'test value');
+
+-- All three of the following should return the same results...
+
+-- SELECT with same type as partition column
+SELECT * FROM coerce_hash WHERE id = 1::bigint;
+
+-- SELECT with similar type to partition column
+SELECT * FROM coerce_hash WHERE id = 1;
+
+-- SELECT with numeric type (original impetus for this change)
+
 -- PostgreSQL will cast id to numeric rather than 1.0 to bigint...
 -- We used to blindly pass the RHS' Datum to our comparison func.,
 -- resulting in inaccurate pruning. An Assert now verifies type-
 -- compatibility; the following would crash the server in an Assert-
 -- before the underlying issue was addressed. It looks like a boring
 -- test now, but if the old behavior is restored, it should crash again.
+SELECT * FROM coerce_hash WHERE id = 1.0;
+
 SELECT * FROM coerce_hash WHERE id = 1.0::numeric;