feat(amber): decouple the Lakekeeper catalog name from the user-facing name - #7754
Open
mengw15 wants to merge 4 commits into
Open
feat(amber): decouple the Lakekeeper catalog name from the user-facing name#7754mengw15 wants to merge 4 commits into
mengw15 wants to merge 4 commits into
Conversation
…g name The catalog name was derived from the display name: user-<uid>-<name>. That one string is also the REST catalog prefix, the S3 key prefix and a component of every result URI an execution wrote into the warehouse, so the display name was frozen at creation and a warehouse could never be renamed -- unlike a computing unit, whose name is pure display metadata because cuid is the identity everywhere else. Derive the catalog name from the row id instead: user-<uid>-<whid>. The id is drawn from the table's sequence before the Lakekeeper call, so the creation order is unchanged (Lakekeeper first, row after, with the compensating delete) and no schema or nullability change is needed. The sequence is resolved through pg_get_serial_sequence rather than named literally, because the generated name is not a stable contract -- the jOOQ output already carries both user_warehouse_whid_seq and ..._seq1. A sequence-derived name also cannot collide, so it needs no retry path. A random suffix would have needed one, and that retry would have to recognise Lakekeeper's name-conflict error -- the same brittle response parsing apache#7742 just had to harden. Rename user_warehouse.warehouse_name to lakekeeper_warehouse_name to sit beside lakekeeper_warehouse_id; the table already has its own `name` column, and the value is no longer a name in any user-facing sense. The wire DTO keeps warehouseName. The table is empty in every deployment while the flag is off, so the rename carries no data -- but it still needs a schema migration (sql/updates/38.sql): texera_ddl.sql is CREATE TABLE IF NOT EXISTS, and jOOQ generates from the live database. Closes apache#7753.
Contributor
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR decouples the Lakekeeper catalog/warehouse identifier from the user-facing warehouse display name by deriving the Lakekeeper name from the user_warehouse.whid sequence ID, enabling future warehouse renames without affecting storage/catalog paths.
Changes:
- Rename
user_warehouse.warehouse_nametolakekeeper_warehouse_nameand add a Liquibase migration. - Update warehouse creation to pre-allocate
whidfrom the sequence and mintuser-<uid>-<whid>for Lakekeeper. - Update affected services and tests to use
lakekeeper_warehouse_nameand assert the new naming behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| sql/updates/38.sql | Adds migration to rename the warehouse name column for the new semantics. |
| sql/texera_ddl.sql | Updates base DDL to use lakekeeper_warehouse_name column name. |
| sql/changelog.xml | Registers migration 38 in Liquibase changelog. |
| common/dao/src/test/scala/org/apache/texera/dao/UserWarehouseSpec.scala | Switches test to column rename (but still asserts old name semantics). |
| amber/src/test/scala/org/apache/texera/web/service/WorkflowServiceWarehouseSpec.scala | Updates test inserts to use lakekeeper_warehouse_name. |
| amber/src/test/scala/org/apache/texera/web/service/ExecutionsMetadataPersistServiceSpec.scala | Updates test inserts to use lakekeeper_warehouse_name. |
| amber/src/test/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowExecutionsResourceSpec.scala | Updates test inserts to use lakekeeper_warehouse_name. |
| amber/src/test/scala/org/apache/texera/web/resource/dashboard/user/warehouse/WarehouseResourceSpec.scala | Updates assertions to user-<uid>-<whid> and adds coverage for name reuse behavior. |
| amber/src/main/scala/org/apache/texera/web/service/WorkflowService.scala | Reads Lakekeeper warehouse name from the renamed column. |
| amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/warehouse/WarehouseResource.scala | Mints Lakekeeper name from pre-allocated whid sequence value and persists it to DB. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
github-actions
Bot
added
feature
engine
ddl-change
Contributor
Automated Reviewer SuggestionsBased on the
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7754 +/- ##
============================================
+ Coverage 91.02% 91.03% +0.01%
- Complexity 4454 4458 +4
============================================
Files 1174 1174
Lines 47146 47156 +10
Branches 5287 5287
============================================
+ Hits 42916 42930 +14
Misses 2550 2550
+ Partials 1680 1676 -4
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 505 | 0.308 | 20,702/31,261/31,261 us | 🔴 +132.5% / 🔴 +92.7% |
| 🔴 | bs=100 sw=10 sl=64 | 1,091 | 0.666 | 83,518/140,827/140,827 us | 🔴 +42.7% / 🔴 +26.8% |
| 🟢 | bs=1000 sw=10 sl=64 | 1,408 | 0.86 | 707,370/784,481/784,481 us | 🟢 +25.4% / 🟢 +41.7% |
Baseline details
Latest main fb9f4e2 from 2026-08-18T14:33:07.467Z
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 505 tuples/sec | 835.59 tuples/sec | 743.3 tuples/sec | -39.6% | -32.1% |
| bs=10 sw=10 sl=64 | MB/s | 0.308 MB/s | 0.51 MB/s | 0.454 MB/s | -39.6% | -32.1% |
| bs=10 sw=10 sl=64 | p50 | 20,702 us | 11,611 us | 13,204 us | +78.3% | +56.8% |
| bs=10 sw=10 sl=64 | p95 | 31,261 us | 13,446 us | 16,220 us | +132.5% | +92.7% |
| bs=10 sw=10 sl=64 | p99 | 31,261 us | 17,885 us | 19,867 us | +74.8% | +57.3% |
| bs=100 sw=10 sl=64 | throughput | 1,091 tuples/sec | 1,091 tuples/sec | 959.52 tuples/sec | +0.0% | +13.7% |
| bs=100 sw=10 sl=64 | MB/s | 0.666 MB/s | 0.666 MB/s | 0.586 MB/s | +0.0% | +13.7% |
| bs=100 sw=10 sl=64 | p50 | 83,518 us | 89,031 us | 103,974 us | -6.2% | -19.7% |
| bs=100 sw=10 sl=64 | p95 | 140,827 us | 98,690 us | 111,036 us | +42.7% | +26.8% |
| bs=100 sw=10 sl=64 | p99 | 140,827 us | 101,358 us | 118,982 us | +38.9% | +18.4% |
| bs=1000 sw=10 sl=64 | throughput | 1,408 tuples/sec | 1,123 tuples/sec | 994.68 tuples/sec | +25.3% | +41.6% |
| bs=1000 sw=10 sl=64 | MB/s | 0.86 MB/s | 0.686 MB/s | 0.607 MB/s | +25.4% | +41.7% |
| bs=1000 sw=10 sl=64 | p50 | 707,370 us | 888,033 us | 1,011,206 us | -20.3% | -30.0% |
| bs=1000 sw=10 sl=64 | p95 | 784,481 us | 934,711 us | 1,055,603 us | -16.1% | -25.7% |
| bs=1000 sw=10 sl=64 | p99 | 784,481 us | 961,294 us | 1,084,808 us | -18.4% | -27.7% |
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,396.30,200,128000,505,0.308,20701.72,31261.19,31261.19
1,100,10,64,20,1833.81,2000,1280000,1091,0.666,83518.46,140826.80,140826.80
2,1000,10,64,20,14201.52,20000,12800000,1408,0.860,707369.61,784480.68,784480.68The column rename left the same ambiguity in the code: a method called resolveWarehouseName and a DTO field called warehouseName both carry the Lakekeeper catalog name while sitting next to a `name` that means the display name. Rename them to resolveLakekeeperWarehouseName and lakekeeperWarehouseName, so all three -- column, method, DTO field -- say what they hold. WorkflowContext.warehouse keeps its name: it feeds a chain of parameters and fields all called `warehouse` (VFSURIFactory here and its Python counterpart), none of which sit beside a display name, so renaming only this end would break that consistency instead of removing ambiguity. A comment records what it holds. LakekeeperClient's own `warehouseName` parameters likewise stay -- inside that client the term is unambiguous.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/warehouse/WarehouseResource.scala:60
- Renaming this case-class parameter changes the serialized field returned by both
POST /warehouseandGET /warehouse/statustolakekeeperWarehouseName. The PR description explicitly says the wire DTO keepswarehouseName, so this introduces an API-contract break unrelated to the database-column rename. Keep or explicitly map the JSON property towarehouseNameand adjust the Scala-facing tests accordingly.
lakekeeperWarehouseName: String,
texera_ddl.sql already creates the column under its new name, so on a fresh local-dev database the rename hit a column that was never there and aborted `up`: the replay of unrecorded change sets only tolerates "already exists", and PostgreSQL reports "column warehouse_name does not exist". Guard it with the same existence check 33.sql uses for its rename. Verified against a live database both ways -- a schema already holding lakekeeper_warehouse_name is left untouched, one still holding warehouse_name is renamed. Also take the qualified table and column names for the sequence lookup from the jOOQ metadata instead of a literal texera_db.user_warehouse, so it follows whatever schema the generated code targets, and rename UserWarehouseSpec's fixture parameter (with a note) since the old value read like a naming rule the DAO layer does not own.
mengw15
force-pushed
the
feat/7753-decouple-catalog-name
branch
from
August 18, 2026 20:12
ecb5aff to
36293cc
Compare
`warehouseId` named two unrelated things: the user_warehouse row id we pick (an Int, the table's whid) and the id Lakekeeper assigns its own warehouse entity (a UUID, stored in lakekeeper_warehouse_id). The two never mean the same thing, so reading a call site required checking the type first. Name them after what they identify: `whid` for ours, matching the column and the DashboardWarehouse field, and `lakekeeperWarehouseId` for Lakekeeper's, matching its column and sitting beside lakekeeperWarehouseName. Anything prefixed `lakekeeper` is now an identifier owned by that system. WorkflowExecuteRequest.warehouseId becomes `whid` as part of this. It is a wire field, but nothing sends it yet -- the warehouse frontend is still an unmerged draft -- so renaming it now costs nothing, whereas doing it after that lands would mean changing a live contract.
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.
What changes were proposed in this PR?
WarehouseResourcederived the Lakekeeper catalog name from the user-facing name:user-<uid>-<name>. That one string is simultaneously the Lakekeeper warehouse identifier, the REST catalog URL prefix, the S3 key prefix, and a component of every result URI stored by executions that wrote into the warehouse — so the display name was frozen at creation and a warehouse could never be renamed. Computing units rename freely precisely because their name is pure display metadata;cuidis the identity everywhere else.user-<uid>-<whid>. The id is drawn from the table's own sequence before the Lakekeeper call, so the creation order is unchanged — Lakekeeper first, DB row after, with the existing compensating delete — and no schema or nullability change is needed. The sequence is resolved throughpg_get_serial_sequencerather than named literally, because the generated sequence name is not a stable contract (the jOOQ output already carries bothuser_warehouse_whid_seqand..._seq1from a re-created table).namestays the per-user-unique display name, now free to change; a rename endpoint mirroring computing-unit rename becomes a straightforward follow-up.user_warehouse.warehouse_name→lakekeeper_warehouse_name, matching its siblinglakekeeper_warehouse_id.resolveWarehouseName→resolveLakekeeperWarehouseNameand the DTO fieldwarehouseName→lakekeeperWarehouseName, so the column, the method and the field all say which name they hold.WorkflowContext.warehousekeeps its name — it feeds a chain of parameters all calledwarehouse(VFSURIFactoryhere and in Python), none of which sit beside a display name.warehouseIdnamed both theuser_warehouserow id we pick (anInt) and the id Lakekeeper assigns its own entity (aUUID), so a call site had to be read type-first. They are nowwhidandlakekeeperWarehouseId, matching their columns; anything prefixedlakekeeperis an identifier owned by that system.WorkflowExecuteRequest.warehouseIdbecomeswhidas part of this — it is a wire field, but nothing sends it yet (the warehouse frontend is an unmerged draft), so renaming now costs nothing while doing it later would change a live contract.Deviation from the issue, and why. The issue proposed
user-<uid>-<8 random hex>, on the reasoning that deriving fromwhid"would need the DB row before the Lakekeeper create — an order flip plus a nullable column". Taking the id from the sequence up front avoids both, so that cost does not apply. Doing so also removes the collision-retry path the random suffix required: a 32-bit suffix collides often enough to need one, and that retry would have to recognise Lakekeeper's name-conflict error — the same brittle response-parsing #7742 just had to harden. A sequence-derived name cannot collide, anduser-7-42points straight atwhid = 42when tracing storage back to a row.The migration is guarded.
texera_ddl.sqlnow creates the column under its new name, so on a fresh database the column to rename was never there — and the local-dev replay of unrecorded change sets tolerates only "already exists", so an unguardedRENAME COLUMNabortsupwith "column warehouse_name does not exist".38.sqlguards the rename with the same existence check33.sqluses, verified against a live database both ways: a schema already holdinglakekeeper_warehouse_nameis a clean no-op, one still holdingwarehouse_nameis renamed.On "zero migration". No data migration is needed — the table is empty in every deployment while the flag is off — but the column rename still needs a schema migration (
sql/updates/38.sql):texera_ddl.sqlisCREATE TABLE IF NOT EXISTS, so an existing database keeps the old column, and jOOQ generates its code from the live database. Without the migration, existing databases would generateWAREHOUSE_NAMEand fail to compile against this change.Any related issues, documentation, discussions?
Closes #7753. Part of #6870, follow-up to #6932. Worth settling while the flag is off everywhere: once real data exists under name-derived prefixes, this becomes a migration project.
How was this PR tested?
WarehouseResourceSpecnow asserts the catalog name equalsuser-<uid>-<whid>and does not contain the display name, pinning the decoupling itself.user-<uid>-<name>, which this change makes unreachable. They now draw an id from the sequence, set it explicitly on the squatter row (so storing it consumes nothing further), and squat on the next one — so they still exercise theUNIQUEconflict, and additionally pin the "catalog name = uid + sequence id" rule.WarehouseResourceSpec,WorkflowServiceWarehouseSpec,ExecutionsMetadataPersistServiceSpec,LakekeeperClientSpecandWorkflowServiceSpecrun locally: 38/38 passed, plusUserWarehouseSpec4/4, against a database migrated withsql/updates/38.sqland jOOQ regenerated from it;WorkflowExecutionService/scalafmtCheck(main + Test) passes.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (claude-opus-4-8)