Add entityType discriminator and table-scoped HTS queries - #696
Open
ruolin59 wants to merge 17 commits into
Open
Add entityType discriminator and table-scoped HTS queries#696ruolin59 wants to merge 17 commits into
ruolin59 wants to merge 17 commits into
Conversation
Tables and views share one (databaseId, objectId) pointer key space, so a name must resolve to exactly one catalog object. This adds a nullable entityType discriminator end-to-end and makes every table path aware of it. Semantics: NULL and any case spelling of TABLE mean table; any case spelling of VIEW means view; any other non-null value fails closed. The column is nullable with no backfill, so existing rows and existing table writes are untouched -- ordinary commits still write no discriminator. Read paths filter in the query, never by post-filtering a returned Page. A fetch-then-filter implementation returns short pages and inflated totals; the predicate and its countQuery are the same shared String constant, so content and count cannot diverge. Applied to both /hts query families, the internal catalog listings, listHouseTables, searchTables, and database enumeration. Write paths separate typed load from name occupancy. findById and findTableRefById answer "can this be loaded as a table?" and hide non-table rows; the new findOccupyingEntityTypeById answers "is this name taken, and by what?" without parsing metadata. CREATE and rename-destination consult occupancy before authorization, storage allocation, metadata writes, and pointer saves, so a collision is an accurate 409 rather than a misleading concurrency error. HTS errors propagate rather than reading as a free name. The drop guard lives in findTableRefById and OpenHouseInternalCatalog rather than doRefresh, because deleteTable deliberately bypasses loadTable so drops survive corrupted metadata; a doRefresh-only guard would be inert there. Wrong-type read and drop return 404; collisions return 409. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The @query annotations added to HouseTableRepository were inert in production and broke a universal convention in this repo, so this reverts that interface to its pre-change state and drops the tests that only exercised them. Why they were inert: TablesSpringApplication excludes DataSourceAutoConfiguration, so the tables service has no DataSource bean and the only @EnableJpaRepositories scan is HTS-scoped. No Spring Data proxy of HouseTableRepository can ever be created there. The sole bean behind that interface is the hand-written HouseTableRepositoryImpl, which ignores @query entirely and talks to HTS over HTTP. HTS in turn already applies the same table-only predicate in SQL inside UserTableHtsJdbcRepository, so production filtering is complete without these annotations. Why they were wrong stylistically: only a handful of files in this repo carry @query, and every one of them executes against a real database. The established precedent for exactly this shape is HtsRepository, an empty interface whose JPA semantics live entirely on its impl/jdbc class. Production interfaces declare the contract; implementations own behavior. Restoring the interface puts HouseTableRepository back in line with that, and leaves internalcatalog's main sources with no spring-data-jpa usage at all. Why the removed tests go with them: the eleven deleted listing tests in RepositoryTest, DatabasesControllerTest and TablesControllerTest ran against the H2 Spring Data double, where the annotations did take effect. The production methods they covered (listTables, listHouseTables, searchTables, findAllIds) are byte-for-byte unchanged by this change set, so those tests were verifying a test double rather than production code. The genuine coverage for the same acceptance criteria lives in services/housetables, where the predicate actually runs in SQL. Every view isolation guard test that exercises real production logic is kept. Adding the same filtering to the H2 doubles is deliberately left out; it belongs with the view-commit work, since nothing in main sources writes a VIEW discriminator yet, which would make the filter unreachable and untestable today. Verified: housetables 153, internalcatalog 124, tables 519 (was 530, exactly the 11 removed), tables-test-fixtures 8, spark-3.5 catalogTest 66 - all green, plus spotlessCheck. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Reverts the table predicate on both findAllDistinctDatabaseIds overloads in UserTableHtsJdbcRepository to their pre-change form, and drops the two tests that only asserted the reverted behavior. The four table-row filters are untouched: findAllByDatabaseIdIgnoreCase, the tableId-pattern variant, their paginated forms, and the findAllByFilters entity-type clause remain exactly as they are. Those are the genuine production filtering for this ticket. These two methods return a projection of database-ID strings, not rows, so no view can appear in their output under any implementation. The filter did not hide a view; it only changed which database names get listed. That is outside the scope this change set set for itself. The design enumerates the queries that need the table predicate and this is not among them, the stated harm is that SHOW TABLES would return views, and the acceptance criterion is that no view appears in a table listing. A database listing is not a table listing. Filtering here also contradicts three other design statements taken together: a namespace maps to an already-existing database and is never created implicitly, the server never auto-creates databases, and HTS infers databases from object rows and has no way to represent an empty database. With the filter, a database holding only views becomes non-existent by the only existence mechanism OpenHouse has - while views may only be created in databases that already exist. Concretely this path is Spark's SHOW DATABASES via OpenHouseCatalog.listNamespaces(). With the filter, a view-only namespace would be missing from SHOW DATABASES while still being addressable at /v2/databases/foo/views/v1. The rule this restores: queries that enumerate objects must be type-scoped; queries that enumerate containers must not. Removed with it, as they asserted only the reverted behavior: HtsRepositoryTest#testFindDistinctDatabasesExcludesViewOnlyDatabases and HtsControllerTest#testDatabaseQueriesExcludeViewOnlyDatabases. No fixture, helper or import became unused. The pre-existing testFindDistinctDatabases and the entity-type case/garbage matrix are unaffected and stay. Verified: housetables 151 (was 153, exactly the 2 removed), internalcatalog 124, tables 519, tables-test-fixtures 8, spark-3.5 catalogTest 66 - all green, plus spotlessCheck. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…ionUtils Restores the private rootMetadataFileLocation in OpenHouseInternalTableOperations to its pre-change form, doing the naming work inline, and deletes MetadataLocationUtils along with its test. The stated goal was to move this into a shared helper so the table and view paths use one implementation. The view path is not part of this change, so the helper has exactly one production caller: the very method it was extracted from. That is indirection rather than sharing. The caller now hops through a private wrapper into a public util, and OpenHouseInternalTableOperations picked up an import and a delegation without getting any simpler. The codecName parameter exists only to serve a future view caller, since Iceberg's table and view compression defaults differ, and the helper's test covered a gzip path that no production caller passes today. An extraction is a refactor that a second caller justifies. The view commit work will have that second caller and can do the extraction then, with the real shape of both callers in hand. This is the same reasoning that deferred the HouseTableMapper ViewMetadata overload out of this change. Behavior is unchanged, as it was when the code was extracted: identical path format, five-digit zero-padded version, random UUID, and extension resolved from the same codec property. Every OpenHouseInternalTableOperations metadata-location test passes untouched. The plain-text javadoc reference to this method in InternalRepositoryUtils#getSchemeLessPath again describes the inline implementation it was written against. The doRefresh non-table guard in this file is untouched; that is real view isolation logic and stays. Verified: internalcatalog 121 (was 124, exactly the 3 MetadataLocationUtilsTest cases), housetables 151, tables 519, tables-test-fixtures 8, spark-3.5 catalogTest 66 - all green, plus spotlessCheck. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…nstead Restores OpenHouseInternalCatalog#resolveFileIO to its pre-change form and gives the raw-pointer test fixtures the storage type they were missing. The guard was compensating for a malformed fixture, not for a production condition. seedRawPointer built a HouseTable with databaseId, tableId, clusterId, tableUri, tableUUID, tableLocation, tableVersion and entityType but no storageType, so storageType.fromString(null) threw. A row seeded that way would have thrown just the same with entityType TABLE; the discriminator was incidental to the failure. The HTS schema settles it: storage_type is VARCHAR(128) DEFAULT 'hdfs' NOT NULL, so a null storage type cannot exist in production, whereas entity_type is DEFAULT NULL and is null on every pre-existing row. The guard was also wrong on its own terms. A real view row carries a valid storage type, so the original code returns the view's actual storage; skipping the row instead consults storageSelector, which can resolve to a different storage than the one the object is really on. And it is unreachable for the purpose it claimed: dropTable rejects a view before reaching this line, and on the newTableOps path doRefresh already treats a view as absent while create-over-view is stopped by the occupancy check. So the fix belongs in the fixture. Both seedRawPointer helpers now set storageType from storageManager.getDefaultStorage(), the same value a real table gets through HouseTableMapper. That makes the seeded row well-formed rather than merely tolerated. Every view-isolation guard test still passes, and now passes because the pointer is realistic rather than because production skips it: drop-VIEW, rename source and destination, CREATE-over-VIEW occupancy, findTableRefById, and the 404/409 status assertions, including all four case and garbage parameterizations of each. The dropTable and renameTable entity-type guards in this file are untouched, and so is the stripOhNamespace null-safety in the mapper - entity_type is DEFAULT NULL, so MapStruct's implicit String conversion would NPE on the real production mapping path without it. Verified: internalcatalog 121, tables 519, housetables 151, tables-test-fixtures 8, spark-3.5 catalogTest 66 - all green with no count change from this commit, plus spotlessCheck. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…e pattern queries Deletes both findAllByDatabaseIdIgnoreCase overloads and routes listTables through findAllByFilters, and gives the two findAllByDatabaseIdAndTableIdLikeAllIgnoreCase overloads an entityType parameter. The paginated listTables already called findAllByFilters(databaseId, null, null, null, null, null, pageable) before this change set; it was switched to findAllByDatabaseIdIgnoreCase along the way. Consolidating restores that shape with entityType added. The non-paginated overload now matches it. The two plain methods were redundant with the parameterized family. Compared clause by clause: databaseId uses the same lower() comparison, tableId is exact equality rather than LIKE so an unset value adds no constraint, every other filter is guarded by an IS NULL check, DISTINCT over a single PK'd root is a no-op, and a null entityType takes the same predicate branch that the old hard-coded table predicate expressed. Identical results, one query family instead of two. The pattern overloads keep their own query because folding pattern matching into findAllByFilters would mean either a second tableId parameter or turning its exact match into a LIKE - and OpenHouse identifiers routinely contain underscores, so a LIKE there would silently treat them as wildcards. They now take entityType instead, reusing the same predicate constant. No listing method has a type baked into its name any more, and the call sites pass the request's own entityType rather than a hard-coded value, so the view path needs no new query methods - only entityType=VIEW at a call site. Verified: housetables 151 and tables 519, both unchanged and green, as expected for a refactor with identical semantics. HtsControllerTest 26, HtsRepositoryTest 17 and UserTablesServiceTest 21 all pass, which covers the rerouted list and pattern paths. Plus spotlessCheck. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
… guards Removes every Java-side entity-type check in the tables service and the internal catalog, along with the tests that exercised them. What remains is the discriminator itself and the SQL that filters on it. Point-read type filtering is deferred to the view-commit ticket, where it will be done at the query level in HTS - a table-scoped getUserTable plus a neutral entity endpoint - rather than as Java guards layered on top of a type-blind read. Shipping the guards here would mean writing them twice and migrating callers off them a ticket later. The epic's acceptance criteria are evaluated across all six tickets rather than per ticket. Nothing deploys until the whole epic ships, and substantial client work is still required before a view can be created at all, so there is no window in which views exist unprotected by this deferral. Removed: the doRefresh non-table guard; the dropTable guard; the renameTable source guard and occupied-destination preflight; the findTableRefById type filter; findOccupyingEntityTypeById and its interface declaration and shared raw-pointer helper; and rejectNonTableNameOccupancy with both call sites. The five production files affected are now byte-identical to their pre-change state. Newly dead with them: HouseTableSerdeUtils.isTableEntityType, isViewEntityType, TABLE_ENTITY_TYPE and VIEW_ENTITY_TYPE, which had no remaining main-source caller. ENTITY_TYPE_FIELD_NAME stays - it is @VisibleForTesting like its neighbours in that class and backs the serde registration test, which is substrate. Write validation keeps its own ENTITY_TYPE_REGEX in ValidatorConstants and never depended on the removed constants. Kept as substrate: the schema column; UserTableRow, UserTable, UserTableDto and UserTablesMapper plumbing; HouseTable.entityType with its serde registration and mapper handling; the entity-type SQL predicate and its four query users in HTS; write validation; the stripOhNamespace null-safety; and every HTS-layer test for the list predicates and the round trip. Verified: housetables 151, internalcatalog 87, tables 475, tables-test-fixtures 8, spark-3.5 catalogTest 66 - all green, no surviving test failed. Plus spotlessCheck and checkstyle. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…lers
Adds a table-scoped point read to HTS and wires getUserTable to it, so a view
at a table's key is invisible to the table path because of the query rather
than because every caller checks.
getUserTable is the single HTS endpoint behind every table point read in the
tables service, so filtering it there makes four call sites correct with no
Java guard at all:
doRefresh findById -> getUserTable -> 404 -> HouseTableNotFound,
already caught, leaves Optional.empty, refreshes from a
null location exactly as for an absent row
findTableRefById findHouseTable catches the same exception and returns
empty
dropTable findHouseTable returns empty, so the existing
orElseThrow raises NoSuchTableException
rename source loadTable(from) -> doRefresh -> no metadata -> the same
NoSuchTableException
findByDatabaseIdIgnoreCaseAndTableIdIgnoreCase stays neutral on purpose.
HtsRepository.findById and existsById delegate to it and back putUserTable,
deleteUserTable, restoreUserTable and renameUserTable inside HTS, which must
see a row of any type to detect a collision at a shared key. Only the read
serving getUserTable changed.
TABLE_ROW_PREDICATE returns as the single statement of "null or TABLE", with
ENTITY_TYPE_FILTER_PREDICATE now composed from it, so the row test is written
once. No view-only method is added: nothing in this change reads views, and
the list queries already reach them through the entityType parameter.
Still deferred to the view-commit ticket, because they need the neutral
fetcher: occupancy, the rename destination preflight, and reading a view back
over HTTP.
The tables-service guard tests could not follow this filter - those tests run
the H2 double, which never goes through HTS - so the coverage moves to
services/housetables where the query actually executes: the case and garbage
matrix on the new point read, the neutral read still seeing every type, the
service-level getUserTable behavior, and the HTTP 404. Replicating the
predicate into the doubles was deliberately not done; that is the
testing-the-fake pattern already reverted for the list queries.
testEntityTypePutAndGetRoundTrip now asserts the view PUT is readable through
the PUT response and the persisted row, and that the table-scoped GET returns
404. That is the deferred neutral read, not a regression.
Verified: housetables 177, internalcatalog 87, tables 475,
tables-test-fixtures 8, spark-3.5 catalogTest 66 - all green, plus
spotlessCheck and checkstyle.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Applies the agreed query-level contract: a method whose name says "table" filters to tables, everything else stays neutral or takes entityType as a parameter. Renamed and filtered, because every caller assumes tables: findAllByDatabaseIdIgnoreCase -> findAllTablesByDatabaseIdIgnoreCase findAllByDatabaseIdIgnoreCase(Pageable) -> findAllTablesByDatabaseIdIgnoreCase(Pageable) findAllByDatabaseIdAndTableIdLikeAllIgnoreCase -> findAllTablesBy... findAllByDatabaseIdAndTableIdLikeAllIgnoreCase(Pageable) -> findAllTablesBy...(Pageable) "TableId" in those names is the column table_id, which under a shared key space holds a view's name too, so the old names were column-scoped and type-ambiguous rather than already table-scoped. Both findAllByDatabaseIdIgnoreCase overloads were removed earlier in this branch when listTables was consolidated onto findAllByFilters; they are restored under the new names and listTables routes back to them. The paged overload was declared but never called before this branch, so adopting it for paged listTables costs nothing. Added findTableByDatabaseIdIgnoreCaseAndTableIdIgnoreCase, which getUserTable now calls. That is the single HTS endpoint behind every table point read in the tables service, so the guards removed earlier are correct by construction: findById maps a 404 to HouseTableNotFoundException, which doRefresh already catches to leave an empty Optional and refresh from a null location, and which findHouseTable already catches to return empty - so dropTable's existing orElseThrow raises NoSuchTableException, findTableRefById returns empty, and a rename whose source is a view fails in loadTable. findByDatabaseIdIgnoreCaseAndTableIdIgnoreCase stays neutral and untouched. findById delegates to it and backs putUserTable, deleteUserTable and restoreUserTable, which must see a row of any type to detect a collision at a shared key. existsBy, deleteBy, renameTableId and both findAllDistinctDatabaseIds overloads are unchanged; findAllByFilters keeps entityType as a parameter because general search is caller-parameterized by design. No view-only method is added: nothing here reads views. TABLE_ROW_PREDICATE is the single statement of "null or TABLE" and is reused verbatim in every filtered query including the paged countQuery. With the list and pattern queries hard-coding the table predicate again, the entityType entry in isNonKeyFieldsNullForUserTable is load-bearing once more: it routes a databaseId + entityType=VIEW request to findAllByFilters instead of to a table-only listing. Tests live in services/housetables, where the query actually runs; the predicate was deliberately not replicated into the services/tables H2 doubles. Verified: housetables 177, internalcatalog 87, tables 475, tables-test-fixtures 8, spark-3.5 catalogTest 66 - all green, plus spotlessCheck and checkstyle. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
… parameter /hts/tables and /hts/tables/query are table endpoints, so the queries behind them hard-code the table predicate and entityType is no longer a query parameter anywhere. Views get mirror endpoints in the view-commit ticket. That removes the parameterized type clause entirely: ENTITY_TYPE_FILTER_PREDICATE is deleted and TABLE_ROW_PREDICATE is the single statement of "null or TABLE", appended to every table-named query and repeated verbatim in each paged countQuery through the same constant. No :entityType parameter remains in the repository. Table-scoped reads, all filtered, none parameterized: findTableByDatabaseIdIgnoreCaseAndTableIdIgnoreCase new; getUserTable calls it findAllTablesByDatabaseIdIgnoreCase restored, renamed, filtered findAllTablesByDatabaseIdIgnoreCase(Pageable) restored, renamed, filtered findAllTablesByDatabaseIdAndTableIdLikeAllIgnoreCase renamed, filtered ...(Pageable) renamed, filtered findAllTablesByFilters renamed, filtered, entityType param dropped ...(Pageable) renamed, filtered, entityType param dropped "TableId" in the pattern names is the column table_id, which under a shared key space holds a view's name too, so those names were column-scoped rather than already table-scoped. Neutral and untouched: findByDatabaseIdIgnoreCaseAndTableIdIgnoreCase, which findById delegates to and which putUserTable, deleteUserTable and restoreUserTable need in order to see a row of any type at a shared key; plus existsBy, deleteBy, renameTableId and both findAllDistinctDatabaseIds overloads. No view-only method is added; nothing here reads views. With entityType gone from the query surface, isNonKeyFieldsNullForUserTable and the query branch of OpenHouseUserTableHtsApiValidator are restored to their pre-change form, so listDatabases, listTables, listTablesWithPattern and searchTables route exactly as at base. The transport-model @pattern stays: entityType is still a valid PUT payload field. Because getUserTable is the one HTS endpoint behind every table point read in the tables service, the guards removed earlier are correct by construction. A 404 becomes HouseTableNotFoundException, which doRefresh already catches to leave an empty Optional and refresh from a null location, and which findHouseTable already catches to return empty - so dropTable's existing orElseThrow raises NoSuchTableException, findTableRefById returns empty, and a rename whose source is a view fails inside loadTable. Tests follow the surface: the type-selection tests are replaced by ones asserting the table-scoped families never return a view, and the entityType query parameter is now pinned as bound-but-ignored at the mapper, service and HTTP layers. The predicate was deliberately not replicated into the services/tables H2 doubles. Verified: housetables 175, internalcatalog 87, tables 475, tables-test-fixtures 8, spark-3.5 catalogTest 66 - all green, plus spotlessCheck and checkstyle. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Restores findAllByFilters and findAllByDatabaseIdAndTableIdLikeAllIgnoreCase
to general methods that take entityType, and adds table-scoped default methods
that delegate to them. Nothing is renamed, and the general forms stay available
for the view and neutral work.
One shared ENTITY_TYPE_PREDICATE now spells all three branches out:
null matches any type - genuinely general, not a table default
TABLE matches TABLE and a stored null, because an absent discriminator
means a table on a column that is nullable with no backfill
VIEW matches VIEW
An unrecognized request value matches no branch, so garbage fails closed. Note
this changes what a null entityType means: it used to be a disguised table
default, and it now returns both types, which is why every table caller pins
TABLE explicitly.
Added, all default and owning no JPQL:
findAllTablesByFilters x2
findAllTablesByDatabaseIdAndTableIdLikeAllIgnoreCase x2
findTableByDatabaseIdIgnoreCaseAndTableIdIgnoreCase
The pattern family keeps its own @query because findAllByFilters matches
tableId exactly; folding a LIKE into it would make _ a wildcard and OpenHouse
identifiers routinely contain underscores. It shares the same predicate
constant.
The point read delegates rather than carrying its own query. The alternative
was a dedicated three-clause @query, which would read slightly more directly
but would restate the table branch of a predicate that already exists. Since
the key is the primary key, at most one row can match, so unwrapping the first
element is exact. The tradeoff is that the hottest read in HTS now runs the
general select DISTINCT; the key predicate is still exact, but say the word if
you would rather pay a duplicated clause to avoid the DISTINCT.
Untouched: findByDatabaseIdIgnoreCaseAndTableIdIgnoreCase, which backs findById
for putUserTable, deleteUserTable and restoreUserTable and must see a row of
any type at a shared key; existsBy; deleteBy; renameTableId; and both
findAllDistinctDatabaseIds overloads. No view-only method is added.
Call sites: listTables and searchTables use findAllTablesByFilters,
listTablesWithPattern uses the table-scoped pattern wrapper, and getUserTable
uses the table-scoped point read. entityType is not read from the wire, so
isNonKeyFieldsNullForUserTable and the validator's query branch stay at their
pre-change form and all four routes behave as at base.
Because getUserTable is the one HTS endpoint behind every table point read in
the tables service, the guards removed earlier remain correct by construction:
a 404 becomes HouseTableNotFoundException, which doRefresh already catches to
leave an empty Optional and which findHouseTable already catches to return
empty, so dropTable throws NoSuchTableException, findTableRefById returns
empty, and a rename off a view source fails inside loadTable.
Verified: housetables 177, internalcatalog 87, tables 475,
tables-test-fixtures 8, spark-3.5 catalogTest 66 - all green, plus
spotlessCheck and checkstyle.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
entityType is no longer a parameter anywhere in the query layer. A caller picks a type by picking a method: findAllByFilters returns both types, findAllTablesByFilters returns tables, and findAllViewsByFilters arrives with the view ticket. That drops the delegating-default idea: a typed wrapper cannot tell a parameterless general method what to filter, so each typed method carries its own @query. To avoid restating the filter body, the six general clauses are extracted once into COMMON_FILTER_CLAUSES and the typed sibling composes that constant with TABLE_ROW_PREDICATE. The pattern family is split the same way through PATTERN_KEY_CLAUSES. The extraction is provably behavior-preserving: both findAllByFilters overloads now read "select DISTINCT u from UserTableRow u where " + COMMON_FILTER_CLAUSES, which expands byte-for-byte to the ba400b3 string. The pattern overloads are restored to their ba400b3 form exactly - derived queries with no @query at all. Added, table-scoped, each with its own query composed from the shared constants: findTableByDatabaseIdIgnoreCaseAndTableIdIgnoreCase findAllTablesByFilters x2 findAllTablesByDatabaseIdAndTableIdLikeAllIgnoreCase x2 Nothing is renamed and no view method is added. Unchanged from ba400b3: findByDatabaseIdIgnoreCaseAndTableIdIgnoreCase, which backs findById for putUserTable, deleteUserTable and restoreUserTable and must see a row of any type at a shared key; existsBy; deleteBy; renameTableId; and both findAllDistinctDatabaseIds overloads. The two findAllByDatabaseIdIgnoreCase overloads stay deleted, since findAllTablesByFilters(db, null, ...) covers them, which is what paged listTables already did at base. Call sites: listTables and searchTables use findAllTablesByFilters, listTablesWithPattern uses the table pattern methods, getUserTable uses the table point read. entityType is not read from the wire, so isNonKeyFieldsNullForUserTable and the validator's query branch remain at their pre-change form and all four routes behave as at base. Because getUserTable is the one HTS endpoint behind every table point read in the tables service, the guards removed earlier stay correct by construction: a 404 becomes HouseTableNotFoundException, which doRefresh already catches to leave an empty Optional and which findHouseTable already catches to return empty, so dropTable throws NoSuchTableException, findTableRefById returns empty, and a rename off a view source fails inside loadTable. Verified: housetables 177, internalcatalog 87, tables 475, tables-test-fixtures 8, spark-3.5 catalogTest 66 - all green, plus spotlessCheck and checkstyle. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Reviewers asked for an enum. Introduce EntityType {TABLE, VIEW} and use it
for the HTS-internal representation only: UserTableRow (@Enumerated STRING)
and UserTableDto. The transport model UserTable and internalcatalog's
HouseTable stay String, so a future entity type is not a breaking change
for already-deployed generated clients.
The String <-> enum hop lives in UserTablesMapper, where the transport model
meets the internal ones. It parses case-insensitively, matching what
ENTITY_TYPE_REGEX already accepts, and turns an unrecognized value into a
RequestValidationFailureException so the mapper cannot convert a client
error into a 500 the way MapStruct's implicit Enum.valueOf conversion would.
Neither the stored column text nor the wire representation changes: the
constant names are the text already written, schema.sql is untouched, and
the regenerated HTS OpenAPI spec still declares entityType as a string with
the same pattern.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
entity_type stays VARCHAR(128) DEFAULT NULL, but every UserTableRow loaded from storage now carries a type. Replace @Enumerated(STRING) with an AttributeConverter, because @Enumerated cannot express a default on read. The converter is deliberately asymmetric. Read defaults: a null column is a legacy row and resolves to TABLE. Write does not: TABLE/VIEW/null pass through verbatim, so the column vocabulary is unchanged and no byte moves. Stamping a type onto a write is the endpoint's job in a later step; storage must not invent one. Read parses case-insensitively so hydration agrees with the case-insensitive table predicate that selected the row. Previously a legacy 'table' row was matched by the query and then exploded while loading, which is the worst of both; now matching and hydration are consistent. A value outside the vocabulary is still a hard failure naming the column and the offending value, so corruption cannot masquerade as a table. Consequence: HTS responses now always carry an entityType where a legacy row previously returned none. Tests are updated to assert that. A row built from a request payload never passes through the converter, so its field is still null in memory until the write-side migration lands. The repository queries, schema.sql, the UserTable transport model and internalcatalog's HouseTable are untouched. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The field had zero production readers. It was fed from an openhouse.entityType table property that nothing ever writes, so it was always null, and that null was the only reason stripOhNamespace grew a null check: MapStruct picks that method up as an implicit String -> String conversion and applied it to getEntityType(). The null also rode out to HTS as a null entityType in every commit's PUT payload. Removing the field removes all three. Populating a type is the write side's job and has moved to its own ticket, so nothing in this PR consumes the field. It goes now rather than sitting as speculative plumbing. HTS_FIELD_NAMES is reflected over HouseTable's declared fields, so the set shrinks on its own and openhouse.entityType stops being a recognized property key. ENTITY_TYPE_FIELD_NAME existed only to name that key and goes with it. stripOhNamespace is restored byte for byte to its pre-PR form; its signature is unchanged, since narrowing the return type would silently unwire it from the 20 other String properties it still converts. toUserTable maps to the generated client UserTable, which keeps entityType, so the target is now explicitly ignored rather than incidentally unmapped. The wire contract is untouched: the HTS OpenAPI spec and generated client still declare entityType as a string. Tests that existed only to exercise the field are deleted. That includes the one asserting ordinary commits do not stamp openhouse.entityType: with no field, no code path can write that key, so the assertion no longer pins behavior. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
services/housetables/src/main/resources/schema.sql is a bootstrap file of CREATE TABLE IF NOT EXISTS statements, which is a no-op against an existing table. Production DDL is applied out of band by the MySQL/DDS team, so nothing in the repository records that a schema change happened or in what order. Add services/housetables/ddl/ as a lightweight manual convention: a baseline snapshot of the schema state before entity_type, and the single ALTER TABLE that adds it. The service does not execute these files; they live outside src/main/resources so Spring cannot load them and they are not packaged. Flyway/Liquibase were evaluated and rejected for now. LinkedIn's internal MySQL spec deprecates Flyway for EI/Prod in favor of Pretzel with removal planned for February 2026, and neither tool's validate detects live schema drift, only history/checksum consistency, so under out-of-band execution the machinery adds little. The baseline definitions are derived from schema.sql and are pending verification against production SHOW CREATE TABLE. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Instant add-column is available from MySQL 8.0.12 but eligibility also depends on table-level properties, so the note no longer implies the operation always qualifies. Also states what an explicit algorithm actually buys: an ineligible table fails the statement instead of silently taking a table copy. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
|
Why are we stacked on this pr vs #683 |
| + "lower(u.databaseId) = lower(:databaseId) AND " | ||
| + "lower(u.tableId) = lower(:tableId) AND " | ||
| + TABLE_ROW_PREDICATE) | ||
| Optional<UserTableRow> findTableByDatabaseIdIgnoreCaseAndTableIdIgnoreCase( |
Member
There was a problem hiding this comment.
We need something similar to get view as well right? This query has table row filter, so will return only table.
Collaborator
Author
There was a problem hiding this comment.
see #696 (comment)
the views-related queries are in the other pr
| () -> | ||
| htsJdbcRepository | ||
| .findAllByFilters(userTable.getDatabaseId(), null, null, null, null, null, pageable) | ||
| .findAllTablesByFilters( |
Member
There was a problem hiding this comment.
So most of the methods are renamed because as they are solely for tables. So in that case what are the methods for view that we are supporting in the initial version. I see on common filter for both tables and views (returns list of views). Are there any other methods supported?
Collaborator
Author
There was a problem hiding this comment.
this pr was originally intended to be "just add the entity_type column and make sure tables paths don't access it", so there's no view access methods here. The view access methods are in the next pr here: #697
aastha25
reviewed
Aug 28, 2026
| + "lower(u.databaseId) = lower(:databaseId) AND " | ||
| + "lower(u.tableId) = lower(:tableId) AND " | ||
| + TABLE_ROW_PREDICATE) | ||
| Optional<UserTableRow> findTableByDatabaseIdIgnoreCaseAndTableIdIgnoreCase( |
Contributor
There was a problem hiding this comment.
maybe a comment for the follow up PR, but in general, there should be some query patterns for findAllTablesAndViewsByDatabaseId(AndTableId)LikeAllIgnoreCase. User initiated queries like - list tables in a db should ideally, and have historically, returned all tables and views existing in a db. Why do we only want to get only tables / only views at this layer and let caller initiate 2 calls to HTS? instead we could return all tables + all views and let caller handle usecases in a targetted manner.
Contributor
|
lgtm, in scope of first adding a nullable column 'entity_type' to the mysql 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
4 participants
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.
Summary
This is the first PR toward supporting Iceberg views in OpenHouse. Views will share the
(databaseId, objectId)key space with tables, so this change adds the discriminator that tellsthem apart and makes the existing table queries filter on it.
The new
entity_typecolumn runs from MySQL/H2 through HTS and the generated client.VIEWmeansview;
TABLEand a legacyNULLboth mean table. The column is nullable and not backfilled, soexisting rows and existing table writes are unaffected. Nothing writes
VIEWyet, so this is inertat runtime.
Inside HTS the discriminator is the
EntityTypeenum. It stays aStringon the wire, becauseUserTablegenerates the OpenAPI spec and:client:hts, and an enum there would make a futureentity type a breaking change for already-deployed clients.
StorageTypesets the same precedent.A JPA
AttributeConverterresolves aNULLcolumn toTABLEon read, soentity_typeis nullableonly inside MySQL and total everywhere in Java.
JDBC methods
Entity type is chosen by calling a different method rather than by passing an argument.
findBy…,existsBy…,deleteBy…,findById,existsById,deleteById,renameTableIdfindAllDistinctDatabaseIds×2findAllByFilters×2,findAllByDatabaseIdAndTableIdLikeAllIgnoreCase×2findTableBy…,findAllTablesByFilters×2,findAllTablesByDatabaseIdAndTableIdLikeAllIgnoreCase×2Nothing existing was renamed or changed behaviourally. The shared JPQL moved into constants
(
COMMON_FILTER_CLAUSES,PATTERN_KEY_CLAUSES,TABLE_ROW_PREDICATE) so each table-scoped methodcomposes it, and the expansions were compared to confirm the general queries are byte-identical to
before. View methods follow the same shape and land with the code that calls them.
Three things shaped this:
contains only views, even though it exists and is addressable.
need to see a row of any type to detect a collision.
findAllByFilters, which matchestableIdexactly. Merging aLIKEparameter would make_behave as a wildcard, and identifiers here often contain underscores.
findAllByDatabaseIdIgnoreCase×2 were dropped. The paged one was never called, and pagedlistTablesalready went throughfindAllByFilters, sofindAllTablesByFilterscovers both.Callers need no type logic
HTS returns the correct rows, so none of its callers check entity type. A view 404s from the table point
read, surfaces as
HouseTableNotFoundException, and reads as absent — which makesdoRefresh,dropTable,findTableRefById, and rename-source all correct as written.dropTablematters herebecause it bypasses
loadTableto survive corrupted metadata, so a guard on the refresh path wouldhave missed it.
Name occupancy — "what is at this key?" — needs to see rows of any type, and its only callers are
CREATE TABLEand the rename-destination check. That lands with the view work.The tables service stays out of it
An earlier revision put an
entityTypefield on the internalHouseTablepointer. It had noconsumer, and it was fed from an
openhouse.entityTypeproperty that nothing writes, so it wasalways null — which then forced a null check into
HouseTableMapper.stripOhNamespace, a methodMapStruct applies to every String property on the mapper.
Both are gone.
stripOhNamespaceis byte-identical tomainagain, and the wholeiceberg/diffis one
@Mapping(target = "entityType", ignore = true). The discriminator is owned by HTS; thetables service has no knowledge of it.
Tests
Existing tests cover every changed call site in
UserTablesServiceImpl—testGetUserTables,testUserTableQuery,testGetUserTablesWithTablePattern,testGetUserTablesWithSearchFilter,testUserTableGet,testListDatabases. All still pass unmodified: the diff on that test class is247 insertions, 0 deletions, and no existing assertion anywhere in this PR was deleted or
relaxed. Since the table behaviour was meant to be unchanged, those tests are the regression proof.
New tests were written before the implementation. They cover both-types vs tables-only results
across the filter and pattern families, page counts with views interleaved, the point read treating
a view as absent while the neutral read still returns it, and unrecognised discriminators failing
closed. Page assertions check content, size, total elements, and total pages, so filtering a
returned page instead of the query would fail them. Case handling is asserted in Java, since H2 in
MODE=MySQLis case-sensitive and production MySQL is not.services:housetablesiceberg:openhouse:internalcatalogservices:tablesiceberg:openhouse:htscatalogtables-test-fixtures_2.12(Iceberg 1.2)tables-test-fixtures-iceberg-1.5_2.12Both fixture variants compile and the 1.2 fixture's tests run, since
HouseTableRepositoryisinherited by Spring Data proxies in published fixture code.
Rollout
schema.sqlusesCREATE TABLE IF NOT EXISTS, so production needsALTER TABLE user_table_row ADD COLUMN entity_type VARCHAR(128) DEFAULT NULLbefore deploying this pr. No backfill needed. Deploy HTS before the tables service, since the filtering lives in HTS.That DDL is also recorded under
services/housetables/ddl/, as a baseline snapshot plus theALTER. The files are inert — outsidesrc/main/resources, so Spring cannot execute them andGradle does not package them — and exist only so the sequence of schema changes is captured in the
repository. The baseline is derived from
schema.sqland is marked pending verification againstproduction
SHOW CREATE TABLE. Migration tooling was evaluated and deferred; Flyway is deprecatedinternally in favour of Pretzel, tracked in BDP-108649.