public inbox for [email protected]
help / color / mirror / Atom feedFrom: Greg Burd <[email protected]>
To: Michael Paquier <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: [PATCH] Add tests for Bitmapset
Date: Mon, 29 Sep 2025 06:27:01 -0400
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
On Sep 29 2025, at 2:20 am, Michael Paquier <[email protected]> wrote:
> On Sat, Sep 27, 2025 at 10:03:14AM -0400, Greg Burd wrote:
>>
>> On Sep 24 2025, at 7:28 pm, Michael Paquier <[email protected]> wrote:
>>
>> > On Wed, Sep 24, 2025 at 07:39:59AM -0400, Greg Burd wrote:
>> >> Thanks Michael, Tom, for the help getting this into shape and in
>> the tree.
>> >
>> > By the way, Greg, do you think that we should aim for a state where we
>> > are closer to completion?
>>
>> Hi Michael,
>>
>> We've come this far, why not go all the way? :)
>>
>> > We have the module up to the point where we
>> > are in pretty good shape, with most things and the infrastructure done
>> > but it can be improved a bit more, as well.
>>
>> Agreed.
>>
>> > Based on the information provided by the coverage report at
>> > https://coverage.postgresql.org/src/backend/nodes/bitmapset.c.gcov.html,
>> > we still have the following things:
>> > - bms_equal for different word counts
>> > - bms_union, bms_nonempty_difference, bms_is_subset and bms_intersect
>> > with shorter word counts.
>> > - bms_different with different word counts
>> > - A couple more cases with bms_subset_compare
>> > - bms_member_index and word counts
>> > - bms_overlap_list with negative number in input list.
>> > - bms_singleton_member ERROR with empty input.
>> > - bms_get_singleton_member with NULL input
>> > - bms_del_member with word counts
>> > - bms_replace_members and repalloc case
>> > - bms_add_range, bms_join and bms_del_members, more word count cases
>> > - bms_prev_member and the prevbit business
>>
>> Thanks for the detailed analysis, I tried to address each of these in
>> the patch.
>>
>> > There is not much we can do with the random function, still we could
>> > do something about the NULL paths in the internal functions:
>> > https://coverage.postgresql.org/src/test/modules/test_bitmapset/test_bitmapset.c.gcov.html
>>
>> Agreed.
>>
>> > The coverage of the latter matters less than the coverage of the
>> > former, of course.
>>
>> Agreed.
>> test_bitmapset.c
>> lines......: 98.9%
>> functions..: 100%
>> branches...: 82.6%
>>
>> Here I removed a bit of code I thought was unnecessary and added a few
>> test cases. The coverage is better overall, not much to say about it.
>>
>>
>> bitmapset.c
>> lines......: 99.8%
>> functions..: 100%
>> branches...: 89.4%
Hello Michael,
> No need for three tests for the word count case in bms_del_member(),
> only one is enough, but I've let them as they you have proposed, as
> you wanted to check more positioning with the members. These are
> cheap, that's fine.
Yes, redundant. Thanks for leaving them.
> Added a comment for bms_del_members() for the two cases that trigger
> word count changes.
Thanks, I considered adding comments to cases that were more subtle but
didn't find the time to review/add them. I appreciate that you added that.
> The test changed with test_bms_difference() is indeed long, but not
> that bad with 140-ish characters, so left it as-is.
Thanks.
> Good catch with bms_del_members() that was missing. There are so many
> APIs that it's easy to miss one.
Indeed, but it's covered now.
> The changes in test_bms_get_singleton_member() don't seem adapted,
> because we also want to test if the default value given by the
> function caller is changed, bms_get_singleton_member() returning a
> boolean status. The patch did not use arg1 at all. Also, if arg0 is
> NULL, let's return the input value.
Doh, I see what you mean. Thanks for the adaptation.
>> A few branches were really tricky, my (least) favorite was the
>> bms_subset_compare(). To get to the first BMS_DIFFERENT return you have
>> to have a test case where the shorter bitmap is long enough to force two
>> or more iterations of the loop and in the first iteration the first word
>> in a needs to be a subset of b and then for the second word the opposite
>> where the second word of b is a subset of a and yet there can be no
>> overlapping bits in the two sets and the same number of words in both sets.
>>
>> test_bms_subset_compare('(b 1 64 65)', '(b 1 2 64)') -> BMS_DIFFERENT
>>
>> Technically the '(b 64 200)', '(b 1 201)' test case is also
>> BMS_DIFFERENT and so redundant, but I was so happy to find the test case
>> for this branch I kept them both.
>
> I can see that you have expanded on these quite a bit, impressive.
> One thing that was confusing is that some of the tests already
> existed, so I've slightly reworded things to reduce the diffs.
Excellent, thanks.
> Per my
> measurements, we are at more than 99% in the module and bitmapset.c
> when only running make check in the module, which is nice
Yes, I think that does it. Thanks so much for the guidance and help
taking this to its logical end. I greatly appreciate your guidance and time.
> --
> Michael
best.
-greg
view thread (81+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: [PATCH] Add tests for Bitmapset
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox