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
