Re: [HACKERS] PATCH: multivariate histograms and MCV lists

At Wed, 13 Mar 2019 19:37:45 +1300, David Rowley <david.rowley@2ndquadrant.com> wrote in
<CAKJS1f_6qDQj9m2H0jF4bRkZVLpfc7O9E+MxdXrq0wgv0z1NrQ@mail.gmail.com>
> On Wed, 13 Mar 2019 at 17:20, Kyotaro HORIGUCHI
> <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> > bms_member_index seems working differently than maybe expected.
> >
> >  bms_member_index((2, 4), 0) => 0, (I think) should be -1
> >  bms_member_index((2, 4), 1) => 0, should be -1
> >  bms_member_index((2, 4), 2) => 0, should be 0
> >  bms_member_index((2, 4), 3) => 1, should be -1
> >  bms_member_index((2, 4), 4) => 1, should be 1
> >  bms_member_index((2, 4), 5) => 2, should be -1
> >  bms_member_index((2, 4), 6) => 2, should be -1
> > ...
> >  bms_member_index((2, 4), 63) => 2, should be -1
> >  bms_member_index((2, 4), 64) => -1, correct
> >
> > It works correctly only when x is a member - the way the function
> > is maybe actually used in this patch -, or needs to change the
> > specifiction (or the comment) of the function.
> 
> Looks like:
> 
> + if (wordnum >= a->nwords)
> + return -1;
> 
> should be:
> 
> + if (wordnum >= a->nwords ||
> + (a->word[wordnum] & ((bitmapword) 1 << bitnum)) == 0)
> + return -1;

Yeah, seems right.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center