SQL Builder for CREATE TABLE

[cfilipov]

Continuing the CREATE TABLE part of the discussion from issue #73...

Let's compare our two present options, last time we attempted at writing actual code:

I abandoned the idea of the single expression builder in favor of the closure-based one you proposed. Like you said, it is nice, but it has quite a few drawbacks. I also noticed the closure approach benefits from better auto-completion in Xcode.

I was planning to give the primary key columns here, and you answered "but remember that you also need to support the optional conflict clause when you specify a primary key" - OK no primary key columns in this method. Conflict clauses on primary keys are rare, but useful: let's keep them in the scope of our DSL.

Happy to hear agreement on that.

struct SQLTableBuilder {
    func column(_ name: String, _ type: SQLType, ...)
}

 enum SQLType: String {
    case text = "TEXT"
    case integer = "INTEGER"
    case double = "DOUBLE"
    case numeric = "NUMERIC"
    case boolean = "BOOLEAN"
    case blob = "BLOB"
    case date = "DATE"
    case datetime = "DATETIME"
}

db.create(table: "person") { t in
    t.column("name", .text)
    t.column("age", .integer)
}

Minor point: if you want to support VARCHAR(X) and similar types, this enum could not use raw values but would have to have an associated value and a corresponding helper function to make the string:

enum _SQLType {
    case text
    case integer
    case varchar(Int)
    ...
}

extension _SQLType {
    var sql: String {
        switch self {
        case text: return "TEXT"
        ...
        case let varchar(x):
            precondition(x <= 255)
            return "VARCHAR(\(x))"
        }
    }
}

Tangential question.. why use the SQL... prefix all over GRDB? This is all defined in a package which is known to be in the context of working with SQL. It's unlikely for Column to have any other meaning within GRDB so the SQL prefix doesn't seem necessary and only serves to make the types more verbose. If you really wanted to separate some parts of the lib you could either create a sub module in the module map to import GRDB.SQL (or just import GRDB and use the types like SQL.Column) or use a caseless enum as a namespace so you can have SQL.Column types as well etc... I don't feel too strongly about this, but I do think it helps keep code more readable by not having the types repeat terms.

Not naming the parameters of column(::) contradicts a few Swift 3 API guidelines, but the very first one: clarity at the point of use.

I think dropping the name label but keeping the type label would be even more clear:

t.column("age", type: .integer)

By the way, I would make SQLColumn a StringLiteralConvertible and change the function definition to accept a column:

struct SQLTableBuilder {
    func column(_ name: SQLColumn, type: SQLType, ...)
}

Any reason not to do this?

Your initial idea about unicity was fluent: column(...).unique(onConflict: .rollback). Not bad, but we can't be fluent at some places, and use a parameter elsewhere, when dealing with the two very similar constructs that are NOT NULL and UNIQUE.
...
Should we eventually pick a fluent interface, the nullability should be updated accordingly:

100% agree. The fluent version is more flexible in what it can express grammar-wise. You can easily overload the fluent methods to provide different versions with different defaults. And as you said, it makes sense to keep similar constructs together, unique and notNull are similar, but one of them cannot be represented by just a parameter, so let's make them all fluent methods.

Easier said than done, though: just like the CHECK constraints you have already studied, ? placeholders aren't accepted in default clauses :-(. Well, I guess we'll have to perform our own escaping... Do you see another option?

Exactly. Unfortunately I don't see a way around performing our own escaping. I was hoping you would reveal some amazing alternative I had not thought of. One option, if you are worried about safety/injection, is to only allow literal convertible types in these types of expressions (prevent the user from accidentally using external input of some kind during table creation), though I can also see this being severely limiting (think of the SQL Browser app example).

Either way, the expression builder will have to be updated (or a new, separate one made) in order to support native expressions in CHECK and DEFAULT.

