feat(amber): decouple the Lakekeeper catalog name from the user-facing name - #7754
Open
mengw15 wants to merge 1 commit into
Open
feat(amber): decouple the Lakekeeper catalog name from the user-facing name#7754mengw15 wants to merge 1 commit into
mengw15 wants to merge 1 commit 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.
Comment on lines
+21
to
+25
| \c texera_db | ||
|
|
||
| SET search_path TO texera_db; | ||
|
|
||
| BEGIN; |
Comment on lines
+32
to
+35
| ALTER TABLE user_warehouse | ||
| RENAME COLUMN warehouse_name TO lakekeeper_warehouse_name; | ||
|
|
||
| COMMIT; |
Comment on lines
+142
to
+145
| val whid: Integer = context.fetchValue( | ||
| DSL.field( | ||
| "nextval(pg_get_serial_sequence('texera_db.user_warehouse','whid'))", | ||
| classOf[Integer] |
Comment on lines
80
to
+81
| row.getName shouldBe "mybucket" | ||
| row.getWarehouseName shouldBe s"user-$uid-mybucket" | ||
| row.getLakekeeperWarehouseName shouldBe s"user-$uid-mybucket" |
Comment on lines
+186
to
+193
| val takenWhid = getDSLContext.fetchValue( | ||
| DSL.field( | ||
| "nextval(pg_get_serial_sequence('texera_db.user_warehouse','whid'))", | ||
| classOf[Integer] | ||
| ) | ||
| ) | ||
| squatter.setWhid(takenWhid) | ||
| squatter.setLakekeeperWarehouseName(s"user-${sessionUser.getUid}-${takenWhid + 1}") |
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.02% -0.01%
Complexity 4454 4454
============================================
Files 1174 1174
Lines 47146 47150 +4
Branches 5287 5287
============================================
+ Hits 42916 42918 +2
- Misses 2550 2552 +2
Partials 1680 1680
*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 | 647 | 0.395 | 15,434/19,294/19,294 us | 🔴 +26.0% / 🔴 +20.7% |
| 🔴 | bs=100 sw=10 sl=64 | 1,276 | 0.779 | 74,348/124,123/124,123 us | 🟢 +41.4% / 🟢 -26.4% |
| 🟢 | bs=1000 sw=10 sl=64 | 1,543 | 0.942 | 649,509/694,362/694,362 us | 🟢 +67.0% / 🟢 +47.2% |
Baseline details
Latest main e80add4 from 2026-08-17T13:30:51.393Z
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 647 tuples/sec | 722.95 tuples/sec | 779.07 tuples/sec | -10.5% | -17.0% |
| bs=10 sw=10 sl=64 | MB/s | 0.395 MB/s | 0.441 MB/s | 0.476 MB/s | -10.5% | -16.9% |
| bs=10 sw=10 sl=64 | p50 | 15,434 us | 13,608 us | 12,818 us | +13.4% | +20.4% |
| bs=10 sw=10 sl=64 | p95 | 19,294 us | 15,308 us | 15,986 us | +26.0% | +20.7% |
| bs=10 sw=10 sl=64 | p99 | 19,294 us | 20,362 us | 19,339 us | -5.2% | -0.2% |
| bs=100 sw=10 sl=64 | throughput | 1,276 tuples/sec | 902.81 tuples/sec | 1,011 tuples/sec | +41.3% | +26.2% |
| bs=100 sw=10 sl=64 | MB/s | 0.779 MB/s | 0.551 MB/s | 0.617 MB/s | +41.4% | +26.2% |
| bs=100 sw=10 sl=64 | p50 | 74,348 us | 109,799 us | 100,965 us | -32.3% | -26.4% |
| bs=100 sw=10 sl=64 | p95 | 124,123 us | 116,633 us | 107,295 us | +6.4% | +15.7% |
| bs=100 sw=10 sl=64 | p99 | 124,123 us | 128,023 us | 115,531 us | -3.0% | +7.4% |
| bs=1000 sw=10 sl=64 | throughput | 1,543 tuples/sec | 924.33 tuples/sec | 1,049 tuples/sec | +66.9% | +47.1% |
| bs=1000 sw=10 sl=64 | MB/s | 0.942 MB/s | 0.564 MB/s | 0.64 MB/s | +67.0% | +47.2% |
| bs=1000 sw=10 sl=64 | p50 | 649,509 us | 1,080,065 us | 978,248 us | -39.9% | -33.6% |
| bs=1000 sw=10 sl=64 | p95 | 694,362 us | 1,121,514 us | 1,021,881 us | -38.1% | -32.1% |
| bs=1000 sw=10 sl=64 | p99 | 694,362 us | 1,140,846 us | 1,050,075 us | -39.1% | -33.9% |
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,308.89,200,128000,647,0.395,15434.05,19294.06,19294.06
1,100,10,64,20,1567.79,2000,1280000,1276,0.779,74348.05,124123.37,124123.37
2,1000,10,64,20,12960.65,20000,12800000,1543,0.942,649508.78,694361.79,694361.79
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. The wire DTO keepswarehouseName.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.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+ExecutionsMetadataPersistServiceSpecrun locally: 26/26 passed 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)