Added type mappings tests and docs for ALL supported DBs - #2002
Added type mappings tests and docs for ALL supported DBs#2002AndreiKingsley wants to merge 12 commits into
Conversation
# Conflicts: # dataframe-jdbc/src/test/kotlin/org/jetbrains/kotlinx/dataframe/io/db/jdbcTypesTest.kt
|
Link it to the issue or create with the proper description |
…tails on `AdvancedDbType` and custom converters.
… MySQL. Standardize column type terminology and add unsupported types section.
…ndling, refine examples, and standardize terminology. Add support for `CLOB` type conversion.
…r `STRUCT` type details.
|
|
||
| ## Exact numeric types | ||
|
|
||
| | Canonical | Aliases | DataFrame column type | Notes | |
There was a problem hiding this comment.
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`).` |
There was a problem hiding this comment.
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 { |
|
|
||
| // 5) CLOB — stored as String. | ||
| if ("CLOB" in declaredUpper) { | ||
| return jdbcToDfConverterFor<String?>(expectedKType) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Ok, but maybe then worth to add a T: Any restriction for jdbcToDfConverterFor?
For consistency.
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
looks good overall :D
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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: CLOB → String), 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` | | |
There was a problem hiding this comment.
Blocking: these rows contradict the mappings this PR's own tests assert. pgjdbc reports xml as Types.SQLXML, and commonJdbcTypeMappings pins Types.SQLXML → java.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.
Fixes #1736
For each
DBTypeimplementation, tests ALL possible sql types metadata -> expected ktype mappings and add info about it in docs.