mengw15 commented on code in PR #7754: URL: https://github.com/apache/texera/pull/7754#discussion_r3807539709
########## sql/updates/38.sql: ########## @@ -0,0 +1,35 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + + +\c texera_db + +SET search_path TO texera_db; + +BEGIN; + +-- The Lakekeeper catalog name is no longer derived from the user-facing name +-- (#7753), and the table already has its own `name` column -- so the old +-- `warehouse_name` was ambiguous. Rename it to sit beside its sibling +-- `lakekeeper_warehouse_id`. The table is empty in every deployment (the +-- warehouse feature flag is off everywhere), so this carries no data. +ALTER TABLE user_warehouse + RENAME COLUMN warehouse_name TO lakekeeper_warehouse_name; Review Comment: Good catch — fixed in `ecb5aff`. `texera_ddl.sql` already creates the column under its new name, so on a fresh local-dev database the unguarded rename hit a column that was never there and aborted `up`: the replay tolerates only "already exists", and PostgreSQL reports "column warehouse_name does not exist". Guarded with the same existence check `33.sql` uses for its rename, and verified against a live database both ways — a schema already holding `lakekeeper_warehouse_name` is a clean no-op, one still holding `warehouse_name` is renamed. ########## sql/updates/38.sql: ########## @@ -0,0 +1,35 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + + +\c texera_db + +SET search_path TO texera_db; + +BEGIN; Review Comment: These migrations are applied with `psql`, not over JDBC — `bin/local-dev/main.sh` runs them with `ON_ERROR_STOP=1`, where `\c` is valid. Every existing migration (35/36/37.sql) opens with the same `\c texera_db` and wraps its body in `BEGIN;`/`COMMIT;`, so dropping them here would make 38.sql the only file shaped differently without changing how it executes. ########## sql/updates/38.sql: ########## @@ -0,0 +1,35 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + + +\c texera_db + +SET search_path TO texera_db; + +BEGIN; + +-- The Lakekeeper catalog name is no longer derived from the user-facing name +-- (#7753), and the table already has its own `name` column -- so the old +-- `warehouse_name` was ambiguous. Rename it to sit beside its sibling +-- `lakekeeper_warehouse_id`. The table is empty in every deployment (the +-- warehouse feature flag is off everywhere), so this carries no data. +ALTER TABLE user_warehouse + RENAME COLUMN warehouse_name TO lakekeeper_warehouse_name; + +COMMIT; Review Comment: Same as above: these files run through `psql` (see `bin/local-dev/main.sh`), and 35/36/37.sql all carry the identical `\c` + `BEGIN;`/`COMMIT;` preamble. ########## amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/warehouse/WarehouseResource.scala: ########## @@ -131,20 +132,34 @@ class WarehouseResource(client: LakekeeperClient, enabled: Boolean) extends Lazy throw new WebApplicationException(s"a warehouse named '$name' already exists", 409) } - val warehouseName = s"user-$uid-$name" + // The catalog name is derived from the row's own id, never from `name`: that one + // string is also the REST catalog prefix, the S3 key prefix and a component of every + // result URI an execution wrote, so deriving it from a user-facing name would freeze + // that name forever (#7753). Take the id from the sequence up front so the creation + // order below is unchanged -- Lakekeeper first, row after, with the compensating + // delete. The sequence is resolved from the catalog rather than named literally, + // because its generated name is not a stable contract. + val whid: Integer = context.fetchValue( + DSL.field( + "nextval(pg_get_serial_sequence('texera_db.user_warehouse','whid'))", + classOf[Integer] Review Comment: Fixed in `ecb5aff` — the qualified table and column names now come from the jOOQ metadata (`USER_WAREHOUSE.getSchema.getName` / `.getName` / `WHID.getName`) rather than a literal `texera_db.user_warehouse`, so the lookup follows whatever schema the generated code targets. ########## common/dao/src/test/scala/org/apache/texera/dao/UserWarehouseSpec.scala: ########## @@ -78,7 +78,7 @@ class UserWarehouseSpec extends AnyFlatSpec with Matchers with BeforeAndAfterAll .where(USER_WAREHOUSE.UID.eq(uid)) .fetchOne() row.getName shouldBe "mybucket" - row.getWarehouseName shouldBe s"user-$uid-mybucket" + row.getLakekeeperWarehouseName shouldBe s"user-$uid-mybucket" Review Comment: This spec covers the DAO layer: it inserts its own rows and asserts they round-trip, so `user-<uid>-mybucket` is just an arbitrary globally-unique catalog name rather than an assertion about how the name is minted — it never calls `WarehouseResource.create`, so it cannot pin or regress that rule. The mint rule is pinned in `WarehouseResourceSpec`, which asserts the name equals `user-<uid>-<whid>` and does not contain the display name. I did rename the fixture parameter and add a note in `ecb5aff`, since the old value read like a naming rule. ########## amber/src/test/scala/org/apache/texera/web/resource/dashboard/user/warehouse/WarehouseResourceSpec.scala: ########## @@ -164,7 +180,17 @@ class WarehouseResourceSpec val squatter = getDSLContext.newRecord(USER_WAREHOUSE) squatter.setUid(otherUser.getUid) squatter.setName("unrelated") - squatter.setWarehouseName(s"user-${sessionUser.getUid}-boom") + // The catalog name now comes from the sequence, so claim the id the next create + // will draw: take one number for the squatter itself (set explicitly, so storing it + // consumes nothing further) and squat on the one after it. + 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}") Review Comment: The sequence cannot be advanced by anything else here: each suite runs against its own database (`MockTexeraDB` creates `texera_db_<uuid>` per suite) and `build.sbt:175` sets `Test / parallelExecution := false`, so no other suite runs concurrently and the two statements are consecutive within the suite. An advisory lock would guard a race the harness already rules out. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