The actual SQLite grammar accepts CURRENT_TIME, CURRENT_DATE and CURRENT_TIMESTAMP as default values. OK, seriously, do we really have to support them, when inserting a date created in Swift has the same effect? Not supporting them lets us use the simple DatabaseValueConvertible protocol, and throw any value in. Should we support the special literals, we'd have to write t.column("date", .datetime, default: .value("Anonymous")) just for t.column("date", .datetime, default: .current_time) :-/

One other problem with CURRENT_TIME, CURRENT_DATE and CURRENT_TIMESTAMP: If you use DatabaseValueConvertible it's possible you implemented your own format for storing dates (which you of course documented), how would those literals work in such cases? I think using DatabaseValueConvertible is the best way to go as you suggest, but that does not preclude us from also supporting literal SQL strings too, which could prove very useful.

db.create(table: "person") { t in
    t.column("datetime", type: .datetime)
        .defaults(sql: "CURRENT_TIME") // or whatever SQL string you want
}

db.create(table: "person") { t in
    t.column("datetime", type: .datetime)
        .defaults(Date()) // the preferred way, any _SQLExpressible should work here
}

db.create(table: "person") { t in
    t.column("status", type: .integer)
        .defaults(42) // Swift integer literal, I guess this also counts as _SQLExpressible?
}

raw SQL snippets?

I think it's valuable to allow for raw SQL snippets in certain parts like check(), defaults() and possibly others. The builder pattern and fluent methods make this easy to do without sacrificing anything.

decide on an eventual default id INTEGER PRIMARY KEY

I'm beginning to see some benefit to this, but still not convinced it's a good idea. By making this the default you then have to specify a way to say "this column, which is an integer, is NOT a primary key".

If it's too magical (lets say you only do this on a column if it's int and the first column) then the declaration will look identical to other integer columns that are integer but not primary key. In the end, marking a column as primary key is simple and doesn't require a lot of extra typing, and it's explicit:

try db.create(table: "foo") { t in
    t.column("id", type: .integer) // primary key?
    t.column("bar_id", type: .integer) // not primary key
    t.column("other_id", type: .integer) // not primary key
}

This also gets even more complicated if you want to have an integer column be the first column, but actually want a compound primary key. I think being explicit with primary key is still the most straightforward choice. These are all valid options for creating a table with a primary key:

try db.create(table: "foo") { t in
    t.column("id", type: .integer)
        .primaryKey()
}

try db.create(table: "foo") { t in
    t.column("id", type: .integer)
        .primaryKey(true) // same as previous example, totally pointless but since the previous one used the default param value this is also technically possible. 
}

try db.create(table: "foo") { t in
    t.column("id", type: .integer)
        .primaryKey(onConflict: .rollback)
}

try db.create(table: "foo") { t in
    t.column("id", type: .integer)
        .primaryKey(order: .desc, onConflict: .rollback)
}

try db.create(table: "foo") { t in
    t.column("id", type: .integer)
        .primaryKey(autoincrement: true, onConflict: .rollback)
}

I've pushed some code to my fork. This is very preliminary, I'm going to need guidance on your preferred code organization eventually (everything is in one source file and one test file right now) and other preferences you might have. I'm happy to see that your code snippets above actually look pretty close to what I have written so far.

OK, a little pause. On the remaining list

Columns:

• name SQLType PRIMARY KEY
• name SQLType PRIMARY KEY ASC/DESC
• name SQLType PRIMARY KEY AUTOINCREMENT
• name SQLType CHECK ... *
• name SQLType COLLATE ...
• name SQLType REFERENCES ... ON ...

* I have CHECK pretty much implemented but of course more work needs to be done to properly generate expression strings in this context. The SQL literal form of CHECK is working, however.

I haven't gotten to COLLATE yet but doesn't look very difficult though I haven't looked into how to also support your custom collations (I don't expect any problems).

Table constraints:

• PRIMARY KEY (...)
• PRIMARY KEY (...) ON CONFLICT ...
• UNIQUE KEY (...)
• UNIQUE KEY (...) ON CONFLICT ...
• CHECK
• FOREIGN KEY ...

What about the SELECT clause for table creation? I don't know how often it's used, but it looks easy to implement so I figure why not.

