Ordering of order clauses (was: Expose initializer for _SQLExpressible, _SQLOrdering)

[palpatim]

Thanks for all your work on this module--it's really great work so far.

One thing that would be handy is to expose an API for constructing _SQLExpressible and _SQLOrdering instances. Right now, if we want to, say, cache a commonly-used predicate for a table, we have to interpolate column names into a String. That loses type safety and adds syntactic cruft, and in general doesn't feel Swifty. In addition, it breaks the feel of the query interface because now instead of crafting a predicate, I'm passing in a String--I might as well use hand-crafted SQL at that point. :)

Example:

// Stringy
let baseSearchPredicate = "\(MyTable.Columns.objectKind) = 1"
let defaultOrder = "\(MyTable.Columns.createdDate) DESC"

Preferred:

// Swifty
let baseSearchPredicate = SQLFilterClause(MyTable.Columns.objectKind == 1)
let defaultOrder = SQLOrderClause(MyTable.Columns.createdDate.desc)

It's very possible I'm just missing something in the API, in which case, please feel free to disregard this message.

[groue]

Hello @palpatim. I'm in vacation now, so I won't be able to answer your question until next week.

[groue]

I'm not sure I understand your problem. Why don't you just write:

// MyTable.Columns.objectKind = SQLColumn("objectKind")
// MyTable.Columns.createdDate = SQLColumn("createdDate")
let baseSearchPredicate = MyTable.Columns.objectKind == 1
let defaultOrder = MyTable.Columns.createdDate.desc

// And there we go:
let records = MyTable.filter(baseSearchPredicate).order(defaultOrder).fetchAll(db)

Pretty much Swifty enough to me. Have a detailed look at https://github.com/groue/GRDB.swift#the-query-interface.

I advise you not to use the underscore-prefixed types in your code. Values are OK, as in the sample code above. But don't write the following:

// WRONG: future versions of GRDB may break this code
let baseSearchPredicate: _SQLExpression = MyTable.Columns.objectKind == 1
let defaultOrder: _SQLSortDescriptor = MyTable.Columns.createdDate.desc

// GOOD
let baseSearchPredicate = MyTable.Columns.objectKind == 1
let defaultOrder = MyTable.Columns.createdDate.desc

[palpatim]

Thanks for the response, Gwendal.

Your suggestion works at the declaration/assignment site, but doesn't allow us to declare parameter types so that we can pass the values around outside of their declaration scope. (I apologize for not including that in my original use case statement.) What I was hoping to achieve was something like:

let baseSearchPredicate = MyTable.Columns.objectKind == 1 // baseSearchPredicate is _SQLExpression
let defaultOrder = MyTable.Columns.createdDate.desc

func doQuery(predicate: ???, order: ???) -> [MyTable]? {
    let request = MyTable.filter(predicate && baseSearchPredicate).order(order, defaultOrder)
    return dbQueue.inDatabase { db in
        return MyTable.fetchAll(db, request)
    }
}

But it looks like the right way to do this is to compose QueryInterfaceRequest<T> together in the query. To use the Person example from the "Record" playground, I might have a default-able query method like:

class Person : Record {
    var id: Int64?
    var firstName: String?
    var lastName: String?

    struct Columns {
        static let id = SQLColumn("id")
        static let firstName = SQLColumn("firstName")
        static let lastName = SQLColumn("lastName")
    }

    static let baseSearchPredicate = Columns.firstName != nil && Columns.lastName != nil
    static let defaultOrder = Columns.id.asc

    static func doQuery(dbQueue: DatabaseQueue, withRequest suppliedRequest: QueryInterfaceRequest<Person>) -> [Person]? {
        let request = suppliedRequest.filter(baseSearchPredicate).order(defaultOrder)
        return dbQueue.inDatabase() {
            db in
            return Person.fetchAll(db, request)
        }
    }

   ...
}

And invoke it like:

let specificRequest = Person.filter(Person.Columns.firstName == "Arthur" || Person.Columns.firstName == "Cinderella").order(Person.Columns.firstName)
let results = Person.doQuery(dbQueue, withRequest: specificRequest)

// SELECT * FROM "persons" WHERE ((("firstName" = 'Arthur') OR ("firstName" = 'Cinderella')) AND (("firstName" IS NOT NULL) AND ("lastName" IS NOT NULL))) ORDER BY "firstName", "id" ASC

Does that seem right? If so, and we should work with QueryInterfaceRequests, the next question is: how can we compose them? It seems like it should be possible, since we can do Person.filter(...).filter(...), but I can't see how to compose them to accomplish something like:

let request = Person.filter(Person.Columns.firstName == "Arthur" || Person.Columns.firstName == "Cinderella").filter(getBaseFilter())

Where getBaseFilter() more or less takes the place of the declared predicate & order above, but evaulates some runtime conditions to return an appropriate QueryInterfaceRequest<Person>

[groue]

Hello again @palpatim

Thanks for the extra details.

What I was hoping to achieve was something like:

