MIME-Version: 1.0
References: <aNspd6syEndiEPNj@paquier.xyz>
 <48833E2B-C6E4-4C3D-A002-87234AF9782F@greg.burd.me>
In-Reply-To: <48833E2B-C6E4-4C3D-A002-87234AF9782F@greg.burd.me>
From: David Rowley <dgrowleyml@gmail.com>
Date: Tue, 30 Sep 2025 23:29:36 +1300
Message-ID: 
 <CAApHDvq-4994jpDnspQ+bgSbzo0+=Z4YQbj+YfP7wi97B0e1qg@mail.gmail.com>
Subject: Re: [PATCH] Add tests for Bitmapset
To: Greg Burd <greg@burd.me>
Cc: Michael Paquier <michael@paquier.xyz>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CAApHDvq-4994jpDnspQ%2BbgSbzo0%2B%3DZ4YQbj%2BYfP7wi97B0e1qg%40mail.gmail.com>
Precedence: bulk

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