I haven't implemented CHECK as a table constraint yet but that's trivial (disregarding the ? expression problem for a moment).

Other:

• DROP TABLE
• CREATE/DROP INDEX
• ALTER TABLE ADD COLUMN
• covering indexes

I haven't even started on any of these. Most of the should actually be much simpler than CREATE TABLE. I'll start working on them soon.

[cfilipov]

Apple itself is using keywords such as 'in' in argument labels. For example, check "Refined API Naming" section on their website:
https://developer.apple.com/swift/
The label 'in' in the sample code they posted is highlighted as keyword.

@zmeyc I incorrectly assumed that keywords could only be used as argument labels, not for actual function names (@grue and I finally agreed on the fluent method builder approach so default would be a method in this case). Turns out you can use them in function names! Thanks!

db.create(table: "person") { t in
    t.column("datetime", type: .datetime)
        .default(sql: "CURRENT_TIME") // works! and it even highlights correctly in xcode (though github still thinks it's a keyword)!!
}

[groue]

Continuing the CREATE TABLE part of the discussion from issue #73...

Hello @cfilipov, I'm back to business. Sorry for the late reply.

enum SQLType: String {
    case text = "TEXT"
    case integer = "INTEGER"
    case double = "DOUBLE"
    case numeric = "NUMERIC"
    case boolean = "BOOLEAN"
    case blob = "BLOB"
    case date = "DATE"
    case datetime = "DATETIME"
}

db.create(table: "person") { t in
    t.column("name", .text)
    t.column("age", .integer)
}

Minor point: if you want to support VARCHAR(X) and similar types, this enum could not use raw values but would have to have an associated value and a corresponding helper function to make the string.

I don't think supporting VARCHAR is necessary. SQLite would behave the same as TEXT. Plus VARCHAR would be somewhat misleading: VARCHAR(16) is supposed to imply a hard limit of 16 characters (bytes), whereas SQLite just completely ignores this and happily stores strings of all lengths.

I think the list above is not that bad: it covers all SQLite features, and it contains most, if not all, types expected by users. Should anybody ask for a VARCHAR, I'll feel no shame at all answering "use text instead, or write your custom SQL query".

Tangential question.. why use the SQL... prefix all over GRDB? This is all defined in a package which is known to be in the context of working with SQL. It's unlikely for Column to have any other meaning within GRDB so the SQL prefix doesn't seem necessary and only serves to make the types more verbose.

I have tried to prefix with SQL all types that are indeed tied to the SQL language itself.

• SQLExpressible: can be turned into an SQL expression
• SQLColumn: represents an SQL column.

If find it important that user knows he's dealing with SQL itself, the low-level, hard core, dirty language, when he plays with the nice looking query interface. The query interface is only sugar on top of SQL: there's no magic.

Consider this record:

// CREATE TABLE products (
//   priceCents INTEGER -- store price as an exact integer, not a double
// )
struct Product : RowConvertible, TableMapping {
    // `price` property
    var price: NSDecimalNumber
    static let databaseTableName = "products"
    init(row: Row) {
        // `priceCents` column
        let priceCents: Int = row.value(named: "priceCents")
        price = NSDecimalNumber(value: priceCents).multiplying(byPowerOf10: -2)
    }
}

By making "SQL" very very very super over explicit, I expect a tiny psychological push towards the valid state of mind: the correct column is SQLColumn("priceCents"), not SQLColumn("price"):

for product in Product.order(SQLColumn("priceCents").desc).fetch(db) { // SQL
    print(product.price)                                               // property
}

More generally speaking, I generally try to have GRDB identifiers explicitly say their relation to the database. The same for most protocol methods (such as databaseTableName).

For example, Queue, Pool, or Value would be bad names. Those are names that don't mean anything unless written as GRDB.Queue, GRDB.Pool, GRDB.Value. Plus they could easily conflict with user types. The names DatabaseQueue, DatabasePool, DatabaseValue are better.

There are some exceptions, though, like Configuration, which is used in very precise context.

