Fujita-san,
Thanks for the review.
On 2018/01/25 21:17, Etsuro Fujita wrote:
> Thanks for the updated patch! Some minor comments:
>
> + /*
> + * Construct an ArrayExpr for the non-null partition
> + * values
> + */
> + arrexpr = makeNode(ArrayExpr);
> + arrexpr->array_typeid =
> + !type_is_array(key->parttypid[0])
> + ? get_array_type(key->parttypid[0])
> + : key->parttypid[0];
>
> We test the type_is_array() above in this bit, so I don't think we need to
> test that again here.
Ah, you're right. Fixed.
> + arrexpr->array_collid = key->parttypcoll[0];
> + arrexpr->element_typeid = key->parttypid[0];
>
> We can assume that keynum=0 here, so it would be okay to specify zero as
> the offset. But ISTM that that makes code a bit less readable, so I'd
> vote for using keynum as the offset and maybe adding an assertion that
> keynum should be zero, somewhere in the PARTITION_STRATEGY_LIST block.
Agreed, done.
>
> * Both comments in the following in get_qual_for_list needs to be updated,
> because the expression made there isn't necessarily = ANY anymore.
>
> if (elems)
> {
> /* Generate the main expression, i.e., keyCol = ANY (arr) */
> opexpr = make_partition_op_expr(key, 0, BTEqualStrategyNumber,
> keyCol, (Expr *) elems);
> }
> else
> {
> /* If there are no partition values, we don't need an = ANY expr */
> opexpr = NULL;
> }
Fixed those.
Attached updated patch. Thanks again.
Regards,
Amit