Hello.
I tried to read this and had some random comments on this.
-- general
I got some warning on compilation on unused variables and wrong
arguemtn type.
I failed to have a query that this patch works on. Could you let
me have some specific example for this patch?
This patch needs more comments. Please put comment about not only
what it does but also the reason and other things for it.
-- about namings
Names for functions and variables are needed to be more
appropriate, in other words, needed to be what properly informs
what they are. The followings are the examples of such names.
"added_restrictlist"'s widely distributed as many function
arguemnts and JoinPathExtraData makes me feel
dizzy.. create_mergejoin_path takes it as "filtering_clauses",
which looks far better.
try_join_pushdown() is also the name with much wider
meaning. This patch tries to move hashjoins on inheritance parent
to under append paths. It could be generically called 'pushdown'
but this would be better be called such like 'transform appended
hashjoin' or 'hashjoin distribution'. The latter would be better.
(The function name would be try_distribute_hashjoin for the
case.)
The name make_restrictinfos_from_check_contr() also tells me
wrong thing. For example,
extract_constraints_for_hashjoin_distribution() would inform me
about what it returns.
-- about what make_restrictinfos_from_check_constr() does
In make_restrictinfos_from_check_constr, the function returns
modified constraint predicates correspond to vars under
hashjoinable join clauses. I don't think expression_tree_mutator
is suitable to do that since it could allow unwanted result when
constraint predicates or join clauses are not simple OpExpr's.
Could you try more simple and straight-forward way to do that?
Otherwise could you give me clear explanation on what it does?
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
