On Fri, Nov 22, 2019 at 4:57 PM Michael Paquier <mich...@paquier.xyz> wrote: > On Fri, Nov 22, 2019 at 02:21:41PM +0900, Amit Langote wrote: > > Actually, we should also refactor > > convert_tuples_by_position() to carve out the code that builds the > > AttrMap into a separate function and move it to attmap.c. > > Not sure how to name that. One logic uses a match based on the > attribute name, and the other uses the type. So the one based on the > attribute name should be something like build_attrmap_by_name() and > the second attrmap_build_by_position()? We could use a better > convention like AttrMapBuildByPosition for example. Any suggestions > of names are welcome.
Actually, I was just suggesting that we create a new function convert_tuples_by_position_map() and put the logic that compares the two TupleDescs to create the AttrMap in it, just like convert_tuples_by_name_map(). Now you could say that there would be no point in having such a function, because no caller currently wants to use such a map (that is, without the accompanying TupleConversionMap), but maybe there will be in the future. Though irrespective of that consideration, I guess you'd agree to group similar code in a single source file. Regarding coming up with any new name, having a word in the name that distinguishes the aspect of mapping by attribute name vs. type (position) should suffice. We can always do the renaming in a separate patch. > Please note that I still have a commit fest to > run and finish, so I'll unlikely come back to that before December. > Let's continue with the tuning of the function names though. As it's mainly just moving around code, I gave it a shot; patch attached (applies on top of yours). I haven't invented any new names yet, but let's keep discussing that as you say. Thanks, Amit
0002-Move-more-code-to-attmap.c.patch
Description: Binary data