let baseSearchPredicate = MyTable.Columns.objectKind == 1 // baseSearchPredicate is _SQLExpression
let defaultOrder = MyTable.Columns.createdDate.desc

func doQuery(predicate: ???, order: ???) -> [MyTable]? {
    let request = MyTable.filter(predicate && baseSearchPredicate).order(order, defaultOrder)
    return dbQueue.inDatabase { db in
        return MyTable.fetchAll(db, request)
    }
}

Indeed, the types for predicate and order in this function are today semi-private: filter() wants _SQLExpressible, and order() wants _SQLOrdering.

So you can't write this method today.

But it looks like the right way to do this is to compose QueryInterfaceRequest together in the query [...]

static func doQuery(dbQueue: DatabaseQueue, withRequest suppliedRequest: QueryInterfaceRequest<Person>) -> [Person]? {
    let request = suppliedRequest.filter(baseSearchPredicate).order(defaultOrder)
    return dbQueue.inDatabase() {
        db in
        return Person.fetchAll(db, request)
    }
}
let specificRequest = Person.filter(Person.Columns.firstName == "Arthur" || Person.Columns.firstName == "Cinderella").order(Person.Columns.firstName)
let results = Person.doQuery(dbQueue, withRequest: specificRequest)

Yes, this is an option that you have today. baseSearchPredicate is added as an AND clause, and ordering of those does not matter. defaultOrder is added last, and thus it applies after the specific ordering, and this sounds sensible. In the above example, results are sorted by first name, as expected.

Yet my experience of other ORMs makes me think that this pattern is odd. Something is wrong.

Usually, in other ORMs, you have a base request that you extend:

// The default request
let defaultPersons = Person.filter(defaultPredicate).order(defaultOrder)

// Later
let request1 = defaultPersons.filter(filter1).order(ordering1)
let results1 = dbQueue.inDatabase { request1.fetchAll($0) }

let request2 = defaultPersons.filter(filter2).order(ordering2)
let results2 = dbQueue.inDatabase { request2.fetchAll($0) }

But the pattern above fails with ordering: the default ordering is applied first, unlike your example. The famous Ruby ORM Active Record uses a reorder method just for this purpose:

// Some constant
let defaultPersons = Person.filter(defaultPredicate).order(defaultOrder)

// Later
let request1 = defaultPersons.filter(filter1).reorder(ordering1)  // reorder
let results1 = dbQueue.inDatabase { request1.fetchAll($0) }

I kind of believe that this would be still better than the proposed doQuery() method, even if the default ordering does not automatically apply.

Your question is interesting, I'll have to consider this topic a little further.

Meanwhile, may I suggest you to change the way you use DatabaseQueue? I know that it's tempting to wrap the dbQueue.inDatabase { ... } in higher-level methods. Actually GRDB used to offer such APIs: once Person.fetchAll(dbQueue) was possible. But there are real issues with this pattern:

1. DatabaseQueue methods are not reentrant (on purpose), so you won't be able to group your doQuery method with other related fetching methods whenever you want to perform a complex database operation:

   func myComplexOperation(dbQueue: DatabaseQueue) {
       dbQueue.inTransaction { db in
           persons = Person.doQuery(dbQueue, ...) // Crash: forbidden reentrancy
           pets = Pet.doQuery(dbQueue, ...)
           for (person, pet) in zip(persons, pet) { person.feed(pet) }
           return .Commit  // Sometimes you really need that.
       }
   }

   A better pattern is:

   // Take Database instead of DatabaseQueue
   static func doQuery(db: Database, withRequest suppliedRequest: QueryInterfaceRequest<Person>) -> [Person]? {
       let request = suppliedRequest.filter(baseSearchPredicate).order(defaultOrder)
       return Person.fetchAll(db, request)
   }
   
   func myComplexOperation(dbQueue: DatabaseQueue) {
       dbQueue.inTransaction { db in
           persons = Person.doQuery(db, ...)   // OK now
           pets = Pet.doQuery(db, ...)
           for (person, pet) in zip(persons, pet) { person.feed(pet) }
           return .Commit
       }
   }

2. As seen above, the doQuery method can not be grouped with other database methods. This is dangerous for your application if it is multi-threaded.

   Basically, here is your problem:

   // Those arrays may differ
   let persons1 = Person.doQuery(dbQueue, ...)
   let persons2 = Person.doQuery(dbQueue, ...)

   Generally speaking, as two database queries run one after the other, a concurrent thread can sneak in and modify the database in between. The two queries can thus perform inconsistent fetches or updates. Maybe we will display funny values on the screen. Or face a relational constraint error. Worse, we can have a silent application model corruption.

   By exposing a Database (not a DatabaseQueue) in your doQuery method, you'll naturally avoid this nasty problem:

   dbQueue.inDatabase { db in
       // GRDB guarantees that they are the same:
       let persons1 = Person.doQuery(db, ...)
       let persons2 = Person.doQuery(db, ...)
   }

   For more information, read https://medium.com/@gwendal.roue/four-different-ways-to-handle-sqlite-concurrency-db3bcc74d00e

