On Mon, Oct 12, 2020 at 4:37 PM Amit Langote <amitlangot...@gmail.com>
wrote:
> Hi,
>
> On Thu, Oct 8, 2020 at 6:56 PM Andy Fan <zhihui.fan1...@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!
--
Best Regards
Andy Fan
