Repository: apache/iceberg (language: python)

Issue:
Core: Update metadata location without updating lastUpdatedMillis
Using `TableMetadata.buildFrom(metadata).withMetadataLocation(metadataLocation).build()` would do the following things:
- update `lastUpdatedMillis`
- write a new `metadata-log` entry

However, in `LoadTableResponse` the goal is to only update the metadata location without updating the two mentioned things above.


Discussion / hints:

Fix patch:
diff --git a/core/src/main/java/org/apache/iceberg/rest/RESTSerializers.java b/core/src/main/java/org/apache/iceberg/rest/RESTSerializers.java
index 341dda0e3f6c..7f39d0bc1f5e 100644
--- a/core/src/main/java/org/apache/iceberg/rest/RESTSerializers.java
+++ b/core/src/main/java/org/apache/iceberg/rest/RESTSerializers.java
@@ -60,6 +60,8 @@
 import org.apache.iceberg.rest.responses.ErrorResponse;
 import org.apache.iceberg.rest.responses.ErrorResponseParser;
 import org.apache.iceberg.rest.responses.ImmutableLoadViewResponse;
+import org.apache.iceberg.rest.responses.LoadTableResponse;
+import org.apache.iceberg.rest.responses.LoadTableResponseParser;
 import org.apache.iceberg.rest.responses.LoadViewResponse;
 import org.apache.iceberg.rest.responses.LoadViewResponseParser;
 import org.apache.iceberg.rest.responses.OAuthTokenResponse;
@@ -115,7 +117,9 @@ public static void registerAll(ObjectMapper mapper) {
         .addDeserializer(LoadViewResponse.class, new LoadViewResponseDeserializer<>())
         .addDeserializer(ImmutableLoadViewResponse.class, new LoadViewResponseDeserializer<>())
         .addSerializer(ConfigResponse.class, new ConfigResponseSerializer<>())
-        .addDeserializer(ConfigResponse.class, new ConfigResponseDeserializer<>());
+        .addDeserializer(ConfigResponse.class, new ConfigResponseDeserializer<>())
+        .addSerializer(LoadTableResponse.class, new LoadTableResponseSerializer<>())
+        .addDeserializer(LoadTableResponse.class, new LoadTableResponseDeserializer<>());
 
     mapper.registerModule(module);
   }
@@ -422,4 +426,21 @@ public T deserialize(JsonParser p, DeserializationContext context) throws IOExce
       return (T) ConfigResponseParser.fromJson(jsonNode);
     }
   }
+
+  static class LoadTableResponseSerializer<T extends LoadTableResponse> extends JsonSerializer<T> {
+    @Override
+    public void serialize(T request, JsonGenerator gen, SerializerProvider serializers)
+        throws IOException {
+      LoadTableResponseParser.toJson(request, gen);
+    }
+  }
+
+  static class LoadTableResponseDeserializer<T extends LoadTableResponse>
+      extends JsonDeserializer<T> {
+    @Override
+    public T deserialize(JsonParser p, DeserializationContext context) throws IOException {
+      JsonNode jsonNode = p.getCodec().readTree(p);
+      return (T) LoadTableResponseParser.fromJson(jsonNode);
+    }
+  }
 }
diff --git a/core/src/main/java/org/apache/iceberg/rest/responses/LoadTableResponse.java b/core/src/main/java/org/apache/iceberg/rest/responses/LoadTableResponse.java
index 5e5353c3909a..519d1fc34044 100644
--- a/core/src/main/java/org/apache/iceberg/rest/responses/LoadTableResponse.java
+++ b/core/src/main/java/org/apache/iceberg/rest/responses/LoadTableResponse.java
@@ -39,6 +39,7 @@ public class LoadTableResponse implements RESTResponse {
   private String metadataLocation;
   private TableMetadata metadata;
   private Map<String, String> config;
+  private TableMetadata metadataWithLocation;
 
   public LoadTableResponse() {
     // Required for Jackson deserialization
@@ -61,7 +62,12 @@ public String metadataLocation() {
   }
 
   public TableMetadata tableMetadata() {
-    return TableMetadata.buildFrom(metadata).withMetadataLocation(metadataLocation).build();
+    if (null == metadataWithLocation) {
+      this.metadataWithLocation =
+          TableMetadata.buildFrom(metadata).withMetadataLocation(metadataLocation).build();
+    }
+
+    return metadataWithLocation;
   }
 
   public Map<String, String> config() {
diff --git a/core/src/main/java/org/apache/iceberg/rest/responses/LoadTableResponseParser.java b/core/src/main/java/org/apache/iceberg/rest/responses/LoadTableResponseParser.java
new file mode 100644
index 000000000000..316c5160ddc5
--- /dev/null
+++ b/core/src/main/java/org/apache/iceberg/rest/responses/LoadTableResponseParser.java
@@ -0,0 +1,90 @@
+/*
+ * 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.iceberg.rest.responses;
+
+import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.databind.JsonNode;
+import java.io.IOException;
+import org.apache.iceberg.TableMetadata;
+import org.apache.iceberg.TableMetadataParser;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.util.JsonUtil;
+
+public class LoadTableResponseParser {
+
+  private static final String METADATA_LOCATION = "metadata-location";
+  private static final String METADATA = "metadata";
+  private static final String CONFIG = "config";
+
+  private LoadTableResponseParser() {}
+
+  public static String toJson(LoadTableResponse response) {
+    return toJson(response, false);
+  }
+
+  public static String toJson(LoadTableResponse response, boolean pretty) {
+    return JsonUtil.generate(gen -> toJson(response, gen), pretty);
+  }
+
+  public static void toJson(LoadTableResponse response, JsonGenerator gen) throws IOException {
+    Preconditions.checkArgument(null != response, "Invalid load table response: null");
+
+    gen.writeStartObject();
+
+    if (null != response.metadataLocation()) {
+      gen.writeStringField(METADATA_LOCATION, response.metadataLocation());
+    }
+
+    gen.writeFieldName(METADATA);
+    TableMetadataParser.toJson(response.tableMetadata(), gen);
+
+    if (!response.config().isEmpty()) {
+      JsonUtil.writeStringMap(CONFIG, response.config(), gen);
+    }
+
+    gen.writeEndObject();
+  }
+
+  public static LoadTableResponse fromJson(String json) {
+    return JsonUtil.parse(json, LoadTableResponseParser::fromJson);
+  }
+
+  public static LoadTableResponse fromJson(JsonNode json) {
+    Preconditions.checkArgument(null != json, "Cannot parse load table response from null object");
+
+    String metadataLocation = null;
+    if (json.hasNonNull(METADATA_LOCATION)) {
+      metadataLocation = JsonUtil.getString(METADATA_LOCATION, json);
+    }
+
+    TableMetadata metadata = TableMetadataParser.fromJson(JsonUtil.get(METADATA, json));
+
+    if (null != metadataLocation) {
+      metadata = TableMetadata.buildFrom(metadata).withMetadataLocation(metadataLocation).build();
+    }
+
+    LoadTableResponse.Builder builder = LoadTableResponse.builder().withTableMetadata(metadata);
+
+    if (json.hasNonNull(CONFIG)) {
+      builder.addAllConfig(JsonUtil.getStringMap(CONFIG, json));
+    }
+
+    return builder.build();
+  }
+}


Test patch:
diff --git a/core/src/test/java/org/apache/iceberg/rest/responses/TestLoadTableResponseParser.java b/core/src/test/java/org/apache/iceberg/rest/responses/TestLoadTableResponseParser.java
new file mode 100644
index 000000000000..b87c66bffe94
--- /dev/null
+++ b/core/src/test/java/org/apache/iceberg/rest/responses/TestLoadTableResponseParser.java
@@ -0,0 +1,203 @@
+/*
+ * 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.iceberg.rest.responses;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.SortOrder;
+import org.apache.iceberg.TableMetadata;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.Test;
+
+public class TestLoadTableResponseParser {
+
+  @Test
+  public void nullAndEmptyCheck() {
+    assertThatThrownBy(() -> LoadTableResponseParser.toJson(null))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage("Invalid load table response: null");
+
+    assertThatThrownBy(() -> LoadTableResponseParser.fromJson((JsonNode) null))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage("Cannot parse load table response from null object");
+
+    assertThatThrownBy(() -> LoadTableResponseParser.fromJson("{}"))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage("Cannot parse missing field: metadata");
+  }
+
+  @Test
+  public void missingFields() {
+    assertThatThrownBy(
+            () -> LoadTableResponseParser.fromJson("{\"metadata-location\": \"custom-location\"}"))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage("Cannot parse missing field: metadata");
+  }
+
+  @Test
+  public void roundTripSerde() {
+    String uuid = "386b9f01-002b-4d8c-b77f-42c3fd3b7c9b";
+    TableMetadata metadata =
+        TableMetadata.buildFromEmpty()
+            .assignUUID(uuid)
+            .setLocation("location")
+            .setCurrentSchema(
+                new Schema(Types.NestedField.required(1, "x", Types.LongType.get())), 1)
+            .addPartitionSpec(PartitionSpec.unpartitioned())
+            .addSortOrder(SortOrder.unsorted())
+            .discardChanges()
+            .withMetadataLocation("metadata-location")
+            .build();
+
+    LoadTableResponse response = LoadTableResponse.builder().withTableMetadata(metadata).build();
+
+    String expectedJson =
+        String.format(
+            "{\n"
+                + "  \"metadata-location\" : \"metadata-location\",\n"
+                + "  \"metadata\" : {\n"
+                + "    \"format-version\" : 2,\n"
+                + "    \"table-uuid\" : \"386b9f01-002b-4d8c-b77f-42c3fd3b7c9b\",\n"
+                + "    \"location\" : \"location\",\n"
+                + "    \"last-sequence-number\" : 0,\n"
+                + "    \"last-updated-ms\" : %d,\n"
+                + "    \"last-column-id\" : 1,\n"
+                + "    \"current-schema-id\" : 0,\n"
+                + "    \"schemas\" : [ {\n"
+                + "      \"type\" : \"struct\",\n"
+                + "      \"schema-id\" : 0,\n"
+                + "      \"fields\" : [ {\n"
+                + "        \"id\" : 1,\n"
+                + "        \"name\" : \"x\",\n"
+                + "        \"required\" : true,\n"
+                + "        \"type\" : \"long\"\n"
+                + "      } ]\n"
+                + "    } ],\n"
+                + "    \"default-spec-id\" : 0,\n"
+                + "    \"partition-specs\" : [ {\n"
+                + "      \"spec-id\" : 0,\n"
+                + "      \"fields\" : [ ]\n"
+                + "    } ],\n"
+                + "    \"last-partition-id\" : 999,\n"
+                + "    \"default-sort-order-id\" : 0,\n"
+                + "    \"sort-orders\" : [ {\n"
+                + "      \"order-id\" : 0,\n"
+                + "      \"fields\" : [ ]\n"
+                + "    } ],\n"
+                + "    \"properties\" : { },\n"
+                + "    \"current-snapshot-id\" : -1,\n"
+                + "    \"refs\" : { },\n"
+                + "    \"snapshots\" : [ ],\n"
+                + "    \"statistics\" : [ ],\n"
+                + "    \"partition-statistics\" : [ ],\n"
+                + "    \"snapshot-log\" : [ ],\n"
+                + "    \"metadata-log\" : [ ]\n"
+                + "  }\n"
+                + "}",
+            metadata.lastUpdatedMillis());
+
+    String json = LoadTableResponseParser.toJson(response, true);
+    assertThat(json).isEqualTo(expectedJson);
+    // can't do an equality comparison because Schema doesn't implement equals/hashCode
+    assertThat(LoadTableResponseParser.toJson(LoadTableResponseParser.fromJson(json), true))
+        .isEqualTo(expectedJson);
+  }
+
+  @Test
+  public void roundTripSerdeWithConfig() {
+    String uuid = "386b9f01-002b-4d8c-b77f-42c3fd3b7c9b";
+    TableMetadata metadata =
+        TableMetadata.buildFromEmpty()
+            .assignUUID(uuid)
+            .setLocation("location")
+            .setCurrentSchema(
+                new Schema(Types.NestedField.required(1, "x", Types.LongType.get())), 1)
+            .addPartitionSpec(PartitionSpec.unpartitioned())
+            .addSortOrder(SortOrder.unsorted())
+            .discardChanges()
+            .withMetadataLocation("metadata-location")
+            .build();
+
+    LoadTableResponse response =
+        LoadTableResponse.builder()
+            .withTableMetadata(metadata)
+            .addAllConfig(ImmutableMap.of("key1", "val1", "key2", "val2"))
+            .build();
+
+    String expectedJson =
+        String.format(
+            "{\n"
+                + "  \"metadata-location\" : \"metadata-location\",\n"
+                + "  \"metadata\" : {\n"
+                + "    \"format-version\" : 2,\n"
+                + "    \"table-uuid\" : \"386b9f01-002b-4d8c-b77f-42c3fd3b7c9b\",\n"
+                + "    \"location\" : \"location\",\n"
+                + "    \"last-sequence-number\" : 0,\n"
+                + "    \"last-updated-ms\" : %d,\n"
+                + "    \"last-column-id\" : 1,\n"
+                + "    \"current-schema-id\" : 0,\n"
+                + "    \"schemas\" : [ {\n"
+                + "      \"type\" : \"struct\",\n"
+                + "      \"schema-id\" : 0,\n"
+                + "      \"fields\" : [ {\n"
+                + "        \"id\" : 1,\n"
+                + "        \"name\" : \"x\",\n"
+                + "        \"required\" : true,\n"
+                + "        \"type\" : \"long\"\n"
+                + "      } ]\n"
+                + "    } ],\n"
+                + "    \"default-spec-id\" : 0,\n"
+                + "    \"partition-specs\" : [ {\n"
+                + "      \"spec-id\" : 0,\n"
+                + "      \"fields\" : [ ]\n"
+                + "    } ],\n"
+                + "    \"last-partition-id\" : 999,\n"
+                + "    \"default-sort-order-id\" : 0,\n"
+                + "    \"sort-orders\" : [ {\n"
+                + "      \"order-id\" : 0,\n"
+                + "      \"fields\" : [ ]\n"
+                + "    } ],\n"
+                + "    \"properties\" : { },\n"
+                + "    \"current-snapshot-id\" : -1,\n"
+                + "    \"refs\" : { },\n"
+                + "    \"snapshots\" : [ ],\n"
+                + "    \"statistics\" : [ ],\n"
+                + "    \"partition-statistics\" : [ ],\n"
+                + "    \"snapshot-log\" : [ ],\n"
+                + "    \"metadata-log\" : [ ]\n"
+                + "  },\n"
+                + "  \"config\" : {\n"
+                + "    \"key1\" : \"val1\",\n"
+                + "    \"key2\" : \"val2\"\n"
+                + "  }\n"
+                + "}",
+            metadata.lastUpdatedMillis());
+
+    String json = LoadTableResponseParser.toJson(response, true);
+    assertThat(json).isEqualTo(expectedJson);
+    // can't do an equality comparison because Schema doesn't implement equals/hashCode
+    assertThat(LoadTableResponseParser.toJson(LoadTableResponseParser.fromJson(json), true))
+        .isEqualTo(expectedJson);
+  }
+}