One JDBC type catalogue, in core, that both transports derive from (BIDC-10a) - #321
Merged
Conversation
…s derive from Lead-directed, folded into BIDC-10a. `getTypeInfo` in the JDBC driver was a hand-written list of 11 rows deriving from nothing, and the Flight producer hand-wrote its own 11 with `java.sql.Types` imported directly - two parallel transcriptions of a mapping that already existed. `core.client.metadata.JdbcTypeCatalog` is now the one authority; jdbc and arrow derive from it next. It lives in `core`, not `sql`: `sql` is the AST and parser module and must not gain a `java.sql` dependency. Verified before building on it - `core` is on the compile classpath of BOTH consumer modules, checked in their own sources (`ElasticDatabaseMetaData.scala` and `ElasticFlightProducer.scala` each import `app.softnetwork.elastic.client.*`), not inferred from a build file. WHAT THE DUPLICATION WAS HIDING, and it is worse than the duplication. `getColumns.TYPE_NAME` reports the ENGINE's type id, so a character column comes back as `KEYWORD` or `TEXT`, never `VARCHAR`. With 11 rows, `KEYWORD`, `TEXT`, `DATETIME`, `CHAR`, `STRUCT`, `GEO_POINT`, `VARBINARY` and `ARRAY<...>` round-tripped to NO catalogue row at all. Neither transcription could reveal it, because each was self-consistent with itself. The catalogue is now 20 rows - every user-facing type a column can actually have - and `find(typeId)` resolves a `getColumns` type name back to its row. DERIVED, NOT STATED. A row carries a `SQLType`, not a code: `typeId` and `dataType` are computed from `sqlType.typeId` and the single `jdbcType(SQLType)` mapping, so a row and the mapping cannot disagree by construction. That is the failure the cross-repo "agreement gate" cannot catch (spec D.2b). THE ORDERING RULE CHANGED, and any surviving distinctness assertion now pins the OLD catalogue. With 11 rows every `DATA_TYPE` was distinct, so ascending `DATA_TYPE` was a total order and `getTypeInfo`'s second clause - "and then by how closely the data type maps to the corresponding JDBC SQL type" - never had to be adjudicated. Both repos asserted "sorted AND distinct" and were right to. At 20 rows `KEYWORD`/`TEXT`/`VARCHAR`/`GEO_POINT` all carry `Types.VARCHAR`, `NUMERIC` shares `Types.DOUBLE`, `DATETIME` shares `Types.TIMESTAMP`: distinctness is gone and the tie-break is live. The tie-break is COMPUTED, not enumerated: the type whose own id IS that JDBC type's name is the closest possible match, read off `java.sql.JDBCType.valueOf(code).getName`, then `typeId` ascending to make the order total. So `VARCHAR` precedes `GEO_POINT`/`KEYWORD`/`TEXT`, `DOUBLE` precedes `NUMERIC`, `TIMESTAMP` precedes `DATETIME`, with nothing hand-listed - hand-listing "VARCHAR first" would have been a fourth place encoding one fact. `entries` is produced by SORTING a declaration whose source order is irrelevant, so no row can be hand-placed. Falsified three ways: dropping the whole tie-break reddens 3 of 11 tests; dropping only the canonical-first component reddens 2, isolating that rule; and shuffling the declaration order changes nothing - the last is what proves the shipped order is computed rather than typed, and no positional fixture can prove it. The nine new sizes are not nine invented numbers. `PRECISION` describes what the JDBC TYPE can hold and `DATA_TYPE` IS the JDBC type, so types sharing a `DATA_TYPE` share its precision: `NUMERIC` takes `DOUBLE`'s 15, `DATETIME` takes `TIMESTAMP`'s 23, `TEXT`/`KEYWORD`/`GEO_POINT` take `VARCHAR`'s 65535. `CHAR` and `VARBINARY` have codes of their own and no engine-imposed bound, so they take the same ceiling rather than a number invented for each; `STRUCT` and `ARRAY<STRUCT>` have none. All eleven pre-existing rows keep their exact values. `NULL` is deliberately excluded, and the source says why: `getTypeInfo` describes types a column can HAVE, and `Types.NULL` is the absence of a value, not a storable type. Its presence in the mapping is legitimate and different - that maps an inferred RUNTIME type. Verified: core 967/967, `+ core/compile`, `++ 2.12.20 core/Test/compile`, lint. Published locally and content-verified with `javap` on the jar, not a directory listing.
…mer boundary
Lead-approved follow-up to the type catalogue. Core's read path hands out Scala `Map`s and
`List`s as CELL values; a JDBC or ADBC consumer that casts gets a `ClassCastException` and
one that calls `toString` gets Scala syntax. Both drivers need the same conversion, so it
lives in core once rather than twice.
THE CENSUS CHANGED THE DESIGN, and it was worth more than the converter.
The obvious home was `ElasticConversion.jsonNodeToAny`, which builds the `List` and the
nested `Map` from `_source`. It is not the only producer. Three make these shapes and only
one goes through it:
1. `jsonNodeToAny:1222-1225` - array branch to a Scala `List`, object branch to
`jsonNodeToMap`, a `ListMap`.
2. the aggregation path, ~`:958` and ~`:977` - a stats aggregation emits
`name -> ListMap("count" -> ..., "sum" -> ...)` and `percentiles` emits
`name -> ListMap(<key> -> <double>)`, both as CELL values, neither built from a
`JsonNode`.
3. `extractInnerHits` ~`:615-630` - `innerHitName -> List[ListMap[String, Any]]`, hand
assembled, and precisely the List-of-Map shape a shallow conversion gets wrong.
So a producer-side converter would have been honest for one path and a lie for two. This one
sits at the CONSUMER boundary, where it is producer-agnostic by construction: it converts
whatever value it is handed, and a fourth producer needs no change to it. Generalising:
before centralising a conversion, census the PRODUCERS - "where is this value built?" usually
has more than one answer, and the obvious one is rarely the complete one.
HOME: `metadata`, beside the type catalogue, not `ElasticConversion`. Co-locating with one of
three producers would misstate the relationship. Nothing inside core calls it; it exists
solely to satisfy a driver-facing contract, exactly like the catalogue.
EAGER OR LAZY: lazy, at the accessor, with an identity fast path. Converting every row at
extraction time would allocate on the hot path for every row of every query, including the
overwhelming majority where no cell is a collection and no consumer ever calls `getObject` -
the path #238 and arrow#139 exist to keep cheap. A non-collection returns the SAME REFERENCE,
so the common case is one type test and no allocation, paid only for cells someone asks for.
Deliberately not memoised here: core does not own the row's lifetime, and a driver that
measures repeated access to one wide struct can cache at its own seam.
RED THEN GREEN, GENUINELY. A deliberately shallow implementation went in first and passed 7
of 11 tests - every single-level case, key order, reference identity, null, byte arrays and
empty collections - failing only the four depth-sensitive ones. That is what makes the
nested-inside-array test a gate rather than a restatement, and it is the reason the lead
asked for it first: a shallow implementation is what every reviewer's eye reads as correct.
One test drives REAL core code (`jsonNodeToAny` on an array of objects) and asserts both
halves: that core still produces Scala collections, so the problem is real, and that the
conversion resolves them at every level, so the fix is. If a producer is ever changed to emit
Java collections directly, that test says so instead of `toJavaValue` silently becoming dead
code.
Boundaries, all recorded in source: call it on a VALUE and never on the row (both are
`ListMap` and this cannot tell them apart); a `byte[]` survives untouched, since a Java array
is not a Scala `Seq` and a `java.util.List[Byte]` would break every consumer that casts to
`[B`; `Option` is deliberately NOT unwrapped, because no producer emits one today - the stats
aggregation unwraps its own - so unwrapping here would hide a real defect rather than fix one.
`LinkedHashMap` and `ArrayList`, so `ListMap` key order survives into the struct a consumer
walks.
The drivers are untouched: this is a seam, not a patch. `toJdbcClassName` reporting `STRUCT`
as `java.lang.String` through the catch-all, and `ARRAY` as `java.sql.Array` which
`createArrayOf`/`createStruct` both refuse to construct, are the jdbc agent's to fix against
it.
Verified: core 979/979, `+ core/compile`, `++ 2.12.20 core/Test/compile`, lint. Published
locally and content-verified with `javap` - `JavaValueConversion$.toJavaValue` present and
`JdbcTypeCatalog$` still intact in the same jar.
CI's `core / Compile / scalafmtCheck` failed on this file. Scaladoc reflow only — verified that no non-comment line changes. 🔴 The reason it reached CI is worth more than the fix: **`scalafmtCheck` and `scalafmtAll` are VACUOUS in a git worktree here**, so every local "lint green" claim about a new file in this branch was meaningless. `.scalafmt.conf:17` sets `project.git = true`, so scalafmt takes its file list from git. This worktree carries `zz-worktree-nogit.sbt` — the documented shim for sbt-git's JGit failure inside a worktree — which stubs out that integration. The two combine into a check that enumerates ZERO files: `scalafmtAll` completes in one second, reports success, and formats nothing. `scalafmtOnly` with an explicit path is filtered the same way. Meanwhile `git ls-files` lists the file fine, so the file is tracked and CI, which runs in a normal checkout, checks it and fails.⚠️ This is the second form of the same class in this story. The first was staging before formatting, so the commit held pre-format bytes while the gate read the formatted working tree. Both have the same shape: **a gate that reads something other than what CI reads proves nothing.** Here the gate read a file list that was empty; there it read a tree that was not the commit. Formatted by temporarily flipping `project.git` to false, running `scalafmtAll`, then restoring it — `.scalafmt.conf` is byte-identical to HEAD, verified, and exactly one file changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fupelaqu
marked this pull request as ready for review
September 10, 2026 07:20
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-on to #320, same story. One JDBC type catalogue, in core, that both transports derive from.
Why
DatabaseMetaData.getTypeInfoin softclient4es-jdbc andXdbcTypeInfoRowsin softclient4es-arrow weretwo independent hand-written transcriptions of a mapping that already existed in the driver
(
TypeMapping.toJdbcType). Nothing forced either list to track that mapping, so both drifted.That is the same defect #320 fixed one layer down: two places encoding one fact. Here it had three
consequences, and the third is the one that matters.
getTypeInfojavadoc (rows must be ordered byDATA_TYPE); it was fixed by hand in both repos, which is exactly the fragility being removed.getColumns.TYPE_NAMEreports the ENGINE's type id, so a character column comes back asKEYWORDorTEXT, neverVARCHAR. At 11 rows,KEYWORD,TEXT,DATETIME,CHAR,STRUCT,GEO_POINT,VARBINARYandARRAY<…>round-tripped to no catalogue row at all. Neithertranscription could reveal it, because each was self-consistent with itself.
What this adds
core/.../client/metadata/JdbcTypeCatalog.scala— 20 rows plusfind(typeId)to resolve agetColumnstype name back to its row.It lives in
core, notsql: the AST and parser module gains nojava.sqldependency, and bothdrivers already compile against
core. That is what turns the cross-repo agreement from a conventioninto a real interlock — previously, adding a row to one repo left the other's build green.
Derived, not stated. A row carries a
SQLType; itstypeIdandDATA_TYPEare computed fromsqlType.typeIdand the singlejdbcTypemapping. A row and the mapping cannot disagree byconstruction.
All eleven pre-existing rows are byte-identical. The nine new sizes follow a rule rather than being
invented per type: types sharing a
DATA_TYPEshare its precision (NUMERIC15,DATETIME23,TEXT/KEYWORD/GEO_POINT65535);CHARandVARBINARYhave their own codes and no engine bound, sothey take the same ceiling;
STRUCTandARRAYhave none.NULLis excluded, with the reason in source.getTypeInfodescribes types a column can have;Types.NULLis the absence of a value, not a storable type, and no mainstream driver lists it. Itspresence in the
SQLType→ code mapping is legitimate and different — that maps an inferred runtimetype.
At 11 rows every
DATA_TYPEwas distinct, so the javadoc's tie-break never applied and both reposasserted "sorted and distinct". At 20 rows, four types share
Types.VARCHAR, two shareDOUBLEand two share
TIMESTAMP. Distinctness is gone, and any surviving distinctness assertion ispinning the old catalogue.
The tie-break is computed, not enumerated: the canonical JDBC name for a code sorts first
(
JDBCType.valueOf(dataType).getName == typeId, which is the javadoc's "how closely the data type mapsto the corresponding JDBC SQL type"), then
typeIdascending for totality. Hand-listing an order wouldhave been a fourth place encoding one fact.
Falsified three ways; the decisive one is that shuffling the declaration changes nothing, which no
positional fixture can prove.
Verification
core/test967/967 ·+ core/compile·++ 2.12.20 core/Test/compile·headerCheck,scalafmtSbtCheck,scalafmtCheck,test:scalafmtCheck. Published locally and content-verified withjavapon the jar, not a directory listing.Downstream
Both drivers repin to
0.23.0-SNAPSHOTand derive from this object; their branches are held pendingthis merge and a publish, since a shared object is invisible from a pinned released core.
Second commit — one deep Scala-to-Java value conversion, at the consumer boundary
core/.../client/metadata/JavaValueConversion.toJavaValueturns ScalaMap/Seq/Setintojava.util.LinkedHashMap/ArrayList/LinkedHashSet, recursively on both keys and values.LinkedHashMapbecause core's maps areListMaps whose key order is meaningful to a consumer walkinga struct.
byte[]is untouched — a Java array is not a ScalaSeq, and ajava.util.List[Byte]breaksevery
[Bcast. Non-collections return the same reference: one type test, zero allocation.It exists because
toJdbcClassNamereportsSTRUCTasjava.lang.String(a catch-all fall-through)and
ARRAYasjava.sql.Array, which the driver never constructs —createArrayOfandcreateStructboth throw. Those repairs are the drivers'; this is the seam they need.
🔴 A census done while building it changed its design, and is the reusable part.
jsonNodeToAnyisnot the only producer of these shapes. Three are, and only one goes through it:
jsonNodeToAny— array → ScalaList, object →ListMap.name -> ListMap("count" -> …, "sum" -> …),percentilesemitsname -> ListMap(key -> double); both as cell values, neither from aJsonNode.extractInnerHits—innerHitName -> List[ListMap[String, Any]], hand-assembled, and preciselythe List-of-Map shape a shallow conversion gets wrong.
A producer-side converter would have been honest for one path and a lie for two. It sits at the
consumer boundary instead, producer-agnostic by construction — a fourth producer needs no change.
Before centralising a conversion, census the producers; the obvious answer is rarely the complete one.
Lazy at the accessor, not eager per row. Eager conversion would allocate on the extraction path for
every row of every query, including the majority with no collection cell and no
getObjectcall — thepath #238 and arrow#139 exist to keep cheap. Not memoised here, because core does not own the row's
lifetime; a driver that measures repeated access to one wide struct can cache at its own seam.
🔴 The shallow implementation passed 7 of 11 tests — written deliberately first. It passed every
single-level case, key order, reference identity, null, byte arrays and empty collections, failing only
the four depth-sensitive ones. A conversion suite without a nested-inside-array case cannot tell deep
from shallow, and shallow is what a reviewer's eye reads as correct. One test drives real core code
(
jsonNodeToAnyon an array of objects) and asserts both halves, so if a producer ever starts emittingJava collections that test says so rather than
toJavaValuesilently becoming dead code.No issue is filed for this story, per the epic's issue-lifecycle rule, so this PR carries no closing
keyword.
🤖 Generated with Claude Code