Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 22 Sep 2025 16:57:05 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Greg Burd <greg@burd.me>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
	Robert Haas <robertmhaas@gmail.com>,
	Nathan Bossart <nathandbossart@gmail.com>,
	Masahiko Sawada <sawada.mshk@gmail.com>
Subject: Re: [PATCH] Add tests for Bitmapset
Message-ID: <aNEBUeYqqSVOSVaP@paquier.xyz>
References: <aMz57nHgkLozddYu@paquier.xyz>
 <CD368B71-2947-407D-8967-181A4CB4A985@greg.burd.me>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="6IFE5lqezFA2FA3M"
Content-Disposition: inline
In-Reply-To: <CD368B71-2947-407D-8967-181A4CB4A985@greg.burd.me>
Archived-At: 
 <https://www.postgresql.org/message-id/aNEBUeYqqSVOSVaP%40paquier.xyz>
Precedence: bulk


--6IFE5lqezFA2FA3M
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Fri, Sep 19, 2025 at 02:48:43PM -0400, Greg Burd wrote:
> I think I've incorporated your feedback which did indeed make the patch
> more readable/crisp and maintainable, thank you.
>=20
> coverage:    HEAD    v7    v8
> lines......:  87.1  90.5  92.2

bitmap_match() had some duplicated tests.

bms_replace_members() was not covered, and a function was added to the
module.  I get up to 93.4% for the lines, once added.

+PG_FUNCTION_INFO_V1(test_bms_from_array);
+PG_FUNCTION_INFO_V1(test_bms_to_array);
These two were still declared, are not required.

There were a couple of functions that acted as dead code in the
module, covered in the tree.  As we are trying to cover the gap I
think that we should just do the whole thing.  Here are the functions:=20
- is_empty
- subset_compare
- get_singleton_member
- nonempty_difference
- member_index
- join.  The test C function had a mistake, because either input may
be modified or updated by bms_join().  The code was freeing the right
input, leading to errors in the node output function when tested.

Mixing the CTEs and the UNION ALL would have been nice if the queries
in the UNIONs relied on more than one input value, but none of them
did that, so I have removed this part and made the tests leaner.  That
feels easier for the eye.

Testing on equality for the hash functions should be OK, so I have
kept these, including the NULL/0 cases.

A few incorrect things related to the style, fixed on the way, like an
incorrect copyright notice in meson.build, the top of test_bitmapset.c
was largely incorrect.  It seems like these were copy-pasted from
elsewhere.

Another thing was testing with -DREALLOCATE_BITMAPSETS, which I've
done.  A couple of code paths related to the test C functions don't
test for NULL input, which is still OK as these don't impact the
internal tests or the backend coverage, so I've left these out.

The result I had was good enough, so applied.  The CI was OK, the
buildfarm may have a different opinion.
--
Michael

--6IFE5lqezFA2FA3M
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmjRAVEACgkQnvQgOdby
QH2u7g//cWSsJvIVS/JA4etZ4r7MB2D6gByyjpSsVtgEEKMnEmzlo0tUvP4YRZGB
Df4ZNiaPm5uBqxdrSBocNJ/amjnu0kNMH9J1zBIutNwYQun92HiMeQmS2mYjJdWR
BkKFACFED5XhmZuZtWX0zdBro1d0x0FeXKK80hxOrihB8dCP5MUNTxtCxBAB+4UP
x9zLY7mV5t8JqnMoaOhD2EiYLq/D6Q9zfczUm1464Zup01+V+xVIHKegLW6UbF87
FL9+WUnO9DxINGj+8QOaNc//kHD3TOI8avz9PoSWO0349jv0oKbBDgumskrR4IYF
b8mkwp43z3JQKOQsHfWnciEbbJCwDigCHTIt7q3YccxGUomhvZf0mcMikLx+GF4Y
Ep5GPbCDH4LFkDziYwMbi2BKSSITTOjrcSmMx/5yOLld8nz5IHFzPi0JYY6PWhV4
qC1th2gRIFa/tVE++TbEWQXyo2rYV9VfA319jdfChQN7PV3EatsMpACfNUMpryVI
ILhKw7StOcuqD8H/CAbfTcgDuDhAfuSj0icsb2HQLIxUMRzEgUUlp0/me7VBDie2
ZrUdBfG6ejWv7/Jd2M35g62NbbShoPmXy0iCWWJ1isQ+8Q8Q/Y+MXg2nSMLMm1r7
cjzfL/1rPRP+4VxDHGb4C0UP53UkuhhfsHv7297V5q8x4sC+Bjw=
=LcKT
-----END PGP SIGNATURE-----

--6IFE5lqezFA2FA3M--