public inbox for [email protected]  

help / color / mirror / Atom feed

From: Greg Burd <[email protected]>
To: Michael Paquier <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: Nathan Bossart <[email protected]>
Cc: Masahiko Sawada <[email protected]>
Subject: Re: [PATCH] Add tests for Bitmapset
Date: Mon, 22 Sep 2025 09:30:24 -0400
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>


On Sep 22 2025, at 3:57 am, Michael Paquier <[email protected]> wrote:

> 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.
>> 
>> coverage:    HEAD    v7    v8
>> lines......:  87.1  90.5  92.2

Michael,

Thank you for your time and help.

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

All great ideas, apologies for the oversights.

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

Agreed, thank you.

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

Sounds good to me.

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

Yes, copied from the radixtree test module.

> Another thing was testing with -DREALLOCATE_BITMAPSETS, which I've
> done.

That seems like a good idea, I thought about doing that as well.

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

Fair.

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

Thanks for the help getting this done, I really appreciate it.  So far
the buildfarm seems okay. :)

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], [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