On Wed, Apr 2, 2025 at 10:26 PM Richard Guo <guofengli...@gmail.com> wrote: > This is not a thorough review of the code; I just randomly looked at > ChangeVarNodes(). It's possible that there are other issues that > haven't been discovered, but I think we should at least fix these. > I'll provide a patch for the fixes later. Apologies for being so > nitpicky.
With more exposure to the changes in ChangeVarNodes(), I have some more concerns. As I understand it, ChangeVarNodes() is designed to update the varno fields of Var nodes in the given tree that belong to the specific relation; neat and clear. However, now, within this function, we also create new NullTest quals for SJE, which doesn't seem like something this function should be handling. It makes this function look a bit like a kludge. Actually, while working on the reduce-NullTest-during-constant-folding patch, I tried to figure out where the new NOT NULL quals from SJE are added. I was a bit surprised to find that they are added in ChangeVarNodes. I didn't expect this function to have this extra responsibility. Additionally, I have a minor suggestion regarding the new boolean parameter, "change_RangeTblRef". AFAIU, by convention, we typically use flags to control the behavior of walker functions, like in pull_var_clause. While it's fine to use a boolean parameter here given that we currently only have one flag, for the sake of future extensibility, I think using flags might be a better choice. Any ideas on how we could improve this function? Apologies again for being nitpicky. Thanks Richard
