On 2018/08/22 21:30, David Rowley wrote:
> On 22 August 2018 at 19:08, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> +#define PartitionTupRoutingGetToParentMap(p, i) \
>> +#define PartitionTupRoutingGetToChildMap(p, i) \
>>
>> If the "Get" could be replaced by "Child" and "Parent", respectively,
>> they'd sound more meaningful, imho.
>
> I did that to save 3 chars. I think putting the additional
> Child/Parent in the name is not really required. It's not as if we're
> going to have a ParentToParent or a ChildToChild map, so I thought it
> might be okay to assume that if it's "ToParent", that it's being
> converted from the child and "ToChild" seems safe to assume it's being
> converted from the parent. I can change it though if you feel very
> strongly that what I've got is no good.
No strong preference as such. Maybe, let's defer to committer.
>> I've looked at v6 and spotted some minor typos.
>>
>> + * ResultRelInfo for, before we go making one, we check for a
>> pre-made one
>>
>> s/making/make/g
>
> I disagree, but perhaps we can just reword it a bit. I've now got:
>
> + * Every time a tuple is routed to a partition that we've yet to set the
> + * ResultRelInfo for, before we go to the trouble of making one, we check
> + * for a pre-made one in the hash table.
Sure. I guess "to the trouble of" was missing then. :)
>> + /* If nobody else set the per-subplan array of maps, do so ouselves. */
>>
>> I guess I'm the one to blame here for misspelling "ourselves".
>
> Thanks for noticing.
>
>> Since the above two are minor issues, fixed them myself in the attached
>> updated version; didn't touch the macro though.
>
> I've attached a v8. The only change from your v7 is in the "go making" comment.
Thanks.
>> Do you agree to setting this patch to "Ready for Committer" in the
>> September CF?
>
> I read through the entire patch a couple of times yesterday and saw
> nothing else, so yeah, I think now is a good time for someone with
> more authority to have a look at it.
Okay, doing it now.
Thanks,
Amit
