SHOW TABLES reports the client vocabulary, through one authority (BIDC-10a Part D) - #320
Merged
Conversation
…ugh one authority
Story BIDC-10a Part D (AC 10, AC 12, T7).
`SHOW TABLES` published elasticsql's INTERNAL table-type vocabulary: the `type` column
was `TableType.name.toUpperCase`, so a plain index came back as `REGULAR` - a value no
client consumes. The sidecar's ADBC leg failed on exactly that (`Expected 'TABLE' in
['REGULAR']`), and the same column is what a JDBC `getTables`, a Flight SQL `GET_TABLES`
and an ODBC object browser receive, because all three execute `SHOW TABLES` through the
gateway. Fixing it in each transport would mean a translation layer per client, all of
which must be kept in step; the lead's ruling is to fix it once, at the source.
`TableType` gains `sqlName`, the single authority for what clients are told:
Regular -> TABLE · View -> VIEW · MaterializedView -> MATERIALIZED_VIEW
External -> EXTERNAL · Changelog -> CHANGELOG · Enrichment -> ENRICHMENT
`sqlName` is ABSTRACT on the sealed trait on purpose: a seventh table type cannot compile
until someone decides what clients should call it. That is stronger than a test, and it is
precisely the default that let `REGULAR` ship.
A materialized view keeps its OWN type rather than collapsing into `VIEW` (AD-A-6): it has
storage, a refresh schedule and a watcher, and JDBC permits arbitrary type strings.
STORAGE IS UNTOUCHED. `TableType.name` remains the `_meta.type` key, `TableType.apply`
still parses only the stored name, and an existing index whose mapping says `"regular"`
still reads back as `Regular`. No reindex, no migration.
Evidence, both halves of AC 10:
- `TableTypeVocabularySpec` (sql, 8 tests) enumerates `TableType` by walking the COMPILED
package - the trait is sealed, so the walk is complete and needs no allow-list - asserts
the stored name and the client name for every type, pins `Regular.sqlName != "REGULAR"`
literally, and asserts the round-trip `TableType(t.name) == t`.
- `ShowTablesTableTypeSpec` (core, 4 tests) drives the real `TableExecutor` projection
Docker-free from mapping JSON carrying the STORED `_meta.type`, covering all six types,
the no-`_meta` default and MV-vs-VIEW.
Falsified: reverting the projection turns 3 of 4 core tests red; `Regular.sqlName =
"REGULAR"` turns 2 of 8 sql tests red; dropping one type from the expectation table turns
3 red with a clue naming it, which is what proves the walk is live.
Release note: the `SHOW TABLES` `type` column changes value for every plain index
(`REGULAR` -> `TABLE`), and materialized views become their own node type in BI browsers -
knowingly reversing part of story 20.3's PD-4, on the measurement that did not exist then.
The driver-side shims (arrow `normaliseTableType`, jdbc `contains("regular")` /
`contains("view")`) are deliberately NOT removed here: they come out at the repin onto a
published core carrying this change, never before (AD-A-7).
…jection
Independent review round. Five findings, all addressed here; nothing deferred.
1. `ResultRenderer.scala:225` renders the `SHOW TABLE <t>` header (REPL `\st <table>`)
as `s"[${table.tableType}]"` - the case-object toString, `Regular` / `MaterializedView`.
That is why no `tableType.name` search found it, and why the first commit called
`GatewayApi.scala:554` the only display projection.
Before this story both surfaces spoke the internal vocabulary and merely disagreed on
case. Fixing one of them did not remove a divergence, it CREATED one - measurable in
this repo's own walkthrough, where `SHOW TABLES` says `TABLE` at
`documentation/sql/dql_statements.md:1315` and `SHOW TABLE users` said `[Regular]` 35
lines below at :1350; same pairing at `documentation/client/repl.md:967` versus :846.
Both projections now read `TableType.sqlName`, the two doc lines are updated, and three
new tests pin the header per type, pin that it is never the Scala name, and pin that it
AGREES with the `SHOW TABLES` type column for every type.
Lesson worth keeping: enumerate a concept's renderings BY TYPE, not by expression. A
grep for the expression you are replacing is blind to the interpolation of the value
itself.
2. The new scaladoc asserted that `name` "is never a display value". That is falsified 80
lines below it: `Table.merge` throws `Cannot alter table <t> of type ${tableType.name}`,
a user-facing message. So `name` has THREE consumers, not the two the design gate
counted - and that count was the justification for the storage/display split being safe.
The message stays: an engine refusing an operation on its own construct legitimately
names the engine's own type. What changes is that the exception is now WRITTEN DOWN, in
`TableType`'s scaladoc and at the throw site, with the rule that a new read of `name`
outside `_meta.type` and that message is a design change. Silence about a projection is
exactly what let `REGULAR` reach clients.
3. elasticsql had no live-cluster assertion of the new value, and ES 6.8 had none anywhere
(the only real-ES assertion lives in extensions, inside an `assume` that 6.8 cancels).
`GatewayApiIntegrationSpec` (template) now asserts `show_users -> TABLE`. Green on all
five clients: ES 6.8 rest 73+1 pre-existing cancel, 6.8 jest 73+1, 7.17 74/74,
8.18 74/74, 9.0 74/74.
4. `ShowTablesTableTypeSpec` hand-listed the six types, so a seventh would redden the sql
vocabulary spec and leave the projection guard green - silently under-covering the thing
it exists to guard. The sealed-hierarchy walk is extracted to `TableTypeEnumeration` and
BOTH specs derive from it. `core` reaches it because `core -> macros -> sql` carries
`test->test`.
5. One of the walk's exits swallowed `ClassNotFoundException` while the other two failed
loudly, contrary to its own comment. All five exits now fail: no root, non-`file:` root,
unloadable class, non-`object` subtype, empty result. The scaladoc says when a `jar:`
root actually happens - running these assertions against a published `softclient4es-sql`
jar - rather than leaving the reader to guess.
Verified: sql 1043/1043, core 956/956, `+ sql/compile` `+ core/compile`,
`++ 2.12.20 {sql,core}/Test/compile`, lint. Falsified: reverting `:225` turns exactly the
three new header tests red.
Release note (extended): BOTH client-facing renderings change together - the `type` column
of `SHOW TABLES` and the `SHOW TABLE <t>` header, which printed `Table: users [Regular]`
and now prints `[TABLE]`.
…W, with a space
Lead ruling AD-A-6-SUPERSEDED, PM concurring. `TableType.MaterializedView.sqlName`
`MATERIALIZED_VIEW` -> `MATERIALIZED VIEW`. The stored `name` is untouched, as always.
The reason is self-consistency with our own SQL, NOT another engine's convention. Every
statement that names this object is spaced - `CREATE MATERIALIZED VIEW`,
`SHOW MATERIALIZED VIEW`, `SHOW MATERIALIZED VIEW STATUS`, `SHOW CREATE MATERIALIZED VIEW`,
`DESCRIBE MATERIALIZED VIEW`. An underscore in the `SHOW TABLES` `type` column would be the
only place the product spells its own object differently from the statement that creates
it. (The Postgres argument is deliberately NOT used: with the transports reporting `VIEW`,
no external tool reads this string, and the reviewer had already caught the previous
scaladoc citing Postgres's spaced form while shipping an underscore.)
The scaladoc is corrected on a second point it was getting wrong: `sqlName` is the ENGINE
surface, not the JDBC/Flight `TABLE_TYPE`. Those drivers map it onto the two values their
`getTableTypes` advertises, and for a materialized view that is `VIEW` - permanently, not as
a shim, because `getTables` filters by exact string match, so a third value with the
advertised list unchanged would make every materialized view vanish from an object browser.
That is jdbc#34's dead-end, and it is written at the case object so a future cleanup cannot
mistake the mapping for temporary.
One assertion had to change to accept the ruling, and it is the interesting part: the
well-formedness pin read `fullyMatch regex "[A-Z][A-Z_]*"`, which a space fails. It is now
`"[A-Z]+( [A-Z]+)*"` - which also REJECTS an underscore, so the same assertion that admits
the new spelling is what stops it silently regressing to the old one. A literal pin sits
beside it (`shouldBe "MATERIALIZED VIEW"`, `should not include "_"`), because a property
assertion is structurally blind to a spelling choice.
`Feature.fromString("MATERIALIZED_VIEWS")` in `FeatureFromStringSpec` was checked and left
alone: a licence feature identifier, unrelated to the table-type vocabulary, and the only
false positive in the repo. No documentation example shows a materialized view in a
`SHOW TABLES` `type` column, so no doc change was needed.
The anti-drift assertion added in the previous commit - the `SHOW TABLE` header agreeing
with the `SHOW TABLES` column - is kept and now agrees on the spaced value.
Verified: sql 1044/1044 (+1), core 956/956, `+ sql/compile` `+ core/compile`,
`++ 2.12.20 {sql,core}/Test/compile`, lint; `JavaClientGatewayApiSpec` 74/74 on real ES 8.18.
Falsified: restoring the underscore turns exactly 3 of 9 sql tests red - the expectation,
the new literal pin, and the well-formedness assertion.
Release note, now SMALLER: a BI tool observes NO change from Part D. A plain index already
reached a browser as `TABLE` through the driver's own mapping and a materialized view
already reached it as `VIEW`, so the BI-browser warning is withdrawn, and so is the claim
that this reverses story 20.3's PD-4 - PD-4's frozen advertised list is respected. What
changes is the engine surface: `SHOW TABLES` `type` goes `REGULAR` -> `TABLE` and
`MATERIALIZED_VIEW` -> `MATERIALIZED VIEW`, and the `SHOW TABLE` header goes
`[Regular]` -> `[TABLE]`.
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
1 participant
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.
Part D of story BIDC-10a:
SHOW TABLESreports the client-facing table-type vocabulary through asingle authority, instead of leaking elasticsql's internal
TableTypenames.The lead ruling behind it: fix the vocabulary at the source, not in each transport. Prompted by a
live CI regression on arrow PR #177, where unifying
GetTableTypesandGetTableson the internalvocabulary made the two agree with each other and stop agreeing with every client — the ADBC leg
failed with
Expected 'TABLE' in ['REGULAR'].What changes
SHOW TABLEStype, plain indexREGULARTABLESHOW TABLEStype, materialized viewMATERIALIZED_VIEWMATERIALIZED VIEW(spaced)SHOW TABLE <t>header (REPL\st)Table: users [Regular]Table: users [TABLE]VIEW·EXTERNAL·CHANGELOG·ENRICHMENT🔴 A BI tool observes NO change from this. The JDBC and Flight
TABLE_TYPEcontracts areunchanged: a plain index already reached a browser as
TABLEthrough the driver's own mapping, and amaterialized view already reached it as
VIEW. Story 20.3's frozen advertised type list isrespected, not reversed. What moves is the engine surface only.
Nothing stored changes.
_meta.typestill holdsregular/materialized_view, still parsed byTableType(...). No reindex, no migration. A test asserts the round trip for every type.Design
TableTypegains an abstractsqlName. Abstract, not a mapping function with a fallback case,because a fallback is exactly how
REGULARshipped in the first place — a seventh table type nowfails to compile until someone decides its client-facing name.
name(storage) andsqlName(display) have disjoint consumers, so no abstraction is invented.
Why the materialized-view spelling is spaced, and it is not an appeal to another engine's
convention: every statement naming the object is spaced —
CREATE MATERIALIZED VIEW,SHOW MATERIALIZED VIEW,SHOW MATERIALIZED VIEW STATUS,SHOW CREATE MATERIALIZED VIEW,DESCRIBE MATERIALIZED VIEW. An underscore would be the only place the product spells its own objectdifferently from the statement that creates it.
What the review round caught
An independent fresh-context reviewer found that the first implementation created the divergence
it set out to close:
ResultRendererrenders the case object viatoString, so it was invisible to agrep for the expression being replaced, and it left
SHOW TABLESsayingTABLEwhileSHOW TABLEsaid
[Regular]— 35 lines apart in the same documentation walkthrough. Both projections now readsqlName, and a test asserts they agree for every type.It also found the new scaladoc asserting an invariant (
name"is never a display value") that a thirdconsumer 80 lines below breaks. The engine's own refusal
Cannot alter table <t> of type materialized_viewlegitimately names the engine's own type; that is now documented as the onedeliberate exception rather than silently contradicted.
Verification
sql/test1044/1044 ·core/test956/956+ sql/compile,+ core/compile, and explicit++ 2.12.20test compiles — green on 2.12.20 and 2.13.16GatewayApiIntegrationSpecon real ES, all five clients: 6.8 rest, 6.8 jest, 7.17, 8.18, 9.0headerCheck·scalafmtSbtCheck·scalafmtCheck·test:scalafmtCheckreddens exactly 3 of 9 sql tests; dropping a type from the expectation reddens 3 with a clue naming it.
The exhaustiveness gate walks the compiled package rather than a hand-written list. Its first
version silently returned empty — the spec shares a package with
TableType, so test-classes shadowedthe main classes dir and 5 of 8 assertions passed vacuously. It now unions every root and fails on
emptiness inside the enumerator.
Downstream
contains("view")→VIEWare load-bearing: removing either makes every materialized view vanish from an object browser, because
getTableTypesadvertises onlyTABLEandVIEWandgetTablesfilters by exact match. Only theircontains("regular")branches become dead at the repin onto a core carrying this change. Each repopins its own collapse against both spellings.
A downstream pinning the literal strings
REGULARorMATERIALIZED_VIEWmust be updated.TableTypeis
sealed, so the added abstract member is source- and binary-compatible for consumers.Sibling branches, held for review, no PRs yet:
feature/BIDC-10ain softclient4es-extensions (testkitexpectation), softclient4es-arrow and softclient4es-jdbc (the capability declaration), and
softclient4es-web (a published walkthrough showing the old value).
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