[GitHub] [ignite-3] korlov42 commented on a diff in pull request #1296: IGNITE-17986 SchemaManager recovery was broken after refactoring

[GitBox <gi...@apache.org>]

korlov42 commented on code in PR #1296:
URL: https://github.com/apache/ignite-3/pull/1296#discussion_r1017561122


##########
modules/index/src/main/java/org/apache/ignite/internal/index/IndexManager.java:
##########
@@ -486,6 +486,8 @@ public BinaryTuple convert(BinaryRow tableRow) {
 
         /** Creates converter for given version of the schema. */
         private VersionedConverter createConverter(int schemaVersion) {
+            assert registry != null : "Registry is not initialized for schema=" + schemaVersion;

Review Comment:
   the message is not quite correct. The registry contains different versions, not the vice versa



##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/ItIgniteNodeRestartTest.java:
##########
@@ -681,7 +681,7 @@ public void twoCustomPropertiesTest() {
      * Restarts the node which stores some data.
      */
     @Test
-    @Disabled("https://issues.apache.org/jira/browse/IGNITE-17986")
+

Review Comment:
   blank line



##########
modules/schema/src/main/java/org/apache/ignite/internal/schema/SchemaUtils.java:
##########
@@ -45,38 +45,53 @@ public static SchemaDescriptor prepareSchemaDescriptor(int schemaVer, TableView
      * Prepares column mapper.
      *
      * @param oldDesc Old schema descriptor.
-     * @param oldTblColumns Old columns configuration.
      * @param newDesc New schema descriptor.
-     * @param newTblColumns New columns configuration.
      * @return Column mapper.
      */
     public static ColumnMapper columnMapper(
             SchemaDescriptor oldDesc,
-            NamedListView<? extends ColumnView> oldTblColumns,
-            SchemaDescriptor newDesc,
-            NamedListView<? extends ColumnView> newTblColumns
+            SchemaDescriptor newDesc
     ) {
-        ColumnMapper mapper = null;
+        // Rename not supported from standard, thus only deletion.
+        if (newDesc.keyColumns().length() != oldDesc.keyColumns().length()) {
+            Optional<Column> droppedKeyCol = Arrays.stream(oldDesc.keyColumns().columns())
+                    .filter(c -> !newDesc.column(c.schemaIndex()).name().equals(c.name()))
+                    .findAny();
+

Review Comment:
   Why do we even bother about table-related constraints during schema mapping creation? Let's remove this validation



##########
modules/schema/src/main/java/org/apache/ignite/internal/schema/SchemaManager.java:
##########
@@ -477,25 +453,25 @@ private int latestSchemaVersion(UUID tblId) {
     }
 
     /**
-     * Collect all schemes for appropriate table.
+     * Gets the defined version of the table schema which available in Metastore.
      *
      * @param tblId Table id.
-     * @return Sorted by key collection of schemes.
+     * @return Schema representation if schema found, {@code null} otherwise.
      */
-    private SortedMap<Integer, byte[]> collectAllSchemas(UUID tblId) {
+    @Nullable private byte[] schemaByVersion(UUID tblId, int ver) {

Review Comment:
   why don't look up _exact_ schema in ms?
   
   Besides, `@Nullable` in that particular place doesn't make any sense



-- 
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: notifications-unsubscribe@ignite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org