Re: Converting SetOp to read its two inputs separately

David Rowley <dgrowleyml(at)gmail(dot)com> writes:
> On Fri, 20 Dec 2024 at 11:23, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Attached is a patch to take it out and then rename
>> BuildTupleHashTableExt() back to BuildTupleHashTable().

> No complaints here. Thanks for cleaning that up.

Thanks, will push.

> I couldn't help but also notice the nbuckets parameter is using the
> "long" datatype. The code in BuildTupleHashTable seems to think it's
> fine to pass the long as the uint32 parameter to tuplehash_create().
> I just thought if we're in the area of adjusting this API function's
> signature then it might be worth fixing the "long" issue at the same
> time, or at least in the same release.

I'm in favor of doing that, but it seems like a separate patch,
because we'd have to chase things back a fairly long way.
For instance, the numGroups fields in Agg, RecursiveUnion, and
SetOp are all "long" at the moment, and some of the callers
are getting their arguments via clamp_cardinality_to_long()
which'd need adjustment, etc etc.

> I'm also quite keen to see less use of long as it's not a very
> consistently sized datatype on all platforms which can lead to
> platform dependent bugs.

Yeah. Switching all these places to int64 likely would be
worthwhile cleanup (but I'm not volunteering). Also, if
tuplehash_create expects a uint32, that isn't consistent
either.

regards, tom lane