Not naming the parameters of column(::) contradicts a few Swift 3 API guidelines, but the very first one: clarity at the point of use.

I think dropping the name label but keeping the type label would be even more clear:

t.column("age", type: .integer)

That's noise to me.

By the way, I would make SQLColumn a StringLiteralConvertible and change the function definition to accept a column:

struct SQLTableBuilder {
    func column(_ name: SQLColumn, type: SQLType, ...)
}

Any reason not to do this?

Sounds like a good idea, but you're drifting :-)

First, I don't think we can't have SQLColumn adopt StringLiteralConvertible, because expressions like "foo" == "bar" would become ambiguous. Is it "foo" == "bar", SQLColumn("foo") == "bar", "foo" == SQLColumn("bar"), SQLColumn("foo") == SQLColumn("bar")?

Last, what's the point? Remember we must not foster the introduction of external values into migration body, so that it is easy to write good migrations that don't change.

By accepting SQLColumn, you're implying that it's a good idea to share values between migrations and the query interface.

It is not.

[...] just like the CHECK constraints you have already studied, ? placeholders aren't accepted in default clauses :-(. Well, I guess we'll have to perform our own escaping... Do you see another option?

Exactly. Unfortunately I don't see a way around performing our own escaping.

db.create(table: "person") { t in
    t.column("datetime", type: .datetime)
        .defaults(sql: "CURRENT_TIME") // or whatever SQL string you want
}

db.create(table: "person") { t in
    t.column("datetime", type: .datetime)
        .defaults(Date()) // the preferred way, any _SQLExpressible should work here
}

db.create(table: "person") { t in
    t.column("status", type: .integer)
        .defaults(42) // Swift integer literal, I guess this also counts as _SQLExpressible?
}

I buy this: the explicit sql argument, or a value. Why do we need _SQLExpressible? Can't we just use DatabaseValueConvertible, which is the protocol for values?

raw SQL snippets?

I think it's valuable to allow for raw SQL snippets in certain parts like check(), defaults() and possibly others. The builder pattern and fluent methods make this easy to do without sacrificing anything.

OK. This is actually the best argument for the fluent methods.

decide on an eventual default id INTEGER PRIMARY KEY

I'm beginning to see some benefit to this, but still not convinced it's a good idea. By making this the default you then have to specify a way to say "this column, which is an integer, is NOT a primary key".

If it's too magical (lets say you only do this on a column if it's int and the first column) then the declaration will look identical to other integer columns that are integer but not primary key.

That would indeed be a very bad idea.

This also gets even more complicated if you want to have an integer column be the first column, but actually want a compound primary key. I think being explicit with primary key is still the most straightforward choice. These are all valid options for creating a table with a primary key:

try db.create(table: "foo") { t in
  t.column("id", type: .integer)
      .primaryKey()
}

try db.create(table: "foo") { t in
  t.column("id", type: .integer)
      .primaryKey(true) // same as previous example, totally pointless but since the previous one used the default param value this is also technically possible. 
}

This "also technically possible" syntax should be avoided.

OK thanks @zmeyc and @cfilipov.

[groue]

@cfilipov master and Swift3 branches now have an internal DatabaseValueConvertible.sqlLiteral property, which returns the SQL-escaped version of any value:

"foo".sqlLiteral // 'foo'
"'foo'".sqlLiteral // '''foo'''
"foo".data(using: .utf8)!.sqlLiteral // x'666f6f'

