Add "LIMIT 1" to `fetchOne` requests #515

alextrob · 2019-04-11T03:57:27Z

Adds "LIMIT 1" to fetchOne requests when appropriate. More discussion and reasoning about this is in #513.

"LIMIT 1" is not added when querying by primary key or a unique key.

// SELECT * FROM player WHERE rowid = 1
try Player.fetchOne(key: 1)

// SELECT * FROM player WHERE email = "test@example.com"
// (assuming `email` is unique)
try Player.fetchOne(key: ["email": "test@example.com"])

But it is added in other cases where only one record needs to be fetched.

This PR includes a change to the FetchRequest protocol. prepare now has a singleResult parameter that conforming types can optionally use to generate a more suitable query.

protocol FetchRequest {
    func prepare(_ db: Database, forSingleResult singleResult: Bool) throws -> (SelectStatement, RowAdapter?)
}

For the built-in FetchRequest conformers:

QueryInterfaceRequest uses singleResult to add (or not add) "LIMIT 1" to a query.
SQLRequest ignores singleResult because when you opt in to raw SQL you probably don't expect GRDB to modify your query.

Pull Request Checklist

~~This pull request is submitted against the development branch.~~ (@groue requested that this target the GRDB-4.0 branch).
Inline documentation has been updated.
README.md or another dedicated guide has been updated.
Changes are tested, for all flavors of GRDB, GRDBCipher, and GRDBCustomSQLite.

Tests currently fail

Hints are ignored by SQLRequest. When you write raw SQL you probably don't want GRDB to modify your query.

This problem was masked by the fact all queries in the test expected the same SQL query.

groue · 2019-04-11T12:07:39Z

Tests/GRDBTests/RecordMinimalPrimaryKeyRowIDTests.swift

