Re: Wired if-statement in gen_partprune_steps_internal

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