PakhomovAlexander commented on code in PR #6648:
URL: https://github.com/apache/ignite-3/pull/6648#discussion_r2377103004
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogTableDescriptor.java:
##########
@@ -143,35 +107,35 @@ public CatalogTableDescriptor(
this.colocationColumns = Objects.requireNonNullElse(colocationCols,
pkCols);
this.schemaVersions = Objects.requireNonNull(schemaVersions, "No
catalog schema versions.");
this.storageProfile = Objects.requireNonNull(storageProfile, "No
storage profile.");
+
+ TableVersion latestTableVersion =
Objects.requireNonNull(schemaVersions.get(schemaVersions.latestVersion()));
+ if (!Objects.equals(latestTableVersion.columns(), columns)) {
+ throw new IllegalArgumentException("Latest schema version columns
do not match descriptor definition columns.");
Review Comment:
Add the context to the exception message like: ".....,
latestTableVersion.columns = [...], schema columns=[...]"
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogTableDescriptor.java:
##########
@@ -282,4 +246,202 @@ public String storageProfile() {
return storageProfile;
}
+ /**
+ * {@code CatalogTableDescriptor} builder static inner class.
+ */
+ public static final class Builder {
+ private int id;
+ private String name;
+ private int zoneId;
+ private int schemaId;
+ private int pkIndexId;
+ private CatalogTableSchemaVersions schemaVersions;
+ private List<CatalogTableColumnDescriptor> columns;
+ private List<String> primaryKeyColumns;
+ @Nullable private List<String> colocationColumns;
+ private String storageProfile;
+ private HybridTimestamp timestamp = INITIAL_TIMESTAMP;
+ private int tableVersion = 0;
+
+ private Builder() {}
+
+ public static Builder builder() {
Review Comment:
you already have `builder()` method in the `CatalogTableDescriptor`, I think
it is better to have the only method rather than two.
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/DefaultValue.java:
##########
@@ -166,7 +166,7 @@ public boolean equals(Object o) {
ConstantValue that = (ConstantValue) o;
- return Objects.equals(value, that.value);
+ return Objects.deepEquals(value, that.value);
Review Comment:
what's the reason for `deepEquals` here? As far as Im concerned,
`deepEquals` makes sense for arrays but here we deal with `DefaultValue`. Do I
miss something?
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/storage/AlterColumnEntry.java:
##########
@@ -79,20 +82,31 @@ public CatalogEventParameters createEventParameters(long
causalityToken, int cat
return new AlterColumnEventParameters(causalityToken, catalogVersion,
tableId, column);
}
+ @Override
+ public Builder newTableDescriptor(CatalogTableDescriptor table,
HybridTimestamp timestamp) {
+ List<CatalogTableColumnDescriptor> updatedTableColumns =
table.columns().stream()
+ .map(source -> source.name().equals(column.name()) ? column :
source)
+ .collect(toList());
+
+ return table.copyBuilder()
+ .columns(updatedTableColumns)
+ .timestamp(timestamp);
+ }
+
@Override
public Catalog applyUpdate(Catalog catalog, HybridTimestamp timestamp) {
CatalogTableDescriptor table = tableOrThrow(catalog, tableId);
CatalogSchemaDescriptor schema = schemaOrThrow(catalog,
table.schemaId());
- CatalogTableDescriptor newTable = table.newDescriptor(
- table.name(),
- table.tableVersion() + 1,
- table.columns().stream()
- .map(source -> source.name().equals(column.name()) ?
column : source)
- .collect(toList()),
- timestamp,
- table.storageProfile()
- );
+ List<CatalogTableColumnDescriptor> updatedTableColumns =
table.columns().stream()
Review Comment:
`newTableDescriptor` might be used here
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/storage/AlterColumnEntry.java:
##########
@@ -79,20 +82,31 @@ public CatalogEventParameters createEventParameters(long
causalityToken, int cat
return new AlterColumnEventParameters(causalityToken, catalogVersion,
tableId, column);
}
+ @Override
+ public Builder newTableDescriptor(CatalogTableDescriptor table,
HybridTimestamp timestamp) {
Review Comment:
javadoc would be useful here
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/storage/UpdateTable.java:
##########
@@ -0,0 +1,65 @@
+/*
+ * 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.ignite.internal.catalog.storage;
+
+import static
org.apache.ignite.internal.catalog.commands.CatalogUtils.defaultZoneIdOpt;
+import static
org.apache.ignite.internal.catalog.commands.CatalogUtils.replaceSchema;
+import static
org.apache.ignite.internal.catalog.commands.CatalogUtils.replaceTable;
+import static
org.apache.ignite.internal.catalog.commands.CatalogUtils.schemaOrThrow;
+import static
org.apache.ignite.internal.catalog.commands.CatalogUtils.tableOrThrow;
+
+import org.apache.ignite.internal.catalog.Catalog;
+import org.apache.ignite.internal.catalog.descriptors.CatalogSchemaDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogTableDescriptor;
+import org.apache.ignite.internal.hlc.HybridTimestamp;
+
+/** Interface describing a particular change to a table within the {@link
VersionedUpdate group}. */
+interface UpdateTable extends UpdateEntry {
+ @Override
+ default Catalog applyUpdate(Catalog catalog, HybridTimestamp timestamp) {
+ CatalogTableDescriptor table = tableOrThrow(catalog, tableId());
+ CatalogSchemaDescriptor schema = schemaOrThrow(catalog,
table.schemaId());
+
+ CatalogTableDescriptor modifiedTable = newTableDescriptor(table,
timestamp)
+ .tableVersion(newTableVersion(table))
+ .timestamp(timestamp)
Review Comment:
you already set a timestamp into a builder in `newTableDescriptor`. If this
is not guaranteed -- why does this exists in the method signature?
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogTableDescriptor.java:
##########
@@ -282,4 +246,202 @@ public String storageProfile() {
return storageProfile;
}
+ /**
+ * {@code CatalogTableDescriptor} builder static inner class.
+ */
+ public static final class Builder {
+ private int id;
+ private String name;
+ private int zoneId;
+ private int schemaId;
+ private int pkIndexId;
+ private CatalogTableSchemaVersions schemaVersions;
+ private List<CatalogTableColumnDescriptor> columns;
+ private List<String> primaryKeyColumns;
+ @Nullable private List<String> colocationColumns;
+ private String storageProfile;
+ private HybridTimestamp timestamp = INITIAL_TIMESTAMP;
+ private int tableVersion = 0;
+
+ private Builder() {}
+
+ public static Builder builder() {
+ return new Builder();
+ }
+
+ /**
+ * Sets the {@code id} and returns a reference to this Builder
enabling method chaining.
+ *
+ * @param id the {@code id} to set
+ * @return a reference to this Builder
+ */
+ public Builder id(int id) {
+ this.id = id;
+ return this;
+ }
+
+ /**
+ * Sets the {@code name} and returns a reference to this Builder
enabling method chaining.
+ *
+ * @param name the {@code name} to set
+ * @return a reference to this Builder
+ */
+ public Builder name(String name) {
+ this.name = name;
+ return this;
+ }
+
+ /**
+ * Sets the {@code timestamp} and returns a reference to this Builder
enabling method chaining.
+ *
+ * @param timestamp the {@code timestamp} to set
+ * @return a reference to this Builder
+ */
+ public Builder timestamp(HybridTimestamp timestamp) {
+ this.timestamp = timestamp;
+ return this;
+ }
+
+ /**
+ * Sets the {@code zoneId} and returns a reference to this Builder
enabling method chaining.
+ *
+ * @param zoneId the {@code zoneId} to set
+ * @return a reference to this Builder
+ */
+ public Builder zoneId(int zoneId) {
+ this.zoneId = zoneId;
+ return this;
+ }
+
+ /**
+ * Sets the {@code schemaId} and returns a reference to this Builder
enabling method chaining.
+ *
+ * @param schemaId the {@code schemaId} to set
+ * @return a reference to this Builder
+ */
+ public Builder schemaId(int schemaId) {
+ this.schemaId = schemaId;
+ return this;
+ }
+
+ /**
+ * Sets the {@code primaryKeyIndexId} and returns a reference to this
Builder enabling method chaining.
+ *
+ * @param primaryKeyIndexId the {@code primaryKeyIndexId} to set
+ * @return a reference to this Builder
+ */
+ public Builder primaryKeyIndexId(int primaryKeyIndexId) {
+ this.pkIndexId = primaryKeyIndexId;
+ return this;
+ }
+
+ /**
+ * Sets the {@code schemaVersions} and returns a reference to this
Builder enabling method chaining.
+ *
+ * @param schemaVersions the {@code schemaVersions} to set
+ * @return a reference to this Builder
+ */
+ public Builder schemaVersions(CatalogTableSchemaVersions
schemaVersions) {
+ this.schemaVersions = schemaVersions;
+ return this;
+ }
+
+ /**
+ * Sets the {@code columns} and returns a reference to this Builder
enabling method chaining.
+ *
+ * @param columns the {@code columns} to set
+ * @return a reference to this Builder
+ */
+ public Builder columns(List<CatalogTableColumnDescriptor> columns) {
+ this.columns = columns;
+ return this;
+ }
+
+ /**
+ * Sets the {@code primaryKeyColumns} and returns a reference to this
Builder enabling method chaining.
+ *
+ * @param primaryKeyColumns the {@code primaryKeyColumns} to set
+ * @return a reference to this Builder
+ */
+ public Builder primaryKeyColumns(List<String> primaryKeyColumns) {
+ this.primaryKeyColumns = primaryKeyColumns;
+ return this;
+ }
+
+ /**
+ * Sets the {@code colocationColumns} and returns a reference to this
Builder enabling method chaining.
+ *
+ * @param colocationColumns the {@code colocationColumns} to set
+ * @return a reference to this Builder
+ */
+ public Builder colocationColumns(@Nullable List<String>
colocationColumns) {
+ this.colocationColumns = colocationColumns;
+ return this;
+ }
+
+ /**
+ * Sets the {@code storageProfile} and returns a reference to this
Builder enabling method chaining.
+ *
+ * @param storageProfile the {@code storageProfile} to set
+ * @return a reference to this Builder
+ */
+ public Builder storageProfile(String storageProfile) {
+ this.storageProfile = storageProfile;
+ return this;
+ }
+
+ /**
+ * Sets the {@code tableVersion} and returns a reference to this
Builder enabling method chaining.
+ *
+ * @param tableVersion the {@code tableVersion} to set.
+ * @return a reference to this Builder.
+ */
+ public Builder tableVersion(int tableVersion) {
+ this.tableVersion = tableVersion;
+ return this;
+ }
+
+ /**
+ * Returns a {@code CatalogTableDescriptor} built from the parameters
previously set.
+ *
+ * @return a {@code CatalogTableDescriptor} built with parameters of
this {@code CatalogTableDescriptor.Builder}
+ */
+ public CatalogTableDescriptor build() {
+ Objects.requireNonNull(columns, "No columns defined.");
Review Comment:
columns must not be empty I suppose
--
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]
