public inbox for [email protected]
help / color / mirror / Atom feedFrom: Julien Rouhaud <[email protected]>
To: Tomas Vondra <[email protected]>
Cc: Thomas <[email protected]>
Cc: [email protected]
Cc: Jaime Casanova <[email protected]>
Cc: pgsql <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: Thomas Mannhart <[email protected]>
Subject: Re: Patch: Range Merge Join
Date: Mon, 17 Jan 2022 15:39:33 +0800
Message-ID: <20220117073933.m7ef2hwaubpcd75v@jrouhaud> (raw)
In-Reply-To: <[email protected]>
References: <CAMWfgiAsJgVrkbrsv740Y2=2duO4rVYRhaD08EhFqBuJFmBH1A@mail.gmail.com>
<CAApHDvoyAAkJjZ=DxyOMDhSNDg3Fo799OAF6c8gzfLWBMNaK0Q@mail.gmail.com>
<[email protected]>
<20211004212754.GA30366@ahch-to>
<20211005003034.GA30929@ahch-to>
<CAMWfgiDDvHqN2STsCyKj9oyBJ1yCQdDOp_6Jc0xupuJ8fn9xtQ@mail.gmail.com>
<[email protected]>
<CAMWfgiBRETALLvhk9P3dCsii8cL1XyYHv87=MH4OtnT5Y+o9Ng@mail.gmail.com>
<[email protected]>
Hi,
On Wed, Nov 17, 2021 at 11:28:43PM +0100, Tomas Vondra wrote:
> On 11/17/21 15:45, Thomas wrote:
> > Thank you for the feedback and sorry for the oversight. I fixed the bug
> > and attached a new version of the patch.
> >
> > Kind Regards, Thomas
> >
> > Am Mi., 17. Nov. 2021 um 15:03 Uhr schrieb Daniel Gustafsson
> > <[email protected] <mailto:[email protected]>>:
> >
> > This patch fails to compile due to an incorrect function name in an
> > assertion:
> >
> > nodeMergejoin.c:297:9: warning: implicit declaration of function
> > 'list_legth' is invalid in C99 [-Wimplicit-function-declaration]
> > Assert(list_legth(node->rangeclause) < 3);
> >
>
> That still doesn't compile with asserts, because MJCreateRangeData has
>
> Assert(list_length(node->rangeclause) < 3);
>
> but there's no 'node' variable :-/
>
>
> I took a brief look at the patch, and I think there are two main issues
> preventing it from moving forward.
>
> 1) no tests
>
> 2) lack of comments
>
> 3) I'm not quite sure I like "Range Merge Join" to be honest. It's still a
> "Merge Join" pretty much. What about ditching the "Range"? There'll still be
> "Range Cond" key, which should be good enough I think.
>
> 4) Some minor whitespace issues (tabs vs. spaces). See 0002.
It's been 2 months since Tomas posted that review.
Thomas, do you plan to work on that patch during this commitfest?
view thread (2+ messages)
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], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Patch: Range Merge Join
In-Reply-To: <20220117073933.m7ef2hwaubpcd75v@jrouhaud>
* 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