Re: [PATCH] Add tests for Bitmapset

On Sep 30 2025, at 6:29 am, David Rowley <dgrowleyml@gmail.com> wrote:

> On Tue, 30 Sept 2025 at 22:30, Greg Burd <greg@burd.me> wrote:
>> Thank you both, patch attached.
> 
> Patch looks good to me.
> 
> Just while reviewing, I was confused at the following code in
> test_bms_add_range().
> 
>    /* Check for invalid range */
>    if (upper < lower)
>    {
>        bms_free(bms);
>        PG_RETURN_NULL();
>    }
> 
> That seems wrong. You should just return the input set in that case,
> not NULL.

Agreed, I should have caught this inconsistency earlier on.

> This results in:
> 
> postgres=# select test_bms_add_range('(b 1)', 2, 5); -- ok.
> test_bms_add_range
> --------------------
> (b 1 2 3 4 5)
> (1 row)
> 
> postgres=# select test_bms_add_range('(b 1)', 5, 2); -- wrong results
> test_bms_add_range
> --------------------
> 
> (1 row)
> 
> I'd expect the last one to return '(b 1)', effectively the input set unmodified.
> 
> If I remove the code quoted above, I also see something else unexpected:
> 
> postgres=# select test_bms_add_range('(b)', 5, 2);
> test_bms_add_range
> --------------------
> <>
> (1 row)
> 
> Now, you might blame me for that as I see you've done things like:
> 
>    if (bms_is_empty(bms))
>       PG_RETURN_NULL();
> 
> in other places, but it's not very consistent as I can get the <> in
> other locations:
> 
> postgres=# select test_bms_add_members('(b)', '(b)');
> test_bms_add_members
> ----------------------
> <>
> (1 row)
> 
> It seems to me that you could save some code and all this
> inconsistency by just making <> the standard way to represent an empty
> Bitmapset. Or if you really want to keep the SQL NULLs, you could make
> a helper macro that handles that consistently.

nodeToString() will turn NULL into '<>' because it has no way to know
that we're referring to a NULL/empty Bitmapset, but that's not what we'd
ideally like to return in those cases because NULL Bitmapsets are in
fact a thing.  The standard/consistent representation for a NULL
Bitmapset encoded by nodeToString() is '(b)' so I'll update to be more
consistent across tests.

> For me, I think stripping as much logic out of the test functions as
> possible is a good way of doing things. I expect you really want to
> witness the behaviour of bitmapset.c, not some anomaly of
> test_bitmapset.c.

You are 100% correct, I've stripped away as much as possible so as to
push the responsibility for results down to the bitmapset.c code as it
should be.
 
> David

v2 attached.

best.

-greg