Add entityType discriminator and table-scoped HTS queries - #696
Merged
Merged
Conversation
Collaborator
|
Why are we stacked on this pr vs #683 |
Contributor
|
lgtm, in scope of first adding a nullable column 'entity_type' to the mysql table |
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>
ruolin59
force-pushed
the
rufan/views-entity-type-discriminator
branch
from
September 1, 2026 21:25
96d4e7c to
47ac84e
Compare
The mapper comment restated that HouseTable has no entityType without saying why that is correct: the pointer has no source for the type, and the endpoint the write arrived on does. The converter's NULL branch carried no note at all that a legacy row means TABLE. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
abhisheknath2011
previously approved these changes
Sep 3, 2026
aastha25
approved these changes
Sep 10, 2026
…TS (alternative to #697) (#703) ## What this is An **alternative implementation of BDP-108627**, the same ticket as **#697**. It is meant to be compared against that PR, not stacked on it. Both branch from `rufan/views-entity-type-discriminator`. The premise: keep #697's HTTP contract and its SQL layer as they are, and rebuild the part between them — the CRUD wiring and error handling. One deliberate exception, called out below: views are paginated-only, so the non-paginated `GET /hts/views/query` that #697 mirrored from tables is dropped. | | #697 | this PR | |---|---|---| | HTTP API | — | **identical except views are paginated-only** — the one non-paginated view route is removed by design (see below); every other route, verb, param, body and status is unchanged | | SQL / JPQL | — | **identical** apart from removing the two now-dead non-paginated view finders; every constant, table finder and paginated view finder is verbatim | | Controller ↔ repository wiring | as landed | rebuilt | ## What is deliberately unchanged **The HTTP surface.** `GET /hts/entities` and every pre-existing table route keep their paths, verbs, parameters, defaults, bodies and status codes. The view routes match #697's with one exception: there is no non-paginated `GET /hts/views/query` (see the next section). **The SQL layer.** Every JPQL constant (`COMMON_FILTER_CLAUSES`, `PATTERN_KEY_CLAUSES`, `TABLE_ROW_PREDICATE`, `VIEW_ROW_PREDICATE`, `STAMP_TABLE_TYPE`), every query and `countQuery`, the typed conditional deletes returning an affected-row count, and the type-scoped rename are carried over verbatim. `TABLE_ROW_PREDICATE` keeps its `IS NULL` arm, so legacy rows still read as tables. ## JDBC repository surface (`UserTableHtsJdbcRepository`) An accounting of the MySQL-backed repository methods, grouped by the entity type each targets. "New" and "reshaped" are relative to the base branch `rufan/views-entity-type-discriminator`, which already carried the table-typed **reads** (the frozen tables path) but only **neutral writes**. **View-scoped — every method is new in this PR** (the repository had no view methods before): | Method | Returns | Purpose | |---|---|---| | `findViewByDatabaseIdIgnoreCaseAndTableIdIgnoreCase(db, table)` | `Optional<UserTableRow>` | point read under `VIEW_ROW_PREDICATE` | | `findAllViewsByFilters(db, table, version, metadataLocation, storageType, creationTime, Pageable)` | `Page<UserTableRow>` | paginated multi-attribute filter | | `findAllViewsByDatabaseIdAndTableIdLikeAllIgnoreCase(db, pattern, Pageable)` | `Page<UserTableRow>` | paginated name-pattern list | | `deleteViewByDatabaseIdIgnoreCaseAndTableIdIgnoreCase(db, table)` | `int` | conditional delete under `VIEW_ROW_PREDICATE`, affected-row count | | `deleteViewById(key)` | `int` | default delegating to the delete above | Plus the constants `VIEW` and `VIEW_ROW_PREDICATE`. There is deliberately **no** non-paginated view finder, no `findViewById` default, and no view rename: views are paginated-only and are not renameable. **Table-scoped:** - `findTableByDatabaseIdIgnoreCaseAndTableIdIgnoreCase → Optional`, `findAllTablesByDatabaseIdAndTableIdLikeAllIgnoreCase` (`Iterable` and `Page`), `findAllTablesByFilters` (`Iterable` and `Page`) — *inherited from base*, unchanged. - `deleteTableByDatabaseIdIgnoreCaseAndTableIdIgnoreCase → int` and `deleteTableById(key) → int` — **new**: replace the base's single neutral `void delete…`, now scoped by `TABLE_ROW_PREDICATE` and returning an affected-row count so the service maps missing and wrong-type alike to 404. - `renameTableId(…) → int` — **reshaped**: the base's neutral `void renameTableId` gains `TABLE_ROW_PREDICATE` scoping and `STAMP_TABLE_TYPE` (rewriting a legacy null or non-canonical spelling as the row moves), and returns a row count. Table-only because views are not renameable. **Both (type-neutral):** - `findByDatabaseIdIgnoreCaseAndTableIdIgnoreCase → Optional` and `existsByDatabaseIdIgnoreCaseAndTableIdIgnoreCase → boolean` — *inherited*; the writers read through these so they can see a row of any type and detect a collision at a shared key, which is why they stay unfiltered. - `findAllByDatabaseIdAndTableIdLikeAllIgnoreCase` (`Iterable` and `Page`), `findAllByFilters` (`Iterable` and `Page`), `findAllDistinctDatabaseIds()` and its paginated form — *inherited*. - `findById` / `existsById` defaults — *inherited*. - `deleteById`, `delete`, `deleteAllById`, `deleteAll(Iterable)`, `deleteAll()` — **sealed in this PR** to throw `UnsupportedOperationException`, forcing callers onto the typed deletes; the base's neutral `deleteBy…` derived delete is removed. ## What is rebuilt, and why **A view write cannot persist a table.** `putUserTable` and `putUserView` each supply their own `EntityType` to one private persistence primitive, so the named method — not the payload — is the authority. In #697, `putView` delegates to `putUserTable` and relies on the controller having stamped the type; a direct Java caller could persist the wrong one. **"Not found" cannot be manufactured from a failure.** The new point reads return `Optional`, and only the handler turns empty into a 404. A dependency or hydration failure travels as an exception and surfaces as a 500. This matters most for the neutral read, where the ticket calls out "the name is free" as the dangerous default. **The service owns its view-query vocabulary.** View queries cross the boundary as an owned `UserViewQuery`, built once in the handler after validation, so the transport `UserTable`'s nullability and its inert `entityType` stop at the handler. The invalid "table pattern without a database" state cannot be constructed. **Corruption is translated at the repository boundary, structurally.** An `@Around` aspect (`UserTableRepositoryTranslationAspect`) wraps every `UserTableHtsJdbcRepository` method: when a corrupt discriminator surfaces during hydration it is rethrown as an unwrapped `CorruptEntityTypeException`, and every other `DataAccessException` is rethrown exactly as it arrived, so non-corrupt failures behave as before. Because it is an aspect over the whole repository rather than a wrap at each call site, a read added later is covered without anyone remembering to wrap it. The interception point is load-bearing and can fail silently — Spring Data's own proxy holds `PersistenceExceptionTranslationInterceptor`, and the aspect must land in the outer proxy to see the already-translated `DataAccessException` — so a structural test pins it (`RepositoryTranslationAspectTest`) and a Spring Data upgrade that relocated the interceptor fails loudly rather than downgrading every corrupt read to a generic 500. Because corruption is unwrapped before it leaves the repository, the shared advice gains **one handler and no ORM vocabulary**. #697 instead teaches `services/common` to import and unwrap `JpaSystemException` and `InvalidDataAccessApiUsageException`. `CorruptEntityTypeException` does not extend `IllegalArgumentException`, so stored-data corruption stays distinguishable from client-input error. **Views are paginated-only.** #697 gave views a non-paginated `GET /hts/views/query` mirroring `GET /hts/tables/query`. This PR drops it: views can be listed only through the paginated `GET /v1/hts/views/query`. The non-paginated table query is legacy — it still backs the tables service's `/v0` and `/v1` list-all, and its empty-filter form returns database names rather than tables — and a trace confirmed neither the tables service nor the internal catalog references any view query path. Mirroring that shape would inherit tech debt into a greenfield surface with no caller, so on this new path we took the more principled option, which also removes the unbounded-query concern for views entirely. Tables are untouched: `GET /hts/tables/query` stays, because it has live callers. **Smaller ones.** PUT ingress validation is an injectable, unit-testable component that throws `RequestValidationFailureException` directly, rather than a private static helper in the controller. No-arg `deleteAll()` is sealed alongside the four key-addressed deletes, with teardown moved to a test fixture. View metrics are owned by housetables; the pre-existing `HTS_`/`REPO_` constants stay in `services/common` because `services/tables` consumes 19 of them. ## Footprint on shared code ``` CorruptEntityTypeException.java +19 (new) OpenHouseExceptionHandler.java +22 (one @hidden handler, 0 deletions) ``` `ServiceAuditAspect` and every other file in `services/common` are byte-identical to base. ORM imports added to `services/common`: **0** (#697 adds 3). The translation mechanism itself — a small `CorruptEntityTypeTranslation` utility and the aspect that applies it — lives in `services/housetables`, the module that already owns JPA. ## Deliberate asymmetries Two remain, both in signatures rather than behaviour: `getUserTable` signals absence by exception while the new point reads return `Optional`, and table queries take the transport `UserTable` while view queries take an owned type. Both were measured rather than assumed. Unifying the absence convention costs 16 pre-existing test edits and risks a silent wire change — the current 404 carries `cause: null` because it wraps a message-less `NoSuchElementException`, and no test pins that field. Unifying the query vocabulary means rewriting 11 private query helpers and 3 branch predicates that encode the frozen empty-filter behaviour. Both are worth doing on their own terms, with their own review; neither has view-specific content. ## Not adopted - **A generic corruption 500 body.** Would make it the only differently-shaped 500 in the service. - **Removing the `.stacktrace(...)` sites from the shared handler.** These do not leak: `ServiceAuditAspect` already strips the stack trace from the client body and keeps it only in the internal audit event, so deleting the sites would degrade the audit trail without changing any client response. The populate-then-strip indirection is real, but it is a `services/common` contract shared by tables, jobs and housetables — a whole-platform refactor, not a views concern. The views-specific error-handling improvement Mike's review asked for was instead made through the corruption path (`CorruptEntityTypeException` + the repository aspect), which gives a corrupt read a proper diagnostic 500 rather than raw ORM vocabulary. - **The soft-delete version TOCTOU.** Pre-existing; needs a version-guarded delete and its own interleaving test, so tracked separately rather than folded into a views PR. The production collation, an open question across both PR stacks, is now confirmed `utf8mb4_0900_ai_ci` — accent-insensitive and NO PAD. This needs no code change: it means a corrupt `'TÁBLE'` matches the type predicate and fails hydration with a diagnostic 500, and a corrupt `'TABLE '` is invisible to the typed routes. Both require a direct database write, since the API writes only canonical enum names, so neither is a reachable state. ## Testing | Module | Tests | Failures | |---|---|---| | `services:housetables` | 404 | 0 | | `services:common` | 12 | 0 | | `services:tables` | 526 | 0 | | `services:jobs` | 52 | 0 | `tables` (526) and `jobs` (52) match their base counts after this branch was rebased onto the current base. `:client:hts` regenerates cleanly with **five** new operations — `getEntity`, `getUserView`, `getPaginatedUserViews`, `putUserView`, `deleteView` (no non-paginated `getUserViews`). The generated OpenAPI response sets match #697's for every route that still exists, including a documented `500` on the three GET routes that can reach a corrupt row (`/hts/entities`, `/hts/views`, `/v1/hts/views/query`). The change was also driven through #698's deployment e2e suite against a real MySQL 8.4 container: **23 passed, 0 skipped**. That suite was written against #697's API; the only edit needed was to route its view-listing through the paginated endpoint, since the non-paginated one is intentionally gone — every other assertion passed unchanged, which is independent evidence the rest of the contract is indistinguishable to a client. The confirmed production collation (`utf8mb4_0900_ai_ci`) is exercised by that run. Pre-existing tests were changed only where the frozen contract or an adopted decision forced it, and each was strengthened rather than weakened; the three teardown edits come from sealing `deleteAll()`. No assertion was deleted or relaxed. ## A note on a pre-existing flake `UserTablesServiceTest.testUserTablePurge` failed twice under concurrent multi-module load and passed 6/6 in isolation. `SoftDeletedUserTablesMapper` derives `deletedAtMs` from `Instant.now()`, and that field is part of the soft-deleted composite key; the test soft-deletes the same table twice and expects two rows, so two calls in the same millisecond upsert into one. The mapper, the key and that test are untouched here, and #697 carries identical code. ## Review process Design-first: an implementation plan reviewed independently over two rounds, tests written before the production logic and reviewed against it, then an implementation review, followed by several rounds of simplification that removed roughly 1,400 lines of machinery in favour of the smallest thing that holds the contract. Load-bearing mechanics were verified empirically rather than assumed — most importantly the aspect's interception point, proven against Spring 5.3.25 by dumping the nested proxy chains: the aspect sits in a separate outer proxy from Spring Data's exception-translation interceptor, so no `@Order` value could move it, and a negative control (aspect disabled) confirmed the corrupt-row tests genuinely depend on it. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
abhisheknath2011
approved these changes
Sep 10, 2026
ruolin59
added a commit
that referenced
this pull request
Sep 19, 2026
## Production incident HTS in `prod-ltx1` saturated and began refusing TCP connections at ~11:35 PDT on 2026-09-18, breaking Airflow partition sensors with `Connection refused ... :4768`. `user_table_row` has ~513,000 rows and a functional index: ``` idx_user_table_upper_db_table ON (upper(database_id), upper(table_id)) ``` A functional index matches only the **exact** expression, so handwritten `lower(...)` cannot use it. `getUserTable` runs at roughly 6,000 QPS; a full scan per request exhausted MySQL connections and HTS worker threads, and pods stopped accepting connections. ## What regressed Before eca3ca3 (#696), `getUserTable` went through `findById(key)`, a `default` method delegating to the Spring-derived `findByDatabaseIdIgnoreCaseAndTableIdIgnoreCase`. Spring Data emits `upper(...)` for `IgnoreCase` (`JpaQueryCreator$PredicateBuilder.upperIfIgnoreCase`), so it matched the index. That is why the index is on `upper`. #696 replaced that call with a new explicit `@Query` using handwritten `lower(...)`, following the file's existing convention for hand-written queries. The method name still reads like the derived finder it displaced, which is how it passed review. The six pre-existing `lower(...)` queries were all on cold paths — filters, search, rename. They had been scanning for a long time without anyone noticing, because a 500ms scan at low QPS is invisible. Moving the hot path onto that convention is what turned it into an outage. ## The fix `lower(` → `upper(` at all 30 token occurrences across the 15 comparison sites in `UserTableHtsJdbcRepository`, both sides of every comparison. Nothing else. Production `EXPLAIN ANALYZE`, measured on the real table: | | plan | rows | time | |---|---|---|---| | before | `Table scan on user_table_row` | 513,418 | ~495ms | | after | `Index lookup using idx_user_table_upper_db_table` | 1 | ~0.041ms | The `entity_type` predicate is **not** the cause. In the fixed plan it is demoted to a cheap residual filter above the index lookup. It is implicated only because it arrived in the same commit. ### Why this is semantically safe Production collation is confirmed `utf8mb4_0900_ai_ci`, which is case- and accent-insensitive. Under it, `lower(a) = lower(b)`, `upper(a) = upper(b)` and `a = b` are equivalent. The wrapping affects index eligibility, not which rows match. At the sole `LIKE` site both column and pattern fold with the same function, and `%`, `_` and escapes are non-alphabetic so `upper()` leaves them byte-identical — wildcard semantics are unchanged. ## Tests Two tests added to `HtsRepositoryTest`, covering cross-case matching for the filter and `LIKE` families. Those families were previously exercised only with same-case data, so a botched substitution could have slipped through; point reads, rename and deletes already had cross-case coverage. **These tests pass both before and after the change, and that is deliberate.** There is no red phase because the change is behaviour-neutral by design. **What the tests prove:** behaviour preservation across case for the affected query families. **What they cannot prove:** index selection, scan avoidance, or latency. Tests run against H2 in MySQL mode, which has no functional indexes and no meaningful planner. The performance claim rests solely on the production `EXPLAIN ANALYZE` above. Suites pass on JDK 11: housetables 406, common 12, zero failures, errors or skips. ## Deliberately out of scope **`SoftDeletedUserTableHtsJdbcRepository`** has 24 `lower(` tokens across 12 lines on `soft_deleted_user_table_row`. That is a different physical table whose indexes are unconfirmed — flipping it blind could be a no-op or a pessimisation. Its paths are cold (restore, purge, querying deleted tables). Needs its own `SHOW INDEX` before anyone touches it. **The schema record is corrected in this PR.** `services/housetables/ddl/0000__baseline.sql` previously recorded only `PRIMARY KEY (database_id, table_id)` for `user_table_row`, and its own header warned that a derived definition "cannot capture secondary indexes". So the index this fix depends on was documented nowhere, and anyone reconstructing the table from that file would have reintroduced this outage. `user_table_row` and `soft_deleted_user_table_row` are now transcribed from production `SHOW CREATE TABLE`. For `user_table_row` that closed more than the index: `database_id`/`table_id` were recorded as `varchar(128)` but are `varchar(255)`, `metadata_location` as `varchar(512)` but is `varchar(255)`, `version` as `NOT NULL` but is nullable, `last_modified_time` was recorded but does not exist, and `table_version` and `deleted_ts` exist but were absent. Engine, charset and collation were missing from both tables. `soft_deleted_user_table_row` has no secondary index, and that is now recorded as the real state rather than an omission. `job_row` and `table_toggle_rule` are deliberately untouched — no production output was available for them, and guessing would recreate exactly the failure this PR is fixing. The header now says which two tables are verified and which two are not. This also explains why no local or containerised MySQL could have caught the regression: the `oh-only-mysql` recipe bootstraps from this DDL, so a local database had no functional index and `lower()` versus `upper()` was indistinguishable there. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
teamurko
pushed a commit
that referenced
this pull request
Sep 22, 2026
#758) (#761) ## Summary Reverts two related commits: - #696 "Add entityType discriminator and table-scoped HTS queries" - #758 "Use upper-case HTS predicates to match the functional index" #758 was a hotfix for a production incident (HTS connection saturation) caused by `getUserTable` scanning the full `user_table_row` table instead of using the `idx_user_table_upper_db_table` functional index, a regression introduced by #696's handwritten `lower(...)` predicates. This PR reverts both changes back to the pre-#696 state, restoring the original `findByDatabaseIdIgnoreCaseAndTableIdIgnoreCase`-based query path (which naturally matched the functional index) and removing the `entity_type` discriminator column, `EntityType` enum, and related JDBC/API/test surface added by #696. A follow-up PR will reintroduce both changes together, combined into a single commit, so the entityType feature and its required index-compatible predicate fix land atomically. ## Test plan - `./gradlew :services:housetables:test :services:common:test` passes. - `./gradlew spotlessCheck` passes (run with `-x CopyGitHooksTask`, a pre-existing worktree-incompatibility in the git-hooks Gradle task, unrelated to this change). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
ruolin59
added a commit
to ruolin59/openhouse
that referenced
this pull request
Sep 22, 2026
…#696, linkedin#758 combined) Combines two previously-separate commits into one so the entityType discriminator feature and its required index-compatible predicate lands atomically: - Add entityType discriminator and table-scoped HTS queries (originally linkedin#696) - Use upper-case HTS predicates to match the functional index (originally linkedin#758, a hotfix for a production incident caused by linkedin#696's lower(...) predicates bypassing idx_user_table_upper_db_table) See the reverted PR (linkedin#761 revert of linkedin#696/linkedin#758) for background on why these two were split apart and are now being reintroduced together. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
ruolin59
added a commit
to ruolin59/openhouse
that referenced
this pull request
Sep 22, 2026
…fix (linkedin#696, linkedin#758)" (linkedin#761) This reverts the revert in linkedin#761, restoring linkedin#696 and linkedin#758 combined into a single commit: - Add entityType discriminator and table-scoped HTS queries (originally linkedin#696) - Use upper-case HTS predicates to match the functional index (originally linkedin#758, a hotfix for the production incident caused by linkedin#696's handwritten lower(...) predicates bypassing the idx_user_table_upper_db_table functional index) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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.
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.