Bug #125331 for DBIx-Class: [RFE] support for window functions

Wed May 16 19:39:04 2018 rrwo [...] cpan.org - Ticket created

Subject:	[RFE] support for window functions
Date:	Thu, 17 May 2018 00:38:54 +0100
To:	bug-DBIx-Class [...] rt.cpan.org
From:	Robert Rothenberg <rrwo [...] cpan.org>

It would be nice to have support for window functions, e.g. ROW_NUMBER() OVER ( PARTITION BY some_column ORDER BY other ) It could probably be handled by allowing sn "-over" key in a select clause, e.g. $rs->select_rs( ..., { '+select' => [ row_number => [], -over => { partition_by => [...], # same as group_by order_by => [...], # same as order_by }, ], } )

Thu May 17 02:45:56 2018 ribasushi [...] leporine.io - Correspondence added

Subject:	Re: [rt.cpan.org #125331] [RFE] support for window functions
Date:	Thu, 17 May 2018 08:44:58 +0200
To:	bug-DBIx-Class [...] rt.cpan.org
From:	Peter Rabbitson <ribasushi [...] leporine.io>

On 05/17/2018 01:39 AM, Robert Rothenberg via RT wrote: Show quoted text

> It would be nice to have support for window functions, e.g. > > ROW_NUMBER() OVER ( PARTITION BY some_column ORDER BY other ) > > It could probably be handled by allowing sn "-over" key in a select clause, e.g. > > $rs->select_rs( > ..., > { > '+select' => [ > > row_number => [], > -over => { > partition_by => [...], # same as group_by > order_by => [...], # same as order_by > }, > > ], > } > ) >

Generally the core API strives to remain tethered to "lowest common denominator" of what various SQL databases would understand. There have been attempts to add rdbms-specific syntax, all of them turned out rather... badly. At the same time it should be beyond trivial and much safer from an experimentation angle to make an external ResultSet component which implements pre-parsing of attributes handed to search_rs(). Is there a reason why you didn't consider going the route of an extension? Let me know your thoughts! P.S. An example implementation of such an extension: https://metacpan.org/source/FREW/DBIx-Class-Helpers-2.033004/lib/DBIx/Class/Helper/ResultSet/RemoveColumns.pm

Thu May 17 02:45:56 2018 The RT System itself - Status changed from 'new' to 'open'

Thu May 17 03:31:34 2018 rrwo [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #125331] [RFE] support for window functions
Date:	Thu, 17 May 2018 08:31:20 +0100
To:	bug-DBIx-Class [...] rt.cpan.org
From:	Robert Rothenberg <rrwo [...] cpan.org>

Window functions are part of SQL (2003?) standard, and are one of the fallbacks used for implementing limits in DBIx::Class. There's a difference between supporting a feature that some databases do not support vs requiring it. I didn't consider an extension, but I will look into it. On 17/05/18 07:45, Peter Rabbitson via RT wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=125331 > > > On 05/17/2018 01:39 AM, Robert Rothenberg via RT wrote: >

>> It would be nice to have support for window functions, e.g. >> >> ROW_NUMBER() OVER ( PARTITION BY some_column ORDER BY other ) >> >> It could probably be handled by allowing sn "-over" key in a select clause, e.g. >> >> $rs->select_rs( >> ..., >> { >> '+select' => [ >> >> row_number => [], >> -over => { >> partition_by => [...], # same as group_by >> order_by => [...], # same as order_by >> }, >> >> ], >> } >> ) >>

> > Generally the core API strives to remain tethered to "lowest common > denominator" of what various SQL databases would understand. There have > been attempts to add rdbms-specific syntax, all of them turned out > rather... badly. > > At the same time it should be beyond trivial and much safer from an > experimentation angle to make an external ResultSet component which > implements pre-parsing of attributes handed to search_rs(). > > Is there a reason why you didn't consider going the route of an > extension? Let me know your thoughts! > > P.S. An example implementation of such an extension: > https://metacpan.org/source/FREW/DBIx-Class-Helpers-2.033004/lib/DBIx/Class/Helper/ResultSet/RemoveColumns.pm >

Thu May 17 03:44:35 2018 ribasushi [...] leporine.io - Correspondence added

Subject:	Re: [rt.cpan.org #125331] [RFE] support for window functions
Date:	Thu, 17 May 2018 09:44:02 +0200
To:	bug-DBIx-Class [...] rt.cpan.org
From:	Peter Rabbitson <ribasushi [...] leporine.io>

On 05/17/2018 09:31 AM, Robert Rothenberg via RT wrote: Show quoted text

> Queue: DBIx-Class > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=125331 > > > > Window functions are part of SQL (2003?) standard, and are one of the > fallbacks used for implementing limits in DBIx::Class.

Not a fallback, but rather convoluted mechanism which is designed to be fully encapsulated / non-reusable and is activated in very tightly controlled circumstances. Show quoted text

> There's a difference between supporting a feature that some databases do not > support vs requiring it. >

The current context is not about supporting a standardized feature that has several outliers. It is about not mixing things that are truly standard with things that are standardized only on paper in the same API. In reality there is essentially no window function expression that you can take from one engine and run on another. Add to that that they are in general poorly supported ( only Pg and Oracle have sufficiently similar syntax iirc, with MSSQLServer having something similar yet different, but I have not looked at this for about 3 years, so don't take my word on it ). Bottom line is that I am not comfortable extending the main DBIC API surface into "one-off" territory ( and while I understand the sentiment of "but the standard says..." - I strongly disagree with it ). Show quoted text

> I didn't consider an extension, but I will look into it.

If you run into major difficulties creating an extension - we should definitely look into providing whatever foundation is missing. I will keep this ticket open for a while, to serve as a spot to coordinate whatever further questions may arise Cheers!

Thu May 17 04:08:55 2018 robrwo [...] gmail.com - Correspondence added

Subject:	Re: [rt.cpan.org #125331] [RFE] support for window functions
Date:	Thu, 17 May 2018 09:08:37 +0100
To:	bug-DBIx-Class [...] rt.cpan.org, rrwo [...] cpan.org
From:	Robert Rothenberg <robrwo [...] gmail.com>

I'm not sure how modifying the ResultSet will be able to add how SQL is generated. It could call the (private) sql_maker methods for handling the select fields to generate the function SQL, but that seems like more of a hack than just subclassing DBIx::Class::SQLMaker and changing to that class in storage. Ideally, it would be nice to have an easy way to apply plugins without resorting to monkey-patching. On 17/05/18 08:44, Peter Rabbitson via RT wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=125331 > > > On 05/17/2018 09:31 AM, Robert Rothenberg via RT wrote:

>> Queue: DBIx-Class >> Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=125331 > >> >> >> Window functions are part of SQL (2003?) standard, and are one of the >> fallbacks used for implementing limits in DBIx::Class.

> > Not a fallback, but rather convoluted mechanism which is designed to be > fully encapsulated / non-reusable and is activated in very tightly > controlled circumstances. >

>> There's a difference between supporting a feature that some databases do not >> support vs requiring it. >>

> > The current context is not about supporting a standardized feature that > has several outliers. It is about not mixing things that are truly > standard with things that are standardized only on paper in the same > API. In reality there is essentially no window function expression that > you can take from one engine and run on another. Add to that that they > are in general poorly supported ( only Pg and Oracle have sufficiently > similar syntax iirc, with MSSQLServer having something similar yet > different, but I have not looked at this for about 3 years, so don't > take my word on it ). > > Bottom line is that I am not comfortable extending the main DBIC API > surface into "one-off" territory ( and while I understand the sentiment > of "but the standard says..." - I strongly disagree with it ). > >

>> I didn't consider an extension, but I will look into it.

> > If you run into major difficulties creating an extension - we should > definitely look into providing whatever foundation is missing. > > I will keep this ticket open for a while, to serve as a spot to > coordinate whatever further questions may arise > > Cheers! >

Thu May 17 04:19:52 2018 ribasushi [...] leporine.io - Correspondence added

Subject:	Re: [rt.cpan.org #125331] [RFE] support for window functions
Date:	Thu, 17 May 2018 10:19:31 +0200
To:	bug-DBIx-Class [...] rt.cpan.org
From:	Peter Rabbitson <ribasushi [...] leporine.io>

On 05/17/2018 10:08 AM, Robert Rothenberg via RT wrote: Show quoted text

> Queue: DBIx-Class > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=125331 > > > > I'm not sure how modifying the ResultSet will be able to add how SQL is > generated. >

No monkeypatching was suggested / would be required. Your hook would inject the already-rendered SQL ( possibly by re-usin $rs->result_source->schema->storage->sql_maker ) into the appropriate part of the search() attr stack as literals, before passing the call further. I.e. you can and should rely on this mechanism: https://github.com/Perl5/DBIx-Class/blob/dc7d89911/lib/DBIx/Class/Storage/DBIHacks.pm#L398-L411

Tue May 22 08:32:37 2018 rrwo [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #125331] [RFE] support for window functions
Date:	Tue, 22 May 2018 13:32:09 +0100
To:	bug-DBIx-Class [...] rt.cpan.org
From:	Robert Rothenberg <rrwo [...] cpan.org>

I have a prototype at https://github.com/robrwo/DBIx-Class-Helper-ResultSet-WindowFunctions It still needs some work. Notably, I'm not sure how to add the current_source_alias to column names in the generated SQL. Note that the tests just use SQLite so that it can test SQL generation. Obviously window functions are not (yet) supported by it. On 17/05/18 09:19, Peter Rabbitson via RT wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=125331 > > > On 05/17/2018 10:08 AM, Robert Rothenberg via RT wrote:

>> Queue: DBIx-Class >> Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=125331 > >> >> >> I'm not sure how modifying the ResultSet will be able to add how SQL is >> generated. >>

> > No monkeypatching was suggested / would be required. Your hook would > inject the already-rendered SQL ( possibly by re-usin > $rs->result_source->schema->storage->sql_maker ) into the appropriate > part of the search() attr stack as literals, before passing the call > further. > > I.e. you can and should rely on this mechanism: > https://github.com/Perl5/DBIx-Class/blob/dc7d89911/lib/DBIx/Class/Storage/DBIHacks.pm#L398-L411 >

Tue May 22 09:17:56 2018 ribasushi [...] leporine.io - Correspondence added

Subject:	Re: [rt.cpan.org #125331] [RFE] support for window functions
Date:	Tue, 22 May 2018 15:17:36 +0200
To:	bug-DBIx-Class [...] rt.cpan.org
From:	Peter Rabbitson <ribasushi [...] leporine.io>

On 05/22/2018 02:32 PM, Robert Rothenberg via RT wrote: Show quoted text

> Queue: DBIx-Class > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=125331 > > > > I have a prototype at > https://github.com/robrwo/DBIx-Class-Helper-ResultSet-WindowFunctions

You got the right idea, yes. However, I would first call $rs->next::method, and then operate on it result: it already would have normalized the select/all lists, unpacking the columns attributes as appropriate. I.e. all this work: https://metacpan.org/source/RIBASUSHI/DBIx-Class-0.082841/lib/DBIx/Class/ResultSet.pm#L3528-3592 Show quoted text

> > It still needs some work. Notably, I'm not sure how to add the > current_source_alias to column names in the generated SQL.

What do you mean? You already have $rs->current_source_alias at your disposal in that method... Show quoted text

> > Note that the tests just use SQLite so that it can test SQL generation. > Obviously window functions are not (yet) supported by it. >

Try to avoid `like $sql, ...` constructs in tests, as they would fail in the face of small non-semantic changes the generated SQL ( it happens rarely, but it happens ). Instead I would strongly suggest using: https://metacpan.org/pod/release/RIBASUSHI/SQL-Abstract-1.81/lib/SQL/Abstract/Test.pm#SYNOPSIS

Tue May 22 09:46:57 2018 rrwo [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #125331] [RFE] support for window functions
Date:	Tue, 22 May 2018 14:46:33 +0100
To:	bug-DBIx-Class [...] rt.cpan.org
From:	Robert Rothenberg <rrwo [...] cpan.org>

On 22/05/18 14:17, Peter Rabbitson via RT wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=125331 > > > On 05/22/2018 02:32 PM, Robert Rothenberg via RT wrote:

>> Queue: DBIx-Class >> Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=125331 > >> >> >> I have a prototype at >> https://github.com/robrwo/DBIx-Class-Helper-ResultSet-WindowFunctions

> > You got the right idea, yes. However, I would first call > $rs->next::method, and then operate on it result: it already would have > normalized the select/all lists, unpacking the columns attributes as > appropriate. I.e. all this work: > https://metacpan.org/source/RIBASUSHI/DBIx-Class-0.082841/lib/DBIx/Class/ResultSet.pm#L3528-3592

If I don't remove the "-over" argument, I get the following error: DBIx::Class::SQLMaker::_recurse_fields(): Malformed select argument - too many keys in hash: -as,-over,rank at t/01-basic.t line 28 Show quoted text

>

>> >> It still needs some work. Notably, I'm not sure how to add the >> current_source_alias to column names in the generated SQL.

> > What do you mean? You already have $rs->current_source_alias at your > disposal in that method...

Yes, but I'd like to avoid duplucating existing code. Show quoted text

>> >> Note that the tests just use SQLite so that it can test SQL generation. >> Obviously window functions are not (yet) supported by it. >>

> > Try to avoid `like $sql, ...` constructs in tests, as they would fail in > the face of small non-semantic changes the generated SQL ( it happens > rarely, but it happens ). Instead I would strongly suggest using: > https://metacpan.org/pod/release/RIBASUSHI/SQL-Abstract-1.81/lib/SQL/Abstract/Test.pm#SYNOPSIS >

Thanks.

Tue May 22 10:20:47 2018 ribasushi [...] leporine.io - Correspondence added

Subject:	Re: [rt.cpan.org #125331] [RFE] support for window functions
Date:	Tue, 22 May 2018 16:20:25 +0200
To:	bug-DBIx-Class [...] rt.cpan.org
From:	Peter Rabbitson <ribasushi [...] leporine.io>

On 05/22/2018 03:46 PM, Robert Rothenberg via RT wrote: Show quoted text

> Queue: DBIx-Class > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=125331 > > > On 22/05/18 14:17, Peter Rabbitson via RT wrote:

>> <URL: https://rt.cpan.org/Ticket/Display.html?id=125331 > >> >> On 05/22/2018 02:32 PM, Robert Rothenberg via RT wrote:

>>> Queue: DBIx-Class >>> Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=125331 > >>> >>> >>> I have a prototype at >>> https://github.com/robrwo/DBIx-Class-Helper-ResultSet-WindowFunctions

>> >> You got the right idea, yes. However, I would first call >> $rs->next::method, and then operate on it result: it already would have >> normalized the select/all lists, unpacking the columns attributes as >> appropriate. I.e. all this work: >> https://metacpan.org/source/RIBASUSHI/DBIx-Class-0.082841/lib/DBIx/Class/ResultSet.pm#L3528-3592

> > If I don't remove the "-over" argument, I get the following error: > > DBIx::Class::SQLMaker::_recurse_fields(): Malformed select argument - too > many keys in hash: -as,-over,rank at t/01-basic.t line 28 >

I see... hm hm hm. For the time being leaving things as-you-wrote-them would make most sense, but gives me food for thought Show quoted text

>>

>>> >>> It still needs some work. Notably, I'm not sure how to add the >>> current_source_alias to column names in the generated SQL.

>> >> What do you mean? You already have $rs->current_source_alias at your >> disposal in that method...

> > Yes, but I'd like to avoid duplucating existing code. >

Errr... you need to expand on that. From my perspective there is no specific handling of c_s_a aside from this: https://metacpan.org/source/RIBASUSHI/DBIx-Class-0.082841/lib/DBIx/Class/ResultSet.pm#L3564-3565 You seem to see this as something more involved, thus my confusion...