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;
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!