On 2018-10-02 11:02:37 -0700, Andres Freund wrote:
> On 2018-10-02 18:35:29 +0900, Amit Langote wrote:
> > Hi,
> >
> > I looked at the patch. Some comments.
> >
> > On 2018/10/02 16:35, Andres Freund wrote:
> > > I wasn't quite happy yet with that patch.
> > >
> > > - ConvertTupleSlot seems like a too generic name, it's very unclear it's
> > > related to tuple mapping, rather than something internal to slots. I
> > > went for execute_attr_map_slot (and renamed do_convert_tuple to
> > > execute_attr_map_tuple, to match).
> > >
> > > I'd welcome a better name.
> >
> > do_convert_tuple -> convert_tuple_using_map
> > ConvertTuplSlot -> convert_tuple_using_map_slot
>
> I think my name is more descriptive, referencing the attr map seems
> better to me.
>
>
> > > - I disliked inheritence_tupconv_map, it doesn't seem very clear why
> > > this is named inheritence_* (typo aside). I went for
> > > convert_tuples_by_name_map_if_req() - while I think this sounds
> > > too much like it converts tuples itself it should be renamed with the
> > > rest of the convert_tuples_by_* routines.
> > >
> > > I'd welcome a better name.
> >
> > Agree about doing something about the convert_* names. A comment near the
> > beginning of tupconvert.c says all of these convert_* routines are meant
> > as conversion "setup" routines:
>
> I'll try to tackle that in a separate commit.
>
>
> > /*
> > * The conversion setup routines have the following common API:
> >
> > So, maybe:
> >
> > convert_tuples_by_position -> get_conversion_map_by_position
> > convert_tuples_by_name -> get_conversion_map_by_name
> > convert_tuples_by_name_map -> get_attr_map_for_conversion
> > convert_tuples_by_name_map_if_req -> get_attr_map_for_conversion_if_req
>
> s/get/build/? I'm also not a fan of the separation of attr and
> conversion map to signal the difference between tuples and slots being
> converted...
>
>
> > I noticed the comment at the top of tupconvert.c requires updating:
> >
> > * of dropped columns. There is some overlap of functionality with the
> > * executor's "junkfilter" routines, but these functions work on bare
> > * HeapTuples rather than TupleTableSlots.
> >
> > Now we have functions that manipulate TupleTableSlots.
>
> Heh. I think I'll just drop the entire sentence - I don't really think
> there's much of an overlap to junkfilters these days.
I've pushed this now. We can discuss about the other renaming and then
I'll commit that separately.
Greetings,
Andres Freund
