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.
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.
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.
David