The implementation is slow (doesn't matter), but reliable (matters a lot).

[groue]

@cfilipov After DatabaseValueConvertible (good candidate for the argument of the default fluent method), here is support for SQL escaping of _SQLExpressible (good candidate for the expression arguments of the methods that create CHECK constraints:

// CREATE TABLE "users" (
//   "username" TEXT
//     DEFAULT 'Anonymous Coward'
//     CHECK("username" <> 'admin' COLLATE NOCASE)
// )
db.create(table: "users") { t in
    t.column("username", .text)
        .default("Anonymous Coward")
        .check { $0.collating("NOCASE") != "admin" }
}

The default function eats DatabaseValueConvertible, and check yields a closure whose argument is SQLColumn, and result _SQLExpressible.

Sample code:

// Turn value into SQL literal:
let string = "Anonymous Coward"
string.sqlLiteral   // 'Anonymous Coward'

// Turn expression into SQL literal (note that a Database instance is required,
// because some expressions need one in order to generate SQL.)
let column = SQLColumn("username")
let expressible: _SQLExpressible = column.collating("NOCASE") != "admin"
let expression = expressible.sqlExpression
// nil arguments makes the expression embed raw literals in the SQL (instead of `?` placeholders):
var arguments: StatementArguments? = nil
try! expression.sql(db, &arguments) // "username" <> 'admin' COLLATE NOCASE

[groue]

... note that a Database instance is required, because some expressions need one in order to generate SQL.

This used to be true, but no longer. I've removed dead code, and now converting _SQLExpressible to an SQL string reads:

let column = SQLColumn("username")
let expressible: _SQLExpressible = column.collating("NOCASE") != "admin"

let expression = expressible.sqlExpression
var arguments: StatementArguments? = nil
expression.sql(&arguments) // "username" <> 'admin' COLLATE NOCASE

[cfilipov]

Welcome back @groue!

I don't think supporting VARCHAR is necessary. SQLite would behave the same as TEXT. Plus VARCHAR would be somewhat misleading: VARCHAR(16) is supposed to imply a hard limit of 16 characters (bytes), whereas SQLite just completely ignores this and happily stores strings of all lengths.

This sounds reasonable to me.

I have tried to prefix with SQL all types that are indeed tied to the SQL language itself.

I don't feel strongly about this. I tend to prefer shorter more concise names when possible but your explanation makes sense. I'll follow the same naming convention you described.

I think dropping the name label but keeping the type label would be even more clear:

t.column("age", type: .integer)

That's noise to me.

I think it's strange Swift coding style to leave out only the first two param arguments and have the rest named. Typically the first argument is dropped when the context is clear from the function name. Other times all argument labels are dropped when their meaning is clear. But I have not seen this done this way.

That said, I don't think it's too bad. This is one area where I wish more people would be able to provide input instead of just being you vs me.

By the way, I would make SQLColumn a StringLiteralConvertible and change the function definition to accept a column:

...

Last, what's the point? Remember we must not foster the introduction of external values into migration body, so that it is easy to write good migrations that don't change.

By accepting SQLColumn, you're implying that it's a good idea to share values between migrations and the query interface.

Accepting values as SQLColumn doesn't imply this any more than accepting a String. You can just as easily share a string with the migration, no difference here.

I was thinking the main benefit of taking a SQLColumn as a parameter would be the ability to validate the column string before building the statement, though that can be just as easily done in the builder itself. Another reason it better clarity in the API. Seeing SQLColumn param tells the user more about the intent than String as the type.

Exactly. Unfortunately I don't see a way around performing our own escaping.

I buy this: the explicit sql argument, or a value. Why do we need _SQLExpressible? Can't we just use DatabaseValueConvertible, which is the protocol for values?

In addition to literal values, DEFAULT also accepts an expression. I suppose a use case would be using some built-in function in the default clause, or something.

That would indeed be a very bad idea.

👍

This "also technically possible" syntax should be avoided.

I agree it's unnecessary, though it isn't unclear or confusing. To be clear, I'm not trying to support this specific use-case, I just noticed it was possible, it could be something you just don't mention in the docs. This is simply a side effect of the fact that this was implemented with default param values and you can still provide the argument with the default (see the code in my fork for details). It might be possible to find a way to make this type of usage impossible, but I wonder if it's worth it (if it means complicating anything else).

@cfilipov master and Swift3 branches now have an internal DatabaseValueConvertible.sqlLiteral property, which returns the SQL-escaped version of any value:

Very cool, I'll update my fork to make use of this.

The implementation is slow (doesn't matter), but reliable (matters a lot).

I agree the performance probably doesn't matter very much, especially for something like generating statements for creating tables which should not be a hot path.

@cfilipov After DatabaseValueConvertible (good candidate for the argument of the default fluent method), here is support for SQL escaping of _SQLExpressible (good candidate for the expression arguments of the methods that create CHECK constraints:

Thanks for this! I'll integrate this into the builder as well.

The default function eats DatabaseValueConvertible, and check yields a closure whose argument is SQLColumn, and result _SQLExpressible.

See my previous comment regarding expressions in DEFAULT. Do you want to only allow values despite it accepting expressions? Since it's already possible to handle expressions because of CHECK, I see no reason why we shouldn't allow it in DEFAULT.

... note that a Database instance is required, because some expressions need one in order to generate SQL.

This used to be true, but no longer. I've removed dead code, and now converting _SQLExpressible to an SQL string reads:

Ah, that explains it. I was wondering why db was passed around when building queries, but I couldn't see where it was used.

I'm looking forward to integrating your latest feedback into the fork and moving this forward. I'll probably do it this weekend.

[groue]

I think dropping the name label but keeping the type label would be even more clear [...]

I think it's strange Swift coding style to leave out only the first two param arguments and have the rest named. Typically the first argument is dropped when the context is clear from the function name. Other times all argument labels are dropped when their meaning is clear. But I have not seen this done this way.

That said, I don't think it's too bad. This is one area where I wish more people would be able to provide input instead of just being you vs me.

I thus think it will be me.

We're writing a DSL with very limited scope: table creation/update. This means that we have much more implicit context than usual APIs, and this is an advantage that allows us to escape a strict application of the language guidelines.

Look: don't you agree that the second version below has better legibility?

db.create(table: "persons") { t in
    t.column("id", type: .integer).primaryKey()
    t.column("name", type: .text)
    t.column("email", type: .text).unique().collate(.noCase)
    t.column("birthdate", type: .date)
}

db.create(table: "persons") { t in
    t.column("id", .integer).primaryKey()
    t.column("name", .text)
    t.column("email", .text).unique().collate(.noCase)
    t.column("birthdate", .date)
}

// CREATE TABLE persons (
//   id INTEGER PRIMARY KEY,
//   name TEXT,
//   email TEXT UNIQUE COLLATE NOCASE,
//   birthdate DATE
// )

BTW, the primaryKey() fluent method makes it very natural to define an INTEGER PRIMARY KEY, as above: I think we can drop the automatic primary key I used to suggest and that you didn't like much. Thanks for having pushed the primaryKey() method and generally the fluent pattern: you have made the API more consistent, explicit, and simple.

I was thinking the main benefit of taking a SQLColumn as a parameter would be the ability to validate the column string before building the statement, though that can be just as easily done in the builder itself. Another reason it better clarity in the API. Seeing SQLColumn param tells the user more about the intent than String as the type.

Those are good arguments, yet: since SQLColumn can not adopt StringLiteralConvertible because of the implied ambiguity of "foo" == "bar", we'd have to write:

t.column(SQLColumn("name"), .text)

Since the code above is too verbose, we'd have to add a String overload:

t.column("name", .text)

Thus the priority one method takes a String: this is the method we can focus on right now. And I think that validating the column string is overkill, when SQLite will do it for us. GRDB already provides very verbose errors that help users fix their mistakes:

// fatal error: SQLite error 1 with statement
// `CREATE TABLE foo(bar+baz TEXT)`: near "+": syntax error
try! dbQueue.inDatabase { db in
    try db.execute("CREATE TABLE foo(bar+baz TEXT)")
}

In addition to literal values, DEFAULT also accepts an expression. I suppose a use case would be using some built-in function in the default clause, or something.

[...]

Do you want to only allow values despite it accepting expressions? Since it's already possible to handle expressions because of CHECK, I see no reason why we shouldn't allow it in DEFAULT.

You're totally right:

CREATE TABLE foo (year INTEGER DEFAULT (STRFTIME('%Y', 'NOW')))
INSERT INTO foo DEFAULT VALUES
SELECT * FROM foo -- year: 2016

So default() should eat _SQLExpressible, with the overload you have already suggested that eats raw sql, right?

A good opportunity to promote _SQLExpressible to a fully public protocol, without underscore:

func default(_ expression: SQLExpressible)
func default(sql: String)
func check(_ expression: SQLExpressible)
func check(sql: String)

I was wondering why db was passed around when building queries, but I couldn't see where it was used.

It was a bad idea that has been reverted since: Person.all.reversed() used to sort by reversed primary key, and needed a database connection to inspect the database schema. Now Person.all.reversed() sorts by reversed rowID - and schema introspection is no longer needed. When fixing this I forgot to remove the database argument.

[groue]

I've pushed some code to my fork.

Great!

cfilipov@53f0697#diff-1dba96091b9e9bac5ff1cdaa9d5f572bR115

public class _ColumnDef: BuilderType {

OK, _ColumnDef is a mutable class, that's what I was expecting.

cfilipov@53f0697#diff-1dba96091b9e9bac5ff1cdaa9d5f572bR61

public class _CreateTableStatement: BuilderType {
    @discardableResult public func column(_ column: SQLColumn, type: _SQLType? = nil) -> _ColumnDef

Here you're allowing to create a column without any type. Yes, SQLite allows this (with a NUMERIC default affinity, as you've shown me earlier). Yet I'm not sure that users expect to be able to create a column without any explicit type. This default nil argument may disturb them. Since missing type is equivalent to an explicit NUMERIC type, our API does not need to support it. Thus it's better not to support it at all:

public class _CreateTableStatement: BuilderType {
    // non optional type
    @discardableResult public func column(_ column: SQLColumn, type: _SQLType) -> _ColumnDef
}
public class _ColumnDef: BuilderType {
    // non optional type
    private var type: _SQLType
}

Our goal is to help people creating tables. Not to generate all possible CREATE TABLE statements.

BTW _SQLType is a type we can commit to maintain in the long term: we can name it SQLType without underscore.

cfilipov@53f0697#diff-1dba96091b9e9bac5ff1cdaa9d5f572bR116, cfilipov@53f0697#diff-1dba96091b9e9bac5ff1cdaa9d5f572bR245, cfilipov@53f0697#diff-1dba96091b9e9bac5ff1cdaa9d5f572bR286

public class _ColumnDef: BuilderType {
    private var column: SQLColumn
...
    @discardableResult public func references(...,
        columns: SQLColumn...
...
private struct _PrimaryKeyTableConstraint: _TableConstraint {
   var columns: [SQLColumn] 

Maybe we'll expose SQLColumn in public APIs, but surely we only need strings in implementation. I'd rather use only Strings until we find a compelling argument for SQLColumn. That's just plain KISS.

cfilipov@53f0697#diff-1dba96091b9e9bac5ff1cdaa9d5f572bR286, cfilipov@53f0697#diff-1dba96091b9e9bac5ff1cdaa9d5f572bR199

@discardableResult public func primaryKey(autoincrement: Bool, onConflict: _ConflictResolution? = nil) -> _ColumnDef
@discardableResult public func primaryKey(order: _Order? = nil, onConflict: _ConflictResolution? = nil) -> _ColumnDef    

According to the grammar, we can mix ORDER and AUTOINCREMENT:

id INTEGER PRIMARY KEY DESC AUTOINCREMENT

Shouldn't we thus write:

@discardableResult public func primaryKey(order: SQLOrder? = nil, onConflict: SQLConflictResolution? = nil, autoincrement: Bool = false) -> _ColumnDef

// id INTEGER PRIMARY KEY
t.column("id", .integer).primaryKey()

// id INTEGER PRIMARY KEY DESC
t.column("id", .integer).primaryKey(order: .desc)

// id INTEGER PRIMARY KEY AUTOINCREMENT
t.column("id", .integer).primaryKey(autoincrement: true)

// id INTEGER PRIMARY KEY ON CONFLICT ABORT
t.column("id", .integer).primaryKey(onConflict: .abort)

// id INTEGER PRIMARY KEY DESC ON CONFLICT ABORT AUTOINCREMENT
t.column("id", .integer).primaryKey(order: .desc, onConflict: .abort, autoincrement: true)

Or did I miss something?

(note the _Order renaming to SQLOrder, and _ConflictResolution to SQLConflictResolution)

cfilipov@53f0697#diff-1dba96091b9e9bac5ff1cdaa9d5f572bR211, cfilipov@53f0697#diff-1dba96091b9e9bac5ff1cdaa9d5f572bR223,
cfilipov@53f0697#diff-1dba96091b9e9bac5ff1cdaa9d5f572bR233

@discardableResult public func primaryKey(_ enabled: Bool) -> _ColumnDef
@discardableResult public func notNull(_ enabled: Bool = true) -> _ColumnDef
@discardableResult public func unique(_ enabled: Bool = true) -> _ColumnDef

What's the purpose of those methods, especially considering the above comment?

cfilipov@53f0697#diff-1dba96091b9e9bac5ff1cdaa9d5f572bR269, cfilipov@53f0697#diff-1dba96091b9e9bac5ff1cdaa9d5f572bR276

extension Collection where Iterator.Element == _ColumnDef
extension Collection where Iterator.Element == _TableConstraint

Ha, ha, looks like the BuilderType protocol has not much utility, after all ;-)

Seriously, if BuilderType has no use, I'd rather go without it.

cfilipov@53f0697#diff-1dba96091b9e9bac5ff1cdaa9d5f572bR119

private var notNullClause: _BoolOr<_ConflictClause>?
private var uniqueClause: _BoolOr<_ConflictClause>?

The problem here is that your private implementation details are a direct copycat of your public API.

// Public API:
t.column("name", .text).notNull()
t.column("name", .text).notNull(true)
t.column("name", .text).notNull(false)
t.column("name", .text).notNull(onConflict: .rollback)

You should decouple them more, so that you can freely criticize your implementation and your public API independently. That's the best way to improve both of them in the long term.

Implementation-wise, what we strictly need to implement is the generation of the empty string, or NOT NULL ON CONFLICT xxx. NOT NULL without conflict clause is not required: we can use the default ABORT conflict resolution algorithm when the user does not specify it. Thus the minimal required implementation is:

/// If not nil, the column is NOT NULL
private var notNullConflictResolution: SQLConflictResolution?
/// If not nil, the column is UNIQUE
private var uniqueConflictResolution: SQLConflictResolution?

If we care about the quality of the generated SQL (and I do actually), we could even generate a simple NOT NULL without conflict clause when resolution is .abort, instead of NOT NULL ON CONFLICT ABORT.

Now the public API could also be simplified:

• notNull(false) is redundant with no call to notNull
• notNull(true) is redundant with notNull()

The minimal public API is thus:

@discardableResult public func notNull(onConflict: SQLConflictResolution? = nil) -> _ColumnDef

t.column("name", .text).notNull()
t.column("name", .text).notNull(onConflict: .rollback)

(It now happens that the public API and the private implementation use the same SQLConflictResolution? type, but that's a coincidence).

OK I stop here for now. You've already done a great amount of work!

[groue]

The problem here is that your private implementation details are a direct copycat of your public API.

I now also understand why you sometimes feel somewhat frustrated when I only comment the surface of your efforts, and seem like I don't care about the implementation. Both of them are of the highest interest to me, as a matter of fact - but they are nearly totally independent in my mind, especially in our current topic.

We're here to help people building their damned tables and move on. A direct consequence is that the public API has to be conservative and boring, and not smart at all. That may sound like a dull goal, but the service we're working on requires that. Nobody wants to spend much time writing the code that creates table, and the fastest it is written the better it is. The learning curve should be very flat: a quick look at the doc should be enough. I thus don't expect any implementation breakthrough to revolutionize the public API: the public API will influence the implementation, not the opposite.

BTW I've finally dived with you into the code, and I hope you're relieved :-)