[palpatim]

Thanks again for your reply, @groue. I agree with you about not being completely happy with the doQuery implementation. I await your further thoughts on this.

In the meantime, I'll re-evaluate my use of DatabaseQueue in light of your comments. (I've actually read the concurrency section in the docs, honest!) Thanks for the explanation.

[groue]

First let's make a strong point: it's better to refine existing requests than process them in a non-extensible method.

Thus:

// Works, but with ad-hoc method that performs hidden request transformation
let persons = Person.doQuery(db, filter, order, ...)

// Better, no ad-hoc method, no hidden request transformation, just request derivation
let baseRequest = Person...
let specificRequest = baseRequest.filter(...)...
let persons = dbQueue.inDatabase { request.fetchAll($0) }

Now let's get specific. Here is the Active Record situation:

class Person < ActiveRecord::Base
end

let basePersons = Person.order(:last_name, :first_name)
basePersons                # ordered by last_name & first_name
basePersons.order(:age)    # likely a bug: ordered by age *after* last_name & first_name
basePersons.reorder(:age)  # ordered by age

I don't like the reorder method at all. It is easily overlooked. When you forget it, the innucuous piece of code defaultPersons.order(:age) gives you the wrong list, and one can spend quite a long time understanding the issue first, and find the needed reorder method next after. Frustration guaranteed.

Yet it is a working solution for anyone that has enough experience with Active Record. And I could not find any alternative or critic of reorder in the Rails bug tracker archive. Something is at work here, and it may be subtle to understand why reorder is maybe the best solution.

Could we improve on it?

What about changing the order method so that it prepends ordering clauses, instead of appending them?

// SELECT * FROM persons ORDER BY lastName, firstName
let baseRequest = Person.order(lastNameColumn, firstNameColumn)

// SELECT * FROM persons ORDER BY age, lastName, firstName
let specificRequest = baseRequest.order(ageColumn)

This solution has many problems. First, it is surprising, because this is not how ORMs usually work (and one should never surprise a developer). Second, it makes the two following lines of code non-equivalent, when they should be (I see room for personal taste here):

let request = Person.order(lastNameColumn).order(ageColumn)
let request = Person.order(lastNameColumn, ageColumn)

OK so what about splitting ordering clauses in two sets, along the major/minor axis, or default/specific, or first/last?

// SELECT * FROM persons ORDER BY lastName, firstName
let baseRequest = Person.lastOrder(lastNameColumn, firstNameColumn)

// SELECT * FROM persons ORDER BY age, lastName, firstName
let specificRequest = baseRequest.order(ageColumn)

This would work, but now I feel like it solves the wrong problem: it is questionable that sorting by lastName and firstName is still relevant after the age ordering.

Second try:

// SELECT * FROM persons ORDER BY lastName, firstName
let baseRequest = Person.defaultOrder(lastNameColumn, firstNameColumn)

// SELECT * FROM persons ORDER BY age
let specificRequest = baseRequest.order(ageColumn)

This second try is more interesting: there is a default ordering, but it is cancelled as soon as a "regular" order clause is added. The defaultOrder method would specifically target the requests that are designed to be extended. And I kind of think that it would actually better address your specific needs, @palpatim.

Of course, now, it is the preservation of the default ordering which is difficult:

// Weird way to get SELECT * FROM persons ORDER BY lastName, firstName, age
let specificRequest = baseRequest.defaultOrder(ageColumn)

OK, this was the first exploration of the topic. So far, reorder is still the best solution, despite its shortcomings:

// SELECT * FROM persons ORDER BY lastName, firstName
let baseRequest = Person.order(lastNameColumn).order(firstNameColumn)

// SELECT * FROM persons ORDER BY lastName, firstName, age (likely a bug)
let specificRequest = baseRequest.order(ageColumn)

// SELECT * FROM persons ORDER BY age (unambiguously the desired request)
let specificRequest = baseRequest.reorder(ageColumn)

Thoughts?

[groue]

(I have renamed the issue to what I think is the root of your question - feel free to refocus the issue if I was misled)

[groue]

It is worth noting that Django ORM does not chain order clauses:

Each order_by() call will clear any previous ordering. For example, this query will be ordered by pub_date and not headline:

Entry.objects.order_by('headline').order_by('pub_date')

Considering the sane "explicit is better than implicit" rule of the Python ecosystem, it is a serious option to consider as well.

[groue]

OK I made up my mind, and chose the Django way: each order call clears any previous ordering.

// SELECT * FROM "persons" ORDER BY "name"
Person.order(scoreColumn).order(nameColumn)

Shipped in v0.76.0.

I believe this closes this issue for good.

[palpatim]

Thanks very much, @groue. I appreciate the careful consideration you gave this. While it doesn't necessarily meet my original goal, I agree this is a simpler, less surprising implementation.

[groue]

@palpatim That's why developers who share their questions and goals are awesome! Your input brought a tiny modification with great consequences !