frioux / DBIx-Class-Helpers

https://metacpan.org/pod/DBIx::Class::Helpers
20 stars 38 forks source link

SetOperations UNION needs extra brackets (PostgreSQL)? #85

Closed ollyg closed 6 years ago

ollyg commented 6 years ago

Hello, I am using SetOperations union() method. I found that when the two queries to be combined in the union have ORDER BY or LIMIT clauses within them, PostgreSQL complains of a syntax error. This is because the two queries should be contained within round brackets "()" before adding the UNION.

However within SetOperations.pm there is the following which seems intended to strip these brackets: https://github.com/frioux/DBIx-Class-Helpers/blob/31cff7200d9404e0210c6f54b9abe18c7971856d/lib/DBIx/Class/Helper/ResultSet/SetOperations.pm#L74

What I did as a patch, which works well in PostgreSQL, is to comment out this line and also add round brackets explicitly to each query (much like line 80, in fact): $sql = q<(> . $sql . q<)>;

Can you let me know whether you want this behaviour or not? I will not be offended if you close the ticket! However at the moment I have monkeypatched SetOperations locally to allow the UNION queries to have ORDER BY and LIMIT.

Thanks, Oliver.

frioux commented 6 years ago

Patches totally welcome! This code ends up really engine specific so these things happen. I can probably fix it without too much effort but if you want to try by all means do.

-- Sent from a rotary phone rented from Ma Bell

On Nov 27, 2017 4:35 AM, "Oliver Gorwits" notifications@github.com wrote:

Hello, I am using SetOperations union() method. I found that when the two queries to be combined in the union have ORDER BY or LIMIT clauses within them, PostgreSQL complains of a syntax error. This is because the two queries should be contained within round brackets "()" before adding the UNION.

However within SetOperations.pm there is the following which seems intended to strip these brackets: https://github.com/frioux/DBIx-Class-Helpers/blob/ 31cff7200d9404e0210c6f54b9abe18c7971856d/lib/DBIx/Class/Helper/ResultSet/ SetOperations.pm#L74

What I did as a patch, which works well in PostgreSQL, is to comment out this line and also add round brackets explicitly to each query (much like line 80, in fact): $sql = q<(> . $sql . q<)>;

Can you let me know whether you want this behaviour or not? I will not be offended if you close the ticket! However at the moment I have monkeypatched SetOperations locally to allow the UNION queries to have ORDER BY and LIMIT.

Thanks, Oliver.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/frioux/DBIx-Class-Helpers/issues/85, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAf45JbqjJKROVnrrFzB96q2ezJPXbaks5s6qyPgaJpZM4QrkeG .

ollyg commented 6 years ago

Upon further investigation, I think that PostgreSQL is supporting a local extension to allow LIMIT clauses within UNION operands. That is, the SetOperations.pm code is correct (according to SQL standard), and its test is correct, so I would not change anything.

frioux commented 6 years ago

FWIW I bet ->as_subselect_rs->search(...) would work with a limit.

-- Sent from a rotary phone rented from Ma Bell

On Dec 8, 2017 12:58 PM, "Oliver Gorwits" notifications@github.com wrote:

Upon further investigation, I think that PostgreSQL is supporting a local extension to allow LIMIT clauses within UNION operands. That is, the SetOperations.pm code is correct (according to SQL standard), and its test is correct, so I would not change anything.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/frioux/DBIx-Class-Helpers/issues/85#issuecomment-350370139, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAf40e8PtfpmsDsTBKGISgQ2CRlfuvGks5s-aLagaJpZM4QrkeG .

ollyg commented 6 years ago

Ah! Thanks! I was heading in that direction too (select x,y from (select .... limit 10)) but the as_subselect_rs tip will get me over the line. Much appreciated!