Remove default ColumnAdapters for non-SQLite dialects

[veyndan]

The SQLDelight SQLite documentation showcases the usage of (what is essentially) preinstalled ColumnAdapter's. For example, the following (user-specified column adapters are not required/allowed):

CREATE TABLE some_types (
    some_short INTEGER AS Short,
    som_boolean INTEGER AS Boolean
)

This makes sense for SQLite, where there isn't a SMALLINT that would automatically map to kotlin.Short, or BOOLEAN that would automatically map to kotlin.Boolean. With dialects that do support these types (either through pseudo or actual types), I don't think it makes sense to have these preinstalled adapters for these dialects as it promotes the abuse of using the wrong underlying SQL type (e.g., using INT AS Boolean instead of BOOLEAN in PostgreSQL).

---

I'm a bit hesitant of this proposal, purely because of its implementation. As AlecKazakova/sql-psi#155 advocates for having dialect specific things in their own sql-psi module and making SQLDelight dialect agnostic, it's not obvious where the specification of preinstalled ColumnAdapter's only for SQLite should go:

1. Have the logic reside in the SQLite dialect specific sql-psi module doesn't make sense, as ColumnAdapters are an SQLDelight construct, and doesn't really relate semantically to what sql-psi is trying to solve.
2. Have the logic reside in SQLDelight wouldn't work, because now we're doing dialect specific things in SQLDelight (something we would like to avoid in the future).

Personally, I'd do away with default column adapters, and have the user explicitly hook the adapter up with the database like they do for their own custom ColumnAdapter's. SQLDelight could provide a set of commonly used ColumnAdapter's (e.g., IntColumnAdapter, BooleanColumnAdapter). This idea would mean that non-SQLite dialects would have access to this suite of ColumnAdapter's, but the adapters could ultimately be put into a separate module with a strong suggestion in its description to only use them if the dialect the user is using doesn't have an SQL type that would otherwise return the same Java type they desire. As was said, this wouldn't address the initial issue 100%, but would make doing the wrong thing by accident a bit harder.

Here's a (temporary) backwards compatible prototype of what I was discussing (branch). The final form would look like this (branch). I can make a PR for either one of these if there's interest.

WDYT?

[AlecKazakova]

I think this is a great idea. I would love to have helpful adapters for things like datetime libraries exposed in different artifacts and I think this concept fits nicely with that. Like you could imagine different artifacts for adapters

com.squareup.sqldelight:primitives-adapters
com.squareup.sqldelight:java-time-adapters
com.squareup.sqldelight:postgres-adapters

or whatever.

The boolean one in particular would be rough for existing sqlite users so I'm inclined to keep just that one or think of a migration strategy but I really like the core idea

[veyndan]

I really like the concept of having multiple Gradle dependencies for adapters. It's been a couple of years since I've used SQLDelight for SQLite, so do you mind expanding on why it would be rough for SQLite users to specify the boolean adapter? Granted it would break their builds, but the fix is fairly trivial.

[AlecKazakova]

i guess thats true. I was thinking it would be a bunch of boilerplate that would have to get added in but thats probably okay, and its a compile time thing so its not like we'd be introducing weird errors

[veyndan]

I've been having a look how to implement this and it seems like there would be an issue with the following test as we could no longer guarantee that the same boolean ColumnAdapter is used for both table test and another_test:

sqldelight/sqldelight-compiler/src/test/kotlin/com/squareup/sqldelight/core/views/InterfaceGeneration.kt

Lines 62 to 103 in 5d939ca

	@Test fun `view with exposed booleans through union of separate tables`() {
	val result = FixtureCompiler.compileSql("""
	|CREATE TABLE test (
	| val INTEGER AS Boolean NOT NULL
	|);
	|
	|CREATE TABLE another_test (
	| val INTEGER AS Boolean NOT NULL
	|);
	|
	|CREATE VIEW someView AS
	|SELECT val, val
	|FROM test
	|UNION
	|SELECT val, val
	|FROM another_test;
	|""".trimMargin(), temporaryFolder)
	assertThat(result.errors).isEmpty()
	val generatedInterface = result.compilerOutput.get(
	File(result.outputDirectory, "com/example/SomeView.kt")
	)
	assertThat(generatedInterface).isNotNull()
	assertThat(generatedInterface.toString()).isEqualTo("""
	|package com.example
	|
	|import kotlin.Boolean
	|import kotlin.String
	|
	|data class SomeView(
	| val val_: Boolean,
	| val val__: Boolean
	|) {
	| override fun toString(): String = ""${'"'}
	| |SomeView [
	| | val_: ${"$"}val_
	| | val__: ${"$"}val__
	| |]
	| ""${'"'}.trimMargin()
	|}
	|""".trimMargin())
	}

Essentially what would happen in the above test is what the following test tests for:

sqldelight/sqldelight-compiler/src/test/kotlin/com/squareup/sqldelight/core/queries/InterfaceGeneration.kt

Lines 86 to 118 in 5d939ca

	@Test fun `incompatible adapter types revert to sqlite types`() {
	val file = FixtureCompiler.parseSql("""
	|CREATE TABLE A(
	| value TEXT AS kotlin.collections.List
	|);
	|
	|CREATE TABLE B(
	| value TEXT AS kotlin.collections.List
	|);
	|
	|unionOfBoth:
	|SELECT value, value
	|FROM A
	|UNION
	|SELECT value, value
	|FROM B;
	""".trimMargin(), temporaryFolder)
	val query = file.namedQueries.first()
	assertThat(QueryInterfaceGenerator(query).kotlinImplementationSpec().toString()).isEqualTo("""
	|data class UnionOfBoth(
	| val value: kotlin.String?,
	| val value_: kotlin.String?
	|) {
	| override fun toString(): kotlin.String = ""${'"'}
	| |UnionOfBoth [
	| | value: ${"$"}value
	| | value_: ${"$"}value_
	| |]
	| ""${'"'}.trimMargin()
	|}
	|""".trimMargin())
	}

I can't think of a great solution to this though, but it feels like such a regression might annoy users. The only thing I can think of is having a very stripped down version of PostgreSQL's domain types for dialects that don't have such a feature. Then you can reuse the same "domain type", and we'd know that it has the same shared ColumnAdapter, so the first test would work. Also, as a side benefit, I believe any non-primitive types (e.g., java.time.Date, kotlin.collections.List) would also pass the first test as long as each column has the same "domain type". This would be a pretty major change though, and I'm completely spitballing…

Very rough idea of the syntax:

typealias Day = java.time.LocalDateTime

CREATE TABLE person(
    birthday TEXT AS Day NOT NULL,
    best_day TEXT AS Day NOT NULL
)

Then you'd just need to pass the single ColumnAdapter for the typealias Day instead of the usual passing of identical adapters for birthday and best_day.