jerryshao commented on code in PR #14627:
URL: https://github.com/apache/gravitino/pull/14627#discussion_r4476441787
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/SemanticModelMetaService.java:
##########
@@ -416,11 +427,34 @@ void deleteSemanticModelWithVersion(
SessionUtils.doWithoutCommit(
SemanticModelMetricMapper.class,
mapper ->
mapper.softDeleteSemanticModelMetricsBySemanticModelId(semanticModelId)),
+ () ->
+ SessionUtils.doWithoutCommit(
+ SemanticModelSourceMapper.class,
+ mapper -> mapper.softDeleteByModelId(semanticModelId)),
() ->
EntityChangeLogWriter.append(
semanticModelIdent, Entity.EntityType.SEMANTIC_MODEL,
OperateType.DROP));
}
+ private void replaceSources(SemanticModelPO po, SemanticModelEntity entity) {
+ if (!entity.sourceUpdate().replacesSources()) {
Review Comment:
[Minor] `preserve()` skips association maintenance even when the new
definition's dataset names change. That matches #14378, but no test pins this
contract. Could you add one? Also, the follow-up write-path PR should set
`withSourceUpdate(replace(...))` on every create and `ReplaceDefinition`, since
the current `ManagedSemanticModelOperations` builders still default to
`preserve()` and would store no associations.
##########
scripts/mysql/schema-2.0.0-mysql.sql:
##########
@@ -768,3 +768,17 @@ CREATE TABLE IF NOT EXISTS `table_snapshot_metrics` (
KEY `idx_tsm_tid_pid_sid` (`table_id`, `policy_id`, `snapshot_id`),
KEY `idx_tsm_created_at` (`created_at`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin COMMENT 'table
snapshot metrics metadata';
+
+CREATE TABLE IF NOT EXISTS `semantic_model_source_rel` (
+ `id` BIGINT(20) UNSIGNED NOT NULL AUTO_INCREMENT,
Review Comment:
[Nit] Column lists and indexes match across the H2, MySQL, and PostgreSQL
schema and upgrade scripts. Please add the column comments from the #14378
MySQL DDL, plus the H2 backticks and PostgreSQL `COMMENT ON` statements used by
`semantic_model_metric_meta`, so the new table matches its neighbors.
##########
core/src/main/java/org/apache/gravitino/semantic/SemanticModelSource.java:
##########
@@ -0,0 +1,51 @@
+/*
+ * 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.
+ */
+package org.apache.gravitino.semantic;
+
+import com.google.common.base.Preconditions;
+import lombok.EqualsAndHashCode;
+import lombok.Getter;
+import org.apache.gravitino.Entity;
+
+/** A dataset's source resolved during caller-authorized definition
validation. */
+@Getter
+@EqualsAndHashCode
+public final class SemanticModelSource {
+ private final String datasetName;
+ private final Entity.EntityType sourceType;
+ private final long sourceId;
+
+ /**
+ * Creates a resolved source association.
+ *
+ * @param datasetName The exact dataset name in the definition.
+ * @param sourceType TABLE or VIEW.
+ * @param sourceId The registered Gravitino entity ID.
+ */
+ public SemanticModelSource(String datasetName, Entity.EntityType sourceType,
long sourceId) {
+ Preconditions.checkArgument(
+ datasetName != null && !datasetName.isEmpty(), "Dataset name is
required");
+ Preconditions.checkArgument(
+ sourceType == Entity.EntityType.TABLE || sourceType ==
Entity.EntityType.VIEW,
+ "Source must be a table or View");
Review Comment:
[Nit] Should this also reject `sourceId <= 0`? Also, nit: "table or View"
should be "table or VIEW" to match `EntityType`.
##########
core/src/main/java/org/apache/gravitino/semantic/SemanticModelSourceUpdate.java:
##########
@@ -0,0 +1,98 @@
+/*
+ * 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.
+ */
+package org.apache.gravitino.semantic;
+
+import com.google.common.base.Preconditions;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
+import lombok.EqualsAndHashCode;
+
+/** Internal write intent for source associations; never part of a public
model definition. */
+@EqualsAndHashCode
+public final class SemanticModelSourceUpdate {
+ private static final SemanticModelSourceUpdate PRESERVE =
+ new SemanticModelSourceUpdate(false, List.of());
+ private final boolean replace;
+ private final List<SemanticModelSource> sources;
+
+ private SemanticModelSourceUpdate(boolean replace, List<SemanticModelSource>
sources) {
+ this.replace = replace;
+ this.sources = List.copyOf(sources);
+ }
+
+ /**
+ * @return An instruction to preserve existing associations during
metadata-only updates.
+ */
+ public static SemanticModelSourceUpdate preserve() {
+ return PRESERVE;
+ }
+
+ /**
+ * Creates an instruction to replace the entire association set.
+ *
+ * @param sources All resolved datasets of the new definition.
+ * @return The immutable replacement instruction.
+ */
+ public static SemanticModelSourceUpdate replace(List<SemanticModelSource>
sources) {
+ Set<String> names = new HashSet<>();
+ sources.forEach(
Review Comment:
[Nit] `replace(null)` throws an NPE here. Consider
`Preconditions.checkNotNull(sources, ...)` before iterating, so callers get a
clear error.
##########
core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/SemanticModelSourceBaseSQLProvider.java:
##########
@@ -0,0 +1,83 @@
+/*
+ * 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.
+ */
+package org.apache.gravitino.storage.relational.mapper.provider.base;
+
+import static
org.apache.gravitino.storage.relational.mapper.SemanticModelSourceMapper.TABLE_NAME;
+
+import java.util.List;
+import org.apache.gravitino.storage.relational.mapper.SemanticModelMetaMapper;
+import org.apache.ibatis.annotations.Param;
+
+/** Provides MySQL-compatible SQL for retiring dataset source associations. */
+public class SemanticModelSourceBaseSQLProvider {
+
+ /** Returns SQL for retiring a model's live associations. */
+ public String softDeleteByModelId(@Param("modelId") Long modelId) {
+ return softDeleteBy("semantic_model_id = #{modelId}");
+ }
+
+ /** Returns SQL for retiring associations owned by models in the given
schemas. */
+ public String softDeleteBySchemaIds(@Param("schemaIds") List<Long>
schemaIds) {
+ return "<script>"
+ + softDeleteBy(
+ "semantic_model_id IN (SELECT semantic_model_id FROM "
+ + SemanticModelMetaMapper.TABLE_NAME
+ + " WHERE schema_id IN ("
+ + "<foreach collection='schemaIds' item='schemaId'
separator=','>"
+ + "#{schemaId}</foreach>))")
+ + "</script>";
+ }
+
+ /** Returns SQL for retiring associations owned by models in a catalog. */
+ public String softDeleteByCatalogId(@Param("catalogId") Long catalogId) {
+ return softDeleteBy(
+ "semantic_model_id IN (SELECT semantic_model_id FROM "
+ + SemanticModelMetaMapper.TABLE_NAME
+ + " WHERE catalog_id = #{catalogId})");
+ }
+
+ /** Returns SQL for retiring associations owned by models in a metalake. */
+ public String softDeleteByMetalakeId(@Param("metalakeId") Long metalakeId) {
+ return softDeleteBy("metalake_id = #{metalakeId}");
+ }
+
+ /** Returns SQL for purging retired rows without depending on model or
parent metadata. */
+ public String deleteByLegacyTimeline(
+ @Param("legacyTimeline") Long legacyTimeline, @Param("limit") int limit)
{
+ return "DELETE FROM "
+ + TABLE_NAME
+ + " WHERE deleted_at > 0 AND deleted_at < #{legacyTimeline} LIMIT
#{limit}";
Review Comment:
[Minor] `deleteByLegacyTimeline` filters only on `deleted_at`, but both
indexes lead with `semantic_model_id` or `metalake_id`, so each GC batch may
scan the table. `LIMIT` still caps the rows removed, and this matches the
#14378 DDL and `semantic_model_metric_meta`. Consider adding
`idx_smsr_deleted_at (deleted_at)` to the H2, MySQL, and PostgreSQL schema and
upgrade scripts if the purge should be index-bounded.
--
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]
