Copilot commented on code in PR #9430: URL: https://github.com/apache/gravitino/pull/9430#discussion_r2645197273
########## scripts/h2/upgrade-1.1.0-to-1.2.0-h2.sql: ########## @@ -0,0 +1,24 @@ +-- +-- 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. +-- +alter table `catalog_meta` add index catalog_meta_idx_name_da (catalog_name, deleted_at); +alter table `schema_meta` add index schema_meta_idx_name_da (catalog_name, deleted_at); +alter table `table_meta` add index table_meta_idx_name_da (table_name, deleted_at); +alter table `fileset_meta` add index fileset_meta_idx_name_da (fileset_name, deleted_at); +alter table `model_meta` add index model_meta_idx_name_da (model_name, deleted_at); +alter table `topic_meta` add index topic_meta_idx_name_da (topic_name, deleted_at); Review Comment: Missing index on `metalake_meta` table. The upgrade script adds indexes for catalog, schema, table, fileset, model, and topic metadata tables, but does not add an index on `metalake_meta(metalake_name, deleted_at)`. For consistency with the schema-1.2.0 files which include this index, the upgrade script should also create this index. ########## scripts/mysql/schema-1.2.0-mysql.sql: ########## @@ -0,0 +1,442 @@ +-- +-- 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. +-- Review Comment: Duplicate Apache license header found. Lines 1-18 contain the first license header, and lines 3-18 contain what appears to be a malformed duplicate (note the double dash on line 3). The second license header should be removed to avoid redundancy. ########## scripts/postgresql/schema-1.2.0-postgresql.sql: ########## @@ -0,0 +1,807 @@ +-- +-- 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. +-- +-- Base schema remains the same as 1.1.0. Apply this after the 1.1.0 schema +-- to add indexes introduced in version 1.2.0. + +-- +-- 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. Review Comment: Duplicate Apache license header found. Lines 1-18 contain the first license header, and lines 23-38 contain a duplicate. The second license header (lines 23-38) should be removed to avoid redundancy. ########## scripts/mysql/upgrade-1.1.0-to-1.2.0-mysql.sql: ########## @@ -0,0 +1,25 @@ +-- +-- 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. +-- + +alter table `catalog_meta` add index idx_name_da (catalog_name, deleted_at); +alter table `schema_meta` add index idx_name_da (schema_name, deleted_at); +alter table `table_meta` add index idx_name_da (table_name, deleted_at); +alter table `fileset_meta` add index idx_name_da (fileset_name, deleted_at); +alter table `model_meta` add index idx_name_da (model_name, deleted_at); +alter table `topic_meta` add index idx_name_da (topic_name, deleted_at); Review Comment: Missing index on `metalake_meta` table. The upgrade script adds indexes for catalog, schema, table, fileset, model, and topic metadata tables, but does not add an index on `metalake_meta(metalake_name, deleted_at)`. For consistency with the schema-1.2.0 files which include this index, the upgrade script should also create this index. ########## scripts/h2/upgrade-1.1.0-to-1.2.0-h2.sql: ########## @@ -0,0 +1,24 @@ +-- +-- 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. +-- +alter table `catalog_meta` add index catalog_meta_idx_name_da (catalog_name, deleted_at); +alter table `schema_meta` add index schema_meta_idx_name_da (catalog_name, deleted_at); Review Comment: The index on `schema_meta` table is using the wrong column name. It should be `schema_name` instead of `catalog_name`. This will create an index on the wrong column, which won't provide the intended performance benefit and may cause incorrect query optimization. ```suggestion alter table `schema_meta` add index schema_meta_idx_name_da (schema_name, deleted_at); ``` ########## core/src/test/java/org/apache/gravitino/storage/TestEntityStorage.java: ########## @@ -242,6 +252,7 @@ private void destroy(String type) { FileUtils.deleteQuietly(new File(H2_FILE)); } else if (type.equalsIgnoreCase("postgresql")) { // Do nothing + Review Comment: Extraneous empty line added. This line appears to be an unnecessary whitespace-only change that doesn't add value to the code. ```suggestion ``` ########## scripts/postgresql/upgrade-1.1.0-to-1.2.0-postgresql.sql: ########## @@ -0,0 +1,25 @@ +-- +-- 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. +-- + +CREATE INDEX IF NOT EXISTS catalog_meta_idx_name_da ON catalog_meta (catalog_name, deleted_at); +CREATE INDEX IF NOT EXISTS schema_meta_idx_name_da ON schema_meta (schema_name, deleted_at); +CREATE INDEX IF NOT EXISTS table_meta_idx_name_da ON table_meta (table_name, deleted_at); +CREATE INDEX IF NOT EXISTS fileset_meta_idx_name_da ON fileset_meta (fileset_name, deleted_at); +CREATE INDEX IF NOT EXISTS model_meta_idx_name_da ON model_meta (model_name, deleted_at); +CREATE INDEX IF NOT EXISTS topic_meta_idx_name_da ON topic_meta (topic_name, deleted_at); Review Comment: Missing index on `metalake_meta` table. The upgrade script adds indexes for catalog, schema, table, fileset, model, and topic metadata tables, but does not add an index on `metalake_meta(metalake_name, deleted_at)`. For consistency with the schema-1.2.0 files which include this index, the upgrade script should also create this index. ########## core/src/main/java/org/apache/gravitino/storage/relational/service/SchemaMetaService.java: ########## @@ -388,6 +395,40 @@ public int deleteSchemaMetasByLegacyTimeline(Long legacyTimeline, int limit) { }); } + private SchemaPO getSchemaPOByIdentifier(NameIdentifier identifier) { + NameIdentifierUtil.checkSchema(identifier); + if (GravitinoEnv.getInstance().config().get(Configs.CACHE_ENABLED)) { + Long catalogId = + EntityIdService.getEntityId( + NameIdentifier.of(identifier.namespace().levels()), Entity.EntityType.CATALOG); + return getSchemaPOByCatalogIdAndName(catalogId, identifier.name()); + } + + String[] namespaceLevels = identifier.namespace().levels(); + SchemaPO schemaPO = + SessionUtils.getWithoutCommit( + SchemaMetaMapper.class, + mapper -> + mapper.selectSchemaByFullQualifiedName( + namespaceLevels[0], namespaceLevels[1], identifier.name())); + + if (schemaPO == null) { + throw new NoSuchEntityException( + NoSuchEntityException.NO_SUCH_ENTITY_MESSAGE, + Entity.EntityType.CATALOG.name().toLowerCase(), + namespaceLevels[1]); Review Comment: Incorrect error message when schema is not found. When `schemaPO` is null (line 415), the error message reports that the CATALOG is not found (line 418), but it should report that the SCHEMA is not found. The entity type and name in the exception should reference the schema, not the catalog. ```suggestion Entity.EntityType.SCHEMA.name().toLowerCase(), identifier.name()); ``` ########## scripts/h2/schema-1.2.0-h2.sql: ########## @@ -0,0 +1,451 @@ +-- +-- 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. +-- Review Comment: Duplicate Apache license header found. Lines 1-18 contain the first license header, and lines 3-18 contain what appears to be a malformed duplicate (note the double dash on line 3). The second license header should be removed to avoid redundancy. -- 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]
