From 4c7f78bd7eee322913c88a369adbf373bee505d3 Mon Sep 17 00:00:00 2001 From: Jason Petersen Date: Mon, 25 Mar 2019 22:07:27 -0500 Subject: [PATCH] Code review feedback --- .../distributed/planner/shard_pruning.c | 26 ++++++++------- .../expected/multi_prune_shard_list.out | 32 ++++++++++++++++--- .../regress/sql/multi_prune_shard_list.sql | 14 ++++++++ 3 files changed, 57 insertions(+), 15 deletions(-) diff --git a/src/backend/distributed/planner/shard_pruning.c b/src/backend/distributed/planner/shard_pruning.c index 89d6346c1..328d1618c 100644 --- a/src/backend/distributed/planner/shard_pruning.c +++ b/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 */ - constantClause = TransformPartitionRestrictionValue(partitionColumn, - (Node *) constantClause); - if (constantClause == NULL) + /* only have extra work to do if const isn't same type as partition column */ + if (constantClause->consttype != partitionColumn->vartype) { - /* couldn't coerce the value, so we note this as a restriction we don't grok */ - prune->otherRestrictions = lappend(prune->otherRestrictions, opClause); + /* we want our restriction value in terms of the type of the partition column */ + 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, - exprType(restrictionValue), + Node *transformedValue = coerce_to_target_type(NULL, (Node *) restrictionValue, + restrictionValue->consttype, partitionColumn->vartype, partitionColumn->vartypmod, COERCION_ASSIGNMENT, diff --git a/src/test/regress/expected/multi_prune_shard_list.out b/src/test/regress/expected/multi_prune_shard_list.out index 2bdf12640..0d62956a9 100644 --- a/src/test/regress/expected/multi_prune_shard_list.out +++ b/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; - id | value -----+------- -(0 rows) +SELECT * FROM coerce_hash WHERE id = 1.0; + id | value +----+------------ + 1 | test value +(1 row) + +SELECT * FROM coerce_hash WHERE id = 1.0::numeric; + id | value +----+------------ + 1 | test value +(1 row) diff --git a/src/test/regress/sql/multi_prune_shard_list.sql b/src/test/regress/sql/multi_prune_shard_list.sql index 89c73d0db..daf648621 100644 --- a/src/test/regress/sql/multi_prune_shard_list.sql +++ b/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;