On Mon, Nov 25, 2019 at 05:55:50PM +0900, Amit Langote wrote:
> 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.
Hmm. I would rather keep the attribute map generation and the tuple
conversion part, the latter depending on the former, into two
different files. That's what I did in the v2 attached.
> 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.
I see. That saved me some time, thanks. It is not really intuitive
to name routines about tuple conversion from tupconvert.c to
attrmap.c, so I'd think about renaming those routines as well, like
build_attrmap_by_name/position instead. That's more consistent with
the rest I added.
Another thing is that we have duplicated code at the end of
build_attrmap_by_name_if_req and build_attrmap_by_position, which I
think would be better refactored as a static function part of
attmap.c. This way the if_req() flavor gets much simpler.
I have also fixed the issue with the FK mapping in
addFkRecurseReferencing() you reported previously.
What do you think about that? I would like to think that we are
getting at something rather committable here, though I feel that I
need to review more the comments around the new files and if we could
document better AttrMap and its properties.
--
Michael