Re: Wired if-statement in gen_partprune_steps_internal

On Mon, Oct 12, 2020 at 4:37 PM Amit Langote <amitlangote09@gmail.com> wrote:

Hi,

On Thu, Oct 8, 2020 at 6:56 PM Andy Fan <zhihui.fan1213@gmail.com> wrote:
>
> Hi:
>
>   I found the following code in gen_partprune_steps_internal,  which
> looks the if-statement to be always true since list_length(results) > 1;
> I added an Assert(step_ids != NIL) and all the test cases passed.
> if the if-statement is always true,  shall we remove it to avoid confusion?
>
>
> gen_partprune_steps_internal(GeneratePruningStepsContext *context,
>
>
>         if (list_length(result) > 1)
>         {
>                 List       *step_ids = NIL;
>
>                 foreach(lc, result)
>                 {
>                         PartitionPruneStep *step = lfirst(lc);
>
>                         step_ids = lappend_int(step_ids, step->step_id);
>                 }
>                 Assert(step_ids != NIL);
>                 if (step_ids != NIL) // This should always be true.
>                 {
>                         PartitionPruneStep *step;
>
>                         step = gen_prune_step_combine(context, step_ids,
>                                                                                   PARTPRUNE_COMBINE_INTERSECT);
>                         result = lappend(result, step);
>                 }
>         }

That seems fine to me.

Looking at this piece of code, I remembered that exactly the same
piece of logic is also present in gen_prune_steps_from_opexps(), which
looks like this:

    /* Lastly, add a combine step to mutually AND these op steps, if needed */
    if (list_length(opsteps) > 1)
    {
        List       *opstep_ids = NIL;

        foreach(lc, opsteps)
        {
            PartitionPruneStep *step = lfirst(lc);

            opstep_ids = lappend_int(opstep_ids, step->step_id);
        }

        if (opstep_ids != NIL)
            return gen_prune_step_combine(context, opstep_ids,
                                          PARTPRUNE_COMBINE_INTERSECT);
        return NULL;
    }
    else if (opsteps != NIL)
        return linitial(opsteps);

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!