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.
Greetings,
Andres Freund
