Re: Wired if-statement in gen_partprune_steps_internal

At Wed, 14 Oct 2020 11:26:33 +0800, Andy Fan <zhihui(dot)fan1213(at)gmail(dot)com> wrote in
> On Mon, Oct 12, 2020 at 4:37 PM Amit Langote <amitlangote09(at)gmail(dot)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(at)master(dot)
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