bharos commented on code in PR #10593:
URL: https://github.com/apache/gravitino/pull/10593#discussion_r3016864864
##########
iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/TestIcebergTableOperations.java:
##########
@@ -981,4 +982,73 @@ void testLoadTableETagDiffersForSnapshotsParam(Namespace
namespace) {
Assertions.assertEquals(
allEtag, allEtag2, "ETag should be consistent for the same snapshots
value");
}
+
+ @ParameterizedTest
+
@MethodSource("org.apache.gravitino.iceberg.service.rest.IcebergRestTestUtil#testNamespaces")
+ void testLoadTableSnapshotsRefsFiltering(Namespace namespace) {
+ verifyCreateNamespaceSucc(namespace);
+ // Create table with data; generatePlanData=true produces two snapshots
(create + append),
+ // but only the latest snapshot is referenced by the "main" branch.
+ verifyCreateTableSucc(namespace, "snapshots_refs_foo1", true);
+
+ // Load with snapshots=all: should return all snapshots
+ Response allResponse = doLoadTableWithSnapshots(namespace,
"snapshots_refs_foo1", "all");
+ Assertions.assertEquals(Status.OK.getStatusCode(),
allResponse.getStatus());
+ LoadTableResponse allTableResponse =
allResponse.readEntity(LoadTableResponse.class);
+ List<Snapshot> allSnapshots = allTableResponse.tableMetadata().snapshots();
+ Assertions.assertTrue(
+ allSnapshots.size() >= 2,
+ "Table with data should have at least 2 snapshots, got " +
allSnapshots.size());
+
+ // Collect snapshot IDs referenced by refs
+ Map<String, SnapshotRef> refs = allTableResponse.tableMetadata().refs();
+ Set<Long> referencedSnapshotIds =
+
refs.values().stream().map(SnapshotRef::snapshotId).collect(Collectors.toSet());
+
+ // Load with snapshots=refs: should return only ref-referenced snapshots
+ Response refsResponse = doLoadTableWithSnapshots(namespace,
"snapshots_refs_foo1", "refs");
+ Assertions.assertEquals(Status.OK.getStatusCode(),
refsResponse.getStatus());
+ LoadTableResponse refsTableResponse =
refsResponse.readEntity(LoadTableResponse.class);
+ List<Snapshot> refsSnapshots =
refsTableResponse.tableMetadata().snapshots();
+
+ Assertions.assertTrue(
+ refsSnapshots.size() < allSnapshots.size(),
+ "snapshots=refs should return fewer snapshots than snapshots=all");
Review Comment:
Should we check if it is exactly equal to
referencedSnapshotIds.size()
##########
iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/TestIcebergTableOperations.java:
##########
@@ -981,4 +982,73 @@ void testLoadTableETagDiffersForSnapshotsParam(Namespace
namespace) {
Assertions.assertEquals(
allEtag, allEtag2, "ETag should be consistent for the same snapshots
value");
}
+
+ @ParameterizedTest
+
@MethodSource("org.apache.gravitino.iceberg.service.rest.IcebergRestTestUtil#testNamespaces")
+ void testLoadTableSnapshotsRefsFiltering(Namespace namespace) {
+ verifyCreateNamespaceSucc(namespace);
+ // Create table with data; generatePlanData=true produces two snapshots
(create + append),
+ // but only the latest snapshot is referenced by the "main" branch.
+ verifyCreateTableSucc(namespace, "snapshots_refs_foo1", true);
+
+ // Load with snapshots=all: should return all snapshots
+ Response allResponse = doLoadTableWithSnapshots(namespace,
"snapshots_refs_foo1", "all");
+ Assertions.assertEquals(Status.OK.getStatusCode(),
allResponse.getStatus());
+ LoadTableResponse allTableResponse =
allResponse.readEntity(LoadTableResponse.class);
+ List<Snapshot> allSnapshots = allTableResponse.tableMetadata().snapshots();
+ Assertions.assertTrue(
+ allSnapshots.size() >= 2,
+ "Table with data should have at least 2 snapshots, got " +
allSnapshots.size());
+
+ // Collect snapshot IDs referenced by refs
+ Map<String, SnapshotRef> refs = allTableResponse.tableMetadata().refs();
+ Set<Long> referencedSnapshotIds =
+
refs.values().stream().map(SnapshotRef::snapshotId).collect(Collectors.toSet());
+
+ // Load with snapshots=refs: should return only ref-referenced snapshots
+ Response refsResponse = doLoadTableWithSnapshots(namespace,
"snapshots_refs_foo1", "refs");
+ Assertions.assertEquals(Status.OK.getStatusCode(),
refsResponse.getStatus());
+ LoadTableResponse refsTableResponse =
refsResponse.readEntity(LoadTableResponse.class);
+ List<Snapshot> refsSnapshots =
refsTableResponse.tableMetadata().snapshots();
+
+ Assertions.assertTrue(
+ refsSnapshots.size() < allSnapshots.size(),
+ "snapshots=refs should return fewer snapshots than snapshots=all");
+
+ // Every returned snapshot should be referenced by a ref
+ for (Snapshot snapshot : refsSnapshots) {
+ Assertions.assertTrue(
+ referencedSnapshotIds.contains(snapshot.snapshotId()),
+ "Snapshot " + snapshot.snapshotId() + " should be referenced by a
ref");
+ }
Review Comment:
nit : Can we combine the size check and snapshot check above with
```
Assertions.assertEquals(
referencedSnapshotIds,
refsSnapshots.stream().map(Snapshot::snapshotId).collect(Collectors.toSet()),
"snapshots=refs should return exactly the ref-referenced snapshots");
```
--
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]