@@ -380,6 +381,7 @@ class RecordMinimalPrimaryKeyRowIDTests : GRDBTestCase {

            let fetchedRecord = try MinimalRowID.filter(key: ["id": record.id]).fetchOne(db)!
            XCTAssertTrue(fetchedRecord.id == record.id)
+            XCTAssertEqual(lastSQLQuery, "SELECT * FROM \"minimalRowIDs\" WHERE (\"id\" = \(record.id!)) LIMIT 1")


I'm very happy you added this test.

I'm somewhat sorry, because this technique for fetching one record by primary or unique key wasn't listed in my "exhaustive" list in #513:

let request = Player.filter(key: 123) // or Player.filter(key: ["email": "me@example.com"]) let player = try dbQueue.read { db in try request.fetchOne(db) }

For the context, this alternative way to perform fetchOne(_:key:) is useful because filter(key:) requests can be observed so that the application is notified of db changes:

let observation = ValueObservation(trackingOne: Player.filter(key: 123)) try observation.start(in: dbQueue) { (player: Player?) in print("Player has changed: \(player)") }

What about trying to remove LIMIT 1 here as well?

One way to achieve this goal is to add a flag to SQLSelectQuery. When this flag is set, we could ignore the limitOne hint. And maybe we'd no longer need the primaryKeyOrUnique hint after that?

We'd need to make sure that all SQLSelectQuery derivation methods transfer this flag from one query to another. But after a quick glance, it looks like all those methods are already information-preserving.

This would be a safe technique because all request derivation methods can only increase the amount of filtering, never relax it:

// Player 123, along with its team, but only if it is active: // // SELECT player.*, team.* // FROM player // JOIN team ON team.id = player.teamId // WHERE player.id = 123 AND player.isActive = 1 let request = Player .filter(key: 123) .filter(Column("isActive") == true) .including(required: Player.team)

NB: this change is not required for this pull request to enter review. We can proceed without it if you prefer. But if you agree with the suggestion, and if you have a little more time, this change would be appreciated.

Hint: look for filter<PrimaryKeyType: DatabaseValueConvertible>(key: and filter(key:.

Good suggestion @groue, and thanks for the hints 😄

Definitely agree it'd be good to remove LIMIT 1 from the SQL there. I can look into it.

Cool, thank you!

Ok I think I've got that implemented as you described it. Tests are updated to exclude LIMIT 1 from the SQL in that scenario.

groue · 2019-04-11T17:16:51Z

GRDB/Core/FetchRequest.swift

@@ -15,10 +15,20 @@ public protocol FetchRequest: DatabaseRegionConvertible {
    /// Returns a tuple that contains a prepared statement that is ready to be
    /// executed, and an eventual row adapter.
    ///
+    /// Default implementation uses `prepare(db, hint: nil)`.


Second question: what is the purpose of keeping prepare(_:) in the protocol? Is there any reason for a type to implement both methods? Isn't prepare(_:hint:) sufficient?

Because the protocol's public I had this idea of not wanting to break compatibility with anything already calling prepare(_:). Also thought hint could be confusing when you don't need it.

I'd have preferred to declare it as:

func prepare(_ db: Database, hint: FetchRequestHint? = nil) throws -> (SelectStatement, RowAdapter?)

… but you can't have a default argument in a protocol.

Anyway, I'm not too attached to it. Happy to take it out if you'd prefer.

All right, I understand. The good news is that we can break things, because we are targeting the next major version. GRDB 4 will break several things (see #479 for the list of changes). So we can keep a single method.

I agree the hint adds a layer of complexity to the API, but I don't expect many users to write their own concrete type which conforms to FetchRequest. This is an advanced technique. If they do, I guess they'll be able to handle it. And we don't want a default value for the hint, because being explicit is better here.

But we have to document clearly that this hint is only an opportunity for optimization, and can safely be ignored.

Towards this goal of clarity, I think we'll also have to look for alternative names for hint and FetchRequestHint, and wonder if we really want an optional. Those currently sound too much like an implementation detail, and not enough like a public API.

I don't know... The radical solution is:

func prepare(_ db: Database, forSingleResult singleResult: Bool) -> ... { if singleResult { // Add LIMIT 1 return ... } else { // Don't add LIMIT 1 return ... } }

But a boolean has little room for further extensions (even if I can't foresee any right now). I agree with your idea of an enum:

func prepare(_ db: Database, forCardinality cardinality: FetchCardinality) -> ... { switch cardinality { case .single: // Add LIMIT 1 return ... case .multiple: // Don't add LIMIT 1 return ... } }

But "cardinality" can't really be extended either. Not really an improvement over a boolean. Let's go all general:

func prepare(_ db: Database, forPurpose purpose: FetchPurpose) -> ... { switch purpose { case .singleResult: // Add LIMIT 1 return ... case .multipleResults: // Don't add LIMIT 1 return ... } }

I wish we could have merged FetchRequest.fetchCount here:

func prepare(_ db: Database, forPurpose purpose: FetchPurpose) -> ... { switch purpose { case .singleResult: // Add LIMIT 1 return ... case .multipleResults: // Don't add LIMIT 1 return ... case .count: // Build the counting statement return ... } }

But we can't choose this last route, because fetchCount is currently an optional method with a non-trivial default implementation: users would now have real difficulties adopting the FetchRequest protocol.

Well.. As you see, we have much room in our way to the best design. I'm curious about your ideas as well. Our goal: turn our implementation strategy into a public API that can make sense for people who don't have a clue about our implementation strategy. I mean that most users are utterly indifferent about how the library works, and only care about their needs. Good public APIs acknowledge this hard fact. It may look difficult, but this can also be a funny game :-)

A technique that has constantly helped me finding an answer to this kind of question has been the README. We currently write, below the introduction to the FetchRequest protocol:

The prepare method returns a prepared statement and an optional row adapter. The prepared statement tells which SQL query should be executed. The row adapter helps presenting the fetched rows in the way expected by the row decoders (see row adapter).

We can try to update this paragraph so that it explains how to use the hint. Experience shows that when the documentation looks like it fills its teaching goal, the API is good as well.

Applying my own advice makes the plain and simple boolean approach look not so bad, after all:

The prepare(_:forSingleResult:) method accepts a database connection, a singleResult hint, and returns a prepared statement and an optional row adapter. Conforming types can use the singleResult hint as an optimization opportunity, and return a prepared statement that fetches at most one row, with a LIMIT SQL clause, when possible. The optional row adapter helps presenting the fetched rows in the way expected by the row decoders (see row adapters).

I'm coming around to the idea of a plain old boolean. Complicating the API for unknown future cases probably isn't worth it. If other scenarios do come up in the future, it can be changed in a way that really makes sense.

Complicating the API for unknown future cases probably isn't worth it.

Indeed, if we really can't find any extension for the "hint" today, then let's not try to outsmart ourselves. As you say, we'll always be able to revise our opinion in the future, if needed!

We have done our exploration job: if the boolean approach is OK with you too, then I think we can just relax :-)

Cool. I've switched it to a boolean and updated the README.

groue · 2019-04-12T07:43:30Z

GRDB/QueryInterface/QueryInterfaceRequest.swift

+
+        switch hint {
+        case .limitOne?:
+            query = self.query.limit(1)


Here we break the following code:

let player = try Player.limit(1, offset: 10).fetchOne(db)

We have to honor the eventual offset, if present.

Whoops! Should be fixed in the latest commits (and added a test to catch it).

Fixes previous test failure

Tests now fail

Adds a flag to SQLSelectQuery indicating that it expects a single record (for example when querying by primary or unique key). Ensures that this example doesn't add an unnecessary LIMIT 1 to the resulting query: let request = Player.filter(key: 123) // or Player.filter(key: ["email": "me@example.com"]) let player = try dbQueue.read { db in try request.fetchOne(db) }

alextrob · 2019-04-16T01:32:47Z

Ready for another look @groue 😊

This better hides this flag inside SQLSelectQuery, and avoids explicit support from FetchableRecord and TableRecord

groue · 2019-04-16T06:58:42Z

@alextrob, this is really great.

I added a couple commits:

46791c8 remove an extra initializer which was not needed.
4716f91 makes filter(key:) sole responsible for the expectsSingleResult flag. I understand that you did not "dare" going this route, but I think this is better now: other types and protocols remain totally unaware of this flag.

Now I'll look at tests, which I'm sure are excellent, and we'll merge this PR very shortly :-)

alextrob · 2019-04-16T08:42:52Z

Nice one. I like your changes @groue 👍

We do not add LIMIT 1 to raw SQL requests because it is difficult. But it would not be wrong to do so.

We usually update all those tests together

groue · 2019-04-17T12:06:49Z

@alextrob, this looks very good. I have removed tests about the lack of extra LIMIT 1 for raw sql requests, because it's more a matter of implementation difficulty than a hard choice. I'll now just add a couple more tests that fill some holes. You can consider your part as done, and well done on top of that!

groue · 2019-04-18T05:12:46Z

Merged. Thank you very much @alextrob and @ealymbaev: I did not immediately recognize the value of this change, but I'm now totally convinced.

Alex, I'd be happy to grant you push access to the repository. Drop me a line if you are interested!

alextrob · 2019-04-18T05:22:25Z

Thanks @groue! Happy I got to contribute 😄

groue · 2019-04-18T05:23:50Z

Invitation sent 🤝

alextrob added 6 commits April 11, 2019 11:51

Update tests to expect "LIMIT 1" where appropriate

f9868bd

Tests currently fail

More tests for presence of LIMIT 1

af1fd6b

Add hints for use in request preparation

072fa25

Hints are ignored by SQLRequest. When you write raw SQL you probably don't want GRDB to modify your query.

Fix testing lastSQLQuery when a cursor hasn't retrieved a record

dd139b6

This problem was masked by the fact all queries in the test expected the same SQL query.

Update FetchRequest protocol in README

207b90f

Update inline docs

088f6c9

groue mentioned this pull request Apr 11, 2019

Record.fetchOne(db) does not add "LIMIT 1" into sql query #513

Closed

groue added the enhancement label Apr 11, 2019

groue reviewed Apr 11, 2019

View reviewed changes

groue added this to the GRDB 4.0.0 milestone Apr 11, 2019

groue mentioned this pull request Apr 11, 2019

GRDB 4.0 #479

Merged

26 tasks

groue reviewed Apr 11, 2019

View reviewed changes

groue requested changes Apr 12, 2019

View reviewed changes

alextrob added 5 commits April 16, 2019 10:20

Add failing test for fetchOne with a non-zero offset

20aecd5

Replace FetchRequestHint with a boolean for single results

7070ded

Fixes previous test failure

Remove LIMIT 1 from expected SQL when filtering by key

235a612

Tests now fail

Update FetchRequest protocol in README

1b3f0e8

groue added 2 commits April 16, 2019 07:56

Remove QueryInterfaceRequest.init(query:)

46791c8

Bake the single result expectation right into filter(key:) methods

4716f91

This better hides this flag inside SQLSelectQuery, and avoids explicit support from FetchableRecord and TableRecord

groue added 2 commits April 17, 2019 13:08

Do not test that fetchOne does not add LIMIT 1 to raw SQL requests

693e6da

We do not add LIMIT 1 to raw SQL requests because it is difficult. But it would not be wrong to do so.

Add SQL tests for all record testFetchOne…() methods

e3a0935

We usually update all those tests together

groue approved these changes Apr 17, 2019

View reviewed changes

groue added 3 commits April 17, 2019 21:31

Tests for the singleResult hint and fetching methods

65fe2bb

Tests for the singleResult hint and fetchCount

21c0175

Tests for the singleResult hint and databaseRegion

a128300

groue added 4 commits April 17, 2019 21:32

Tests for the singleResult hint and SQLRequest.init(_:request:)

3f8047f

CHANGELOG

a172473

Merge branch 'GRDB-4.0' into feature/fetchOne-limit-1

2711b9b

Thank you notice

5af1686

groue merged commit bdf8d0b into groue:GRDB-4.0 Apr 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "LIMIT 1" to `fetchOne` requests #515

Add "LIMIT 1" to `fetchOne` requests #515

alextrob commented Apr 11, 2019 •

edited

Loading

groue Apr 11, 2019 •

edited

Loading

groue Apr 11, 2019

groue Apr 11, 2019

alextrob Apr 11, 2019

groue Apr 12, 2019

alextrob Apr 16, 2019

groue Apr 11, 2019 •

edited

Loading

alextrob Apr 11, 2019 •

edited

Loading

groue Apr 12, 2019 •

edited

Loading

groue Apr 12, 2019 •

edited

Loading

alextrob Apr 15, 2019

groue Apr 15, 2019 •

edited

Loading

alextrob Apr 16, 2019

groue Apr 12, 2019

alextrob Apr 16, 2019

alextrob commented Apr 16, 2019

groue commented Apr 16, 2019

alextrob commented Apr 16, 2019

groue commented Apr 17, 2019

groue commented Apr 18, 2019

alextrob commented Apr 18, 2019

groue commented Apr 18, 2019

Add "LIMIT 1" to fetchOne requests #515

Add "LIMIT 1" to fetchOne requests #515

Conversation

alextrob commented Apr 11, 2019 • edited Loading

Pull Request Checklist

groue Apr 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

groue Apr 11, 2019 • edited Loading

Choose a reason for hiding this comment

alextrob Apr 11, 2019 • edited Loading

Choose a reason for hiding this comment

groue Apr 12, 2019 • edited Loading

Choose a reason for hiding this comment

groue Apr 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

groue Apr 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alextrob commented Apr 16, 2019

groue commented Apr 16, 2019

alextrob commented Apr 16, 2019

groue commented Apr 17, 2019

groue commented Apr 18, 2019

alextrob commented Apr 18, 2019

groue commented Apr 18, 2019

Add "LIMIT 1" to `fetchOne` requests #515

Add "LIMIT 1" to `fetchOne` requests #515

alextrob commented Apr 11, 2019 •

edited

Loading

groue Apr 11, 2019 •

edited

Loading

groue Apr 11, 2019 •

edited

Loading

alextrob Apr 11, 2019 •

edited

Loading

groue Apr 12, 2019 •

edited

Loading

groue Apr 12, 2019 •

edited

Loading

groue Apr 15, 2019 •

edited

Loading