From 30fdb59a80396d4e112ff1e1044b262eeac61c8b Mon Sep 17 00:00:00 2001 From: Jason Petersen Date: Mon, 11 Apr 2016 16:25:45 -0600 Subject: [PATCH] Add clarifying comment in HashableClauseMutator While reading this code last week, it appeared as though there was no place we ensured that the partition clause actually used equality ops. As such, I was worried that we might transform a clause such as id < 5 into a constraint like hash(id) = hash(5) when doing shard pruning. The relevant code seemed to just ensure: 1. The node is an OpExpr 2. With a related hash function 3. It compares the partition column 4. Against a constant A superficial reading implied we didn't actually make sure the original op was equality-related, but it turns out the hash lookup function DOES ensure that for us. So I added a comment. --- src/backend/distributed/planner/multi_physical_planner.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/backend/distributed/planner/multi_physical_planner.c b/src/backend/distributed/planner/multi_physical_planner.c index c9525c5dd..c6435015b 100644 --- a/src/backend/distributed/planner/multi_physical_planner.c +++ b/src/backend/distributed/planner/multi_physical_planner.c @@ -2858,6 +2858,13 @@ HashableClauseMutator(Node *originalNode, Var *partitionColumn) Oid leftHashFunction = InvalidOid; Oid rightHashFunction = InvalidOid; + + /* + * If operatorExpression->opno is NOT the registered '=' operator for + * any hash opfamilies, then get_op_hash_functions will return false. + * This means this function both ensures a hash function exists for the + * types in question AND filters out any clauses lacking equality ops. + */ bool hasHashFunction = get_op_hash_functions(operatorExpression->opno, &leftHashFunction, &rightHashFunction);