vapor / sql-kit

*️⃣ Build SQL queries in Swift. Extensible, protocol-based design that supports DQL, DML, and DDL.
MIT License
248 stars 58 forks source link

Adds `LIMIT`, `OFFSET` and `ORDER BY` to Union results #147

Closed NeedleInAJayStack closed 2 years ago

NeedleInAJayStack commented 2 years ago

Previously it was not easy to do pagination on the result of SELECT ... UNION queries. For example:

(SELECT * FROM "Table" WHERE "name" = 'first thing')
UNION ALL
(SELECT * FROM "Zone" WHERE "name" = 'second thing')
LIMIT 5
OFFSET 3
ORDER BY "name"

This pull request adds LIMIT, OFFSET, and ORDER BY functionality to the SQLUnion/SQLUnionBuilder, primarily by copying over the SQLSelect and SQLSubqueryClauseBuilder implementations.

I also have a protocol-based approach that reduces code copying but it comes with it's own smells. I'm happy to elaborate if desired.

codecov-commenter commented 2 years ago

Codecov Report

Merging #147 (adc48ae) into main (b7901df) will increase coverage by 0.62%. The diff coverage is 91.30%.

@@            Coverage Diff             @@
##             main     #147      +/-   ##
==========================================
+ Coverage   68.60%   69.23%   +0.62%     
==========================================
  Files          93       94       +1     
  Lines        3450     3478      +28     
==========================================
+ Hits         2367     2408      +41     
+ Misses       1083     1070      -13     
Flag Coverage Ξ”
unittests 69.23% <91.30%> (+0.62%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Ξ”
...rces/SQLKit/Builders/SQLSubqeryClauseBuilder.swift 70.73% <66.66%> (+2.64%) :arrow_up:
Sources/SQLKit/Builders/SQLUnionBuilder.swift 81.18% <66.66%> (+3.29%) :arrow_up:
...rces/SQLKit/Builders/SQLPartialResultBuilder.swift 100.00% <100.00%> (ΓΈ)
Sources/SQLKit/Query/SQLUnion.swift 81.39% <100.00%> (+4.25%) :arrow_up:
Sources/SQLKit/Query/SQLSelect.swift 100.00% <0.00%> (+7.27%) :arrow_up:
Sources/SQLKit/Query/SQLDirection.swift 83.33% <0.00%> (+8.33%) :arrow_up:
NeedleInAJayStack commented 2 years ago

Hey @gwynne, thanks for the review!

I've broken out a builder protocol that SQLSelectBuilder and SQLUnionBuilder both conform to. I thought that SQLPaginateBuilder was more descriptive than SQLPartialResultBuilder, but I'm definitely happy to change it to SQLPartialResultBuilder if you prefer.

Thanks again!

gwynne commented 2 years ago

I've broken out a builder protocol that SQLSelectBuilder and SQLUnionBuilder both conform to. I thought that SQLPaginateBuilder was more descriptive than SQLPartialResultBuilder, but I'm definitely happy to change it to SQLPartialResultBuilder if you prefer.

I would prefer the more generic name in this case, mostly because it is more generic and the intent of the various builder protocols is to encapsulate generalized functionality which is common to multiple - but not all - builder contexts; pagination is a very specific operation that is supported by the clauses in question, but not the only one (to grab a trivial example, the LIMIT clause is also used to ensure there's only one result from a .first() operation).

Other than that, great work! I added a couple of minor additional remarks to the updated code, once that's all taken care of, this should be ready for merging πŸ‘

gwynne commented 2 years ago

I wish the API breakage checker was smarter about cross-protocol refactoring 😞

gwynne commented 2 years ago

(Sorry that I keep finding more things to fix; I haven't looked closely enough every time through, I'm afraid πŸ€¦β€β™€οΈ)

NeedleInAJayStack commented 2 years ago

(Sorry that I keep finding more things to fix; I haven't looked closely enough every time through, I'm afraid πŸ€¦β€β™€οΈ)

No worries at all! It's better to get it right now than have to fix it later.

VaporBot commented 2 years ago

These changes are now available in 3.20.0