On Sat, 1 Mar 2025 at 23:07, Yuya Watari <watari.y...@gmail.com> wrote: > The previous patches did not apply to the current master, so I have > rebased them.
Thank you for continuing to work on this. My apologies for having completely disappeared from this thread for so long. Looking at v33-0001, there are a few choices you've made that are not clear to me: 1) Can you describe the difference between PlannerInfo.top_parent_relid_array and RelOptInfo.top_parent_relids? If you've added the PlannerInfo field for performance reasons, then that needs to be documented. I think the bar for adding another field to do the same thing should be quite high. The RelOptInfo.top_parent_relids field already is commented with "redundant, but handy", so having another field in another struct that's also redundant leads me to think that some design needs more thought. If you need a cheap way to take the same shortcut as you're doing in setup_eclass_child_member_iterator() with "if (root->top_parent_relid_array == NULL)", then maybe PlannerInfo should have a boolean field to record if there are any other member rels 2) I think the naming of setup_eclass_child_member_iterator() and dispose_eclass_child_member_iterator() is confusing. From the names, I'd expect these to only be returning em_is_child == true members, but that's not the case. 3) The header comment for setup_eclass_child_member_iterator() does not seem concise enough. It claims "so that it can iterate over EquivalenceMembers in 'ec'.", but what does that mean? The definition of "EquivalenceMembers in 'ec'" isn't clear. Is that just the class's ec_members, or also the child members that are stored somewhere else. Users of this function need to know what they'll get so they know which members they need to ignore or which they can assume won't be returned. If you don't document that, then it's quite hard to determine where the faulty code is when we get bugs. The "relids" parameter needs to be documented too. 4) add_transformed_child_version sounds like it does some transformation, but all it does is add the EMs for the given RelOptInfo to the iterator's list. I don't quite follow what's being "transformed". Maybe there's a better name? That's all I have for now. David
