At Wed, 14 Oct 2020 11:26:33 +0800, Andy Fan <zhihui.fan1...@gmail.com> wrote
in
> On Mon, Oct 12, 2020 at 4:37 PM Amit Langote <amitlangot...@gmail.com>
> wrote:
> > I think we should remove this duplicative logic and return the
> > generated steps in a list from this function, which the code in
> > gen_partprune_steps_internal() then "combines" using an INTERSECT
> > step. See attached a patch to show what I mean.
> >
> >
> This changes LGTM, and "make check" PASSED, thanks for the patch!
FWIW, both looks fine to me.
By the way, I guess that some of the caller sites of
gen_prune_step_combine(PARTPRUNE_COMBINE_INTERSECT) is useless if we
do that later?
(Diff1 below)
Mmm. I was wrong. *All the other caller site* than that at the end of
gen_partprune_steps_internal is useless?
(Note: The Diff1 alone leads to assertion failure at partprune.c:945@master.
See below.)
By the way, I'm confused to see the following portion in
gen_partprune_steps_internal.
> /*
> * Finally, results from all entries appearing in result should be
> * combined using an INTERSECT combine step, if more than one.
> */
> if (list_length(result) > 1)
...
> step = gen_prune_step_combine(context, step_ids,
>
> PARTPRUNE_COMBINE_INTERSECT);
> result = lappend(result, step);
The result contains both the source terms and the combined term. If I
understand it correctly, we should replace the source terms with
combined one. (With this change the assertion above doesn't fire and
passes all regression tests.)
=====
@@ -1180,13 +1163,9 @@ gen_partprune_steps_internal(GeneratePruningStepsContext
*context,
}
if (step_ids != NIL)
- {
- PartitionPruneStep *step;
-
- step = gen_prune_step_combine(context, step_ids,
-
PARTPRUNE_COMBINE_INTERSECT);
- result = lappend(result, step);
- }
+ result =
+ list_make1(gen_prune_step_combine(context,
step_ids,
+
PARTPRUNE_COMBINE_INTERSECT));
}
return result;
=====
regards.
Diff1
======
@@ -983,9 +983,7 @@ gen_partprune_steps_internal(GeneratePruningStepsContext
*context,
else if (is_andclause(clause))
{
List *args = ((BoolExpr *) clause)->args;
- List *argsteps,
- *arg_stepids = NIL;
- ListCell *lc1;
+ List *argsteps;
/*
* args may itself contain clauses of arbitrary
type, so just
@@ -998,21 +996,7 @@ gen_partprune_steps_internal(GeneratePruningStepsContext
*context,
if (context->contradictory)
return NIL;
- foreach(lc1, argsteps)
- {
- PartitionPruneStep *step = lfirst(lc1);
-
- arg_stepids = lappend_int(arg_stepids,
step->step_id);
- }
-
- if (arg_stepids != NIL)
- {
- PartitionPruneStep *step;
-
- step = gen_prune_step_combine(context,
arg_stepids,
-
PARTPRUNE_COMBINE_INTERSECT);
- result = lappend(result, step);
- }
+ result = list_concat(result, argsteps);
continue;
}
====
--
Kyotaro Horiguchi
NTT Open Source Software Center
