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
