committed
On 22.04.25 10:47, Ashutosh Bapat wrote:
Thanks Jian for your review.
On Tue, Apr 22, 2025 at 12:32 PM jian he <[email protected]
<mailto:[email protected]>> wrote:
On Tue, Apr 22, 2025 at 11:45 AM Ashutosh Bapat
<[email protected] <mailto:[email protected]>>
wrote:
>
> I have included your original tests, but ended up rewriting code.
Please let me know what do you think.
>
+ if (attno < 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("partition key expressions cannot contain system column
references")));
+
+ /*
+ * We do not check dropped columns explicitly since they will
+ * be eliminated when expanding the the whole row expression
+ * anyway.
+ */
typo: "the the".
I am confused by the above comments.
ComputePartitionAttrs only called in DefineRelation.
DefineRelation will only CREATE a table, there will be no dropped
column via DefineRelation.
You are right! Fixed.
+ /*
+ * transformPartitionSpec() should have already rejected
+ * subqueries, aggregates, window functions, and SRFs, based
+ * on the EXPR_KIND_ for partition expressions.
+ */
"EXPR_KIND_" can change to EXPR_KIND_PARTITION_EXPRESSION
?
That's an existing comment which appears to be moved in diff but it's
actually untouched by the patch. Maybe you are right, IDK, but since
it's not related to the fix, let's leave it untouched. I did wonder
whether that comment has any relation to the pull_varattnos call which
has been moved earlier since pull_varattnos doesn't expect any Query
node in the tree. But pondering more, I think the comment is related to
the number of rows participating in the partition key computation -
there should be exactly one key for one tuple. Having SRFs, subqueries
in part expression means a possibility of producing more than one set of
partition keys, aggregates and window functions OTOH will produce one
partition key for more than one row - all of them breaking 1:1 mapping
between a tuple and a partition key. Hence I think the comment is at the
right place.
PFA revised patch.
--
Best Wishes,
Ashutosh Bapat
