This is an automated email from the ASF dual-hosted git repository.
etudenhoefner pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg.git
The following commit(s) were added to refs/heads/main by this push:
new d23b0d39bf Core: Include query params into ETag calculation in
reference IRC (#15057)
d23b0d39bf is described below
commit d23b0d39bfd0a1f914aaa6d24d65906fc99200a3
Author: gaborkaszab <[email protected]>
AuthorDate: Tue Feb 10 11:22:18 2026 +0100
Core: Include query params into ETag calculation in reference IRC (#15057)
---
.../java/org/apache/iceberg/rest/ETagProvider.java | 17 ++++-
.../apache/iceberg/rest/RESTCatalogProperties.java | 1 +
.../apache/iceberg/rest/RESTSessionCatalog.java | 3 +-
.../apache/iceberg/rest/RESTCatalogAdapter.java | 21 ++++--
.../org/apache/iceberg/rest/TestETagProvider.java | 29 ++++++--
.../iceberg/rest/TestFreshnessAwareLoading.java | 77 +++++++++++++++++++---
6 files changed, 127 insertions(+), 21 deletions(-)
diff --git a/core/src/main/java/org/apache/iceberg/rest/ETagProvider.java
b/core/src/main/java/org/apache/iceberg/rest/ETagProvider.java
index 82fdc5fcae..bb8e32b0a8 100644
--- a/core/src/main/java/org/apache/iceberg/rest/ETagProvider.java
+++ b/core/src/main/java/org/apache/iceberg/rest/ETagProvider.java
@@ -19,6 +19,9 @@
package org.apache.iceberg.rest;
import java.nio.charset.StandardCharsets;
+import java.util.Map;
+import java.util.TreeMap;
+import org.apache.iceberg.relocated.com.google.common.base.Joiner;
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
import org.apache.iceberg.relocated.com.google.common.hash.HashFunction;
import org.apache.iceberg.relocated.com.google.common.hash.Hashing;
@@ -26,12 +29,22 @@ import
org.apache.iceberg.relocated.com.google.common.hash.Hashing;
class ETagProvider {
private static final HashFunction MURMUR3 = Hashing.murmur3_32_fixed();
+ private static final Joiner.MapJoiner PARAMS_JOINER =
Joiner.on(",").withKeyValueSeparator("=");
+ private static final Joiner COMMA = Joiner.on(',');
+
private ETagProvider() {}
- public static String of(String metadataLocation) {
+ public static String of(String metadataLocation, Map<String, String> params)
{
Preconditions.checkArgument(null != metadataLocation, "Invalid metadata
location: null");
Preconditions.checkArgument(!metadataLocation.isEmpty(), "Invalid metadata
location: empty");
- return MURMUR3.hashString(metadataLocation,
StandardCharsets.UTF_8).toString();
+ String stringToHash = metadataLocation;
+ if (params != null && !params.isEmpty()) {
+ Map<String, String> orderedParams = new TreeMap<>(params);
+
+ stringToHash = COMMA.join(metadataLocation,
PARAMS_JOINER.join(orderedParams));
+ }
+
+ return MURMUR3.hashString(stringToHash, StandardCharsets.UTF_8).toString();
}
}
diff --git
a/core/src/main/java/org/apache/iceberg/rest/RESTCatalogProperties.java
b/core/src/main/java/org/apache/iceberg/rest/RESTCatalogProperties.java
index e294bcfebe..7281862481 100644
--- a/core/src/main/java/org/apache/iceberg/rest/RESTCatalogProperties.java
+++ b/core/src/main/java/org/apache/iceberg/rest/RESTCatalogProperties.java
@@ -26,6 +26,7 @@ public final class RESTCatalogProperties {
public static final String SNAPSHOT_LOADING_MODE = "snapshot-loading-mode";
public static final String SNAPSHOT_LOADING_MODE_DEFAULT =
SnapshotMode.ALL.name();
+ public static final String SNAPSHOTS_QUERY_PARAMETER = "snapshots";
public static final String METRICS_REPORTING_ENABLED =
"rest-metrics-reporting-enabled";
public static final boolean METRICS_REPORTING_ENABLED_DEFAULT = true;
diff --git a/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java
b/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java
index beb350ef03..cda71fccda 100644
--- a/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java
+++ b/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java
@@ -426,7 +426,8 @@ public class RESTSessionCatalog extends
BaseViewSessionCatalog
}
private static Map<String, String> snapshotModeToParam(SnapshotMode mode) {
- return ImmutableMap.of("snapshots", mode.name().toLowerCase(Locale.US));
+ return ImmutableMap.of(
+ RESTCatalogProperties.SNAPSHOTS_QUERY_PARAMETER,
mode.name().toLowerCase(Locale.US));
}
private LoadTableResponse loadInternal(
diff --git a/core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java
b/core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java
index 5c9e8fe6d4..8ba5daef3f 100644
--- a/core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java
+++ b/core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java
@@ -285,7 +285,8 @@ public class RESTCatalogAdapter extends BaseHTTPClient {
CatalogHandlers.createTable(catalog, namespace, request);
responseHeaders.accept(
ImmutableMap.of(
- HttpHeaders.ETAG,
ETagProvider.of(response.metadataLocation())));
+ HttpHeaders.ETAG,
+ ETagProvider.of(response.metadataLocation(),
defaultQueryParams())));
return castResponse(responseType, response);
});
}
@@ -323,7 +324,7 @@ public class RESTCatalogAdapter extends BaseHTTPClient {
Optional<HTTPHeaders.HTTPHeader> ifNoneMatchHeader =
httpRequest.headers().firstEntry(HttpHeaders.IF_NONE_MATCH);
- String eTag = ETagProvider.of(response.metadataLocation());
+ String eTag = ETagProvider.of(response.metadataLocation(),
httpRequest.queryParameters());
if (ifNoneMatchHeader.isPresent() &&
eTag.equals(ifNoneMatchHeader.get().value())) {
return null;
@@ -383,7 +384,8 @@ public class RESTCatalogAdapter extends BaseHTTPClient {
responseHeaders.accept(
ImmutableMap.of(
- HttpHeaders.ETAG,
ETagProvider.of(response.metadataLocation())));
+ HttpHeaders.ETAG,
+ ETagProvider.of(response.metadataLocation(),
defaultQueryParams())));
return castResponse(responseType, response);
});
@@ -402,7 +404,8 @@ public class RESTCatalogAdapter extends BaseHTTPClient {
responseHeaders.accept(
ImmutableMap.of(
- HttpHeaders.ETAG,
ETagProvider.of(response.metadataLocation())));
+ HttpHeaders.ETAG,
+ ETagProvider.of(response.metadataLocation(),
defaultQueryParams())));
return castResponse(responseType, response);
});
@@ -537,6 +540,12 @@ public class RESTCatalogAdapter extends BaseHTTPClient {
return null;
}
+ private static Map<String, String> defaultQueryParams() {
+ return Map.of(
+ RESTCatalogProperties.SNAPSHOTS_QUERY_PARAMETER,
+ SnapshotMode.ALL.toString().toLowerCase(Locale.US));
+ }
+
/**
* This is a very simplistic approach that only validates the requirements
for each table and does
* not do any other conflict detection. Therefore, it does not guarantee
true transactional
@@ -735,7 +744,9 @@ public class RESTCatalogAdapter extends BaseHTTPClient {
private static SnapshotMode snapshotModeFromQueryParams(Map<String, String>
queryParams) {
return SnapshotMode.valueOf(
queryParams
- .getOrDefault("snapshots",
RESTCatalogProperties.SNAPSHOT_LOADING_MODE_DEFAULT)
+ .getOrDefault(
+ RESTCatalogProperties.SNAPSHOTS_QUERY_PARAMETER,
+ RESTCatalogProperties.SNAPSHOT_LOADING_MODE_DEFAULT)
.toUpperCase(Locale.US));
}
}
diff --git a/core/src/test/java/org/apache/iceberg/rest/TestETagProvider.java
b/core/src/test/java/org/apache/iceberg/rest/TestETagProvider.java
index 1a3971492d..aaf5c3442e 100644
--- a/core/src/test/java/org/apache/iceberg/rest/TestETagProvider.java
+++ b/core/src/test/java/org/apache/iceberg/rest/TestETagProvider.java
@@ -21,30 +21,49 @@ package org.apache.iceberg.rest;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import java.util.Map;
import org.junit.jupiter.api.Test;
public class TestETagProvider {
+ static final String METADATA_LOCATION =
+
"/var/folders/20/290st0_52y5fyjcj2mlg49500000gn/T/junit-3064022805908958416/db_name/tbl_name/metadata/00000-f7a7956e-61d0-499b-be60-b141283f8229.metadata.json";
+
@Test
public void testNullInput() {
- assertThatThrownBy(() -> ETagProvider.of(null))
+ assertThatThrownBy(() -> ETagProvider.of(null, null))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("Invalid metadata location: null");
}
@Test
public void testEmptyInput() {
- assertThatThrownBy(() -> ETagProvider.of(""))
+ assertThatThrownBy(() -> ETagProvider.of("", null))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("Invalid metadata location: empty");
}
@Test
public void testETagContent() {
- assertThat("1f865717")
+ assertThat("90b8ad4e")
+ .isEqualTo(
+ ETagProvider.of(METADATA_LOCATION, Map.of("param1", "value1",
"param2", "value2")));
+
+ assertThat("cb787e6a")
.isEqualTo(
ETagProvider.of(
-
"/var/folders/20/290st0_52y5fyjcj2mlg49500000gn/T/junit-3064022805908958416/db_name/tbl_name/metadata/00000-f7a7956e-61d0-499b-be60-b141283f8229.metadata.json"));
+ METADATA_LOCATION, Map.of("param1", "other_value1", "param2",
"other_value2")));
+
+ assertThat("55faa5d9").isEqualTo(ETagProvider.of("/short/path", null));
+
+ assertThat("55faa5d9").isEqualTo(ETagProvider.of("/short/path", Map.of()));
- assertThat("55faa5d9").isEqualTo(ETagProvider.of("/short/path"));
+ assertThat("8adf3766").isEqualTo(ETagProvider.of("/short/path",
Map.of("param", "some_value")));
+ }
+
+ @Test
+ public void testDifferentParameterOrderGiveSameETag() {
+ assertThat(ETagProvider.of(METADATA_LOCATION, Map.of("param1", "value1",
"param2", "value2")))
+ .isEqualTo(
+ ETagProvider.of(METADATA_LOCATION, Map.of("param2", "value2",
"param1", "value1")));
}
}
diff --git
a/core/src/test/java/org/apache/iceberg/rest/TestFreshnessAwareLoading.java
b/core/src/test/java/org/apache/iceberg/rest/TestFreshnessAwareLoading.java
index 0652bb378b..1cee12dfec 100644
--- a/core/src/test/java/org/apache/iceberg/rest/TestFreshnessAwareLoading.java
+++ b/core/src/test/java/org/apache/iceberg/rest/TestFreshnessAwareLoading.java
@@ -169,14 +169,68 @@ public class TestFreshnessAwareLoading extends
TestBaseWithRESTServer {
assertThat(respHeaders).containsEntry(HttpHeaders.ETAG, eTag);
}
+ @Test
+ public void differentETagForDifferentSnapshotMode() {
+ Map<String, String> responseHeaders = Maps.newHashMap();
+ RESTCatalogAdapter adapter =
adapterCapturingResponseHeaders(responseHeaders);
+ RESTCatalog catalog =
+ new RESTCatalog(SessionCatalog.SessionContext.createEmpty(), config ->
adapter);
+ catalog.initialize(
+ "test",
+ ImmutableMap.of(
+ RESTCatalogProperties.SNAPSHOT_LOADING_MODE,
+ RESTCatalogProperties.SnapshotMode.REFS.name()));
+
+ catalog.createNamespace(TABLE.namespace());
+ catalog.createTable(TABLE, SCHEMA);
+
+ assertThat(responseHeaders).containsKey(HttpHeaders.ETAG);
+ String eTagForCreateTable = responseHeaders.get(HttpHeaders.ETAG);
+ responseHeaders.clear();
+
+ catalog.loadTable(TABLE);
+
+ assertThat(responseHeaders).containsKey(HttpHeaders.ETAG);
+
assertThat(eTagForCreateTable).isNotEqualTo(responseHeaders.get(HttpHeaders.ETAG));
+
+ // Verify that table load used the refs query parameter
+ verify(adapter, times(1))
+ .execute(
+ matches(
+ HTTPRequest.HTTPMethod.GET,
+ RESOURCE_PATHS.table(TABLE),
+ Map.of(),
+ Map.of("snapshots", "refs")),
+ eq(LoadTableResponse.class),
+ any(),
+ any());
+ }
+
@Test
public void notModifiedResponse() {
+ // Capture the response headers from createTable to get an ETag.
+ Map<String, String> responseHeaders = Maps.newHashMap();
+ Mockito.doAnswer(
+ invocation ->
+ adapterForRESTServer.execute(
+ invocation.getArgument(0),
+ invocation.getArgument(1),
+ invocation.getArgument(2),
+ responseHeaders::putAll,
+ ParserContext.builder().build()))
+ .when(adapterForRESTServer)
+ .execute(
+ matches(HTTPRequest.HTTPMethod.POST,
RESOURCE_PATHS.tables(TABLE.namespace())),
+ eq(LoadTableResponse.class),
+ any(),
+ any());
+
restCatalog.createNamespace(TABLE.namespace());
restCatalog.createTable(TABLE, SCHEMA);
- Table table = restCatalog.loadTable(TABLE);
+ restCatalog.loadTable(TABLE);
- String eTag =
- ETagProvider.of(((BaseTable)
table).operations().current().metadataFileLocation());
+ assertThat(responseHeaders).containsKeys(HttpHeaders.ETAG);
+ String eTag = responseHeaders.get(HttpHeaders.ETAG);
Mockito.doAnswer(
invocation -> {
@@ -730,8 +784,8 @@ public class TestFreshnessAwareLoading extends
TestBaseWithRESTServer {
assertThat(table.operations()).isInstanceOf(CustomTableOps.class);
}
- private RESTCatalog catalogWithResponseHeaders(Map<String, String>
respHeaders) {
- RESTCatalogAdapter adapter =
+ private RESTCatalogAdapter adapterCapturingResponseHeaders(Map<String,
String> respHeaders) {
+ return Mockito.spy(
new RESTCatalogAdapter(backendCatalog) {
@Override
public <T extends RESTResponse> T execute(
@@ -739,11 +793,18 @@ public class TestFreshnessAwareLoading extends
TestBaseWithRESTServer {
Class<T> responseType,
Consumer<ErrorResponse> errorHandler,
Consumer<Map<String, String>> responseHeaders) {
- return super.execute(request, responseType, errorHandler,
respHeaders::putAll);
+ Consumer<Map<String, String>> compositeConsumer =
+ headers -> {
+ responseHeaders.accept(headers);
+ respHeaders.putAll(headers);
+ };
+ return super.execute(request, responseType, errorHandler,
compositeConsumer);
}
- };
+ });
+ }
- return catalog(adapter);
+ private RESTCatalog catalogWithResponseHeaders(Map<String, String>
respHeaders) {
+ return catalog(adapterCapturingResponseHeaders(respHeaders));
}
private RESTCatalog catalog(RESTCatalogAdapter adapter) {
