Skip to content

Added type mappings tests and docs for ALL supported DBs - #2002

Open
AndreiKingsley wants to merge 12 commits into
masterfrom
jdbc_types_tests_and_docs
Open

Added type mappings tests and docs for ALL supported DBs#2002
AndreiKingsley wants to merge 12 commits into
masterfrom
jdbc_types_tests_and_docs

Conversation

@AndreiKingsley

@AndreiKingsley AndreiKingsley commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Fixes #1736

For each DBType implementation, tests ALL possible sql types metadata -> expected ktype mappings and add info about it in docs.

@zaleslaw

Copy link
Copy Markdown
Collaborator

Link it to the issue or create with the proper description

@AndreiKingsley AndreiKingsley changed the title Jdbc types tests and docs Added type mappings tests and docs for ALL supported DBs Aug 7, 2026
@AndreiKingsley
AndreiKingsley marked this pull request as ready for review August 19, 2026 14:11

## Exact numeric types

| Canonical | Aliases | DataFrame column type | Notes |

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tables need reformatting :)

before being placed into the DataFrame (`DbType.preprocessValue`). For example,
`java.sql.Timestamp` is turned into `kotlin.time.Instant`.

Nullable columns produce nullable Kotlin types (`Int?` instead of `Int`).`

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not every db has a notion of this right? Maybe it helps to say we default to nullable, unless columns are explicitly not nullable

of conversion by supplying a [custom converter](#custom-converters), e.g.

```kotlin
val sqlite = Sqlite.withCustomConverters {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

korro? :)


// 5) CLOB — stored as String.
if ("CLOB" in declaredUpper) {
return jdbcToDfConverterFor<String?>(expectedKType)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can leave out the ? for jdbcToDfConverterFor btw :) nullablity is handled via supplied ktype and the return type automatically turns nullable. It doesn't hurt of course

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but maybe then worth to add a T: Any restriction for jdbcToDfConverterFor?
For consistency.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the current implementation, the ? is taken into account when you don't supply isNullable or a KType, so in some cases it does make sense to use it. However, not all. Maybe we could solve this by creating more overloads in the future :) (or design a better API altogether)

@Jolanrensen Jolanrensen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good overall :D

@jetbrains-air jetbrains-air Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice groundwork here — a couple of blocking spots to sort out first, then this is good to go.

Produced by AIR Automations. Name: Code Review / Run: https://air.jetbrains.cloud/org/05cf1a7f-6ab5-713b-abd3-29d0c8a05e2d/automations/61bd2991-de64-4a10-9895-fd00e7c1f42f?run=9f19003d-4019-41b7-bc81-8eadf4f49d13


// 5) CLOB — stored as String.
if ("CLOB" in declaredUpper) {
return jdbcToDfConverterFor<String?>(expectedKType)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: the branch doesn't do what the comment, the <String?> type argument, and the new SQLite doc row all promise. expectedKType comes from javaClassNameToKType(javaClassName, …), whose else arm is Any — so the resulting column type is the driver-reported class, not String.

Failure scenario: verified locally against this branch —

sqlTypeName jdbcType javaClassName resulting type
CLOB Types.CLOB java.lang.String kotlin.String
CLOB Types.CLOB java.lang.Object kotlin.Any

Xerial does report java.lang.Object in practice (that's exactly the metadata shape from #964, which the two deleted LONGVARCHAR/java.lang.Object cases used to pin). A user follows readSqlTypeMapping_SQLite.md (TEXT affinity: CLOBString), writes val notes: String in a @DataSchema, and convertTo/cast(verify = true) fails at runtime on an Any column.

Suggested fix: pin the type instead of forwarding the driver's guess — return jdbcToDfConverterFor<String?>(isNullable = nullable), with a toString() preprocessor if a non-TEXT storage class can reach a CLOB-declared column — and add a javaClassName = "java.lang.Object" case to sqliteTextAffinityMappings. While here: the duplicated // 5) numbering, and CLOB missing from the resolution-order lists in the class KDoc, Sqlite.default's KDoc, and the doc page's numbered list.

| Canonical | Aliases | DataFrame column type | Notes |
|-----------|---------|-----------------------|---------------------------------------------------------------------------------------|
| `uuid` | *none* | `String` | Read as text by default (for now). Use [`parse`](parse.md) to get `kotlin.uuid.Uuid`. |
| `xml` | *none* | `String` | |

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: these rows contradict the mappings this PR's own tests assert. pgjdbc reports xml as Types.SQLXML, and commonJdbcTypeMappings pins Types.SQLXMLjava.sql.SQLXML, not String. uuid/json/jsonb (and further down inet, cidr, macaddr, macaddr8, tsvector, tsquery, the range types, pg_lsn) are all Types.OTHER in pgjdbc — the same path unrecognised PGobject types fall back to Any asserts resolves to Any. The page even says so itself for hstore/PostGIS/composite under Unsupported types, so it disagrees with its own tables.

Failure scenario: a user reads this table and declares val uuidcol: String in a @DataSchema — literally what io/local/postgresTest.kt:86 already does — then convertTo/cast(verify = true) throws, because the column is Any.

Suggested fix: correct these rows to Any / java.sql.SQLXML, and derive them from evidence rather than from the driver's documented intent: io/local/postgresTest.kt already creates a table covering uuid, xml, json, jsonb, so asserting the read schema per column there (and doing the same in the other io/local/* and io/h2/* suites) would keep every page honest. As it stands the new unit tests feed on hand-written metadata, so they can't catch a wrong row — both come from the same assumption.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add getExpectedJdbcType tests for different DbTypes

3 participants