On Fri, Nov 22, 2019 at 4:57 PM Michael Paquier <michael@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
