Copilot commented on code in PR #4724:
URL: https://github.com/apache/polaris/pull/4724#discussion_r3401504638
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandlerTest.java:
##########
@@ -296,4 +304,43 @@ void loadCredentialsFallsBackWhenEntityLocationMissing() {
assertThat(c.config()).containsExactlyInAnyOrderEntriesOf(fakeCredentials);
});
}
+
+ @Test
+ @SuppressWarnings("resource")
+ void filterResponseToSnapshotsRefsPreservesMetadataLocation() throws
Exception {
Review Comment:
The method-level `@SuppressWarnings(\"resource\")` is broad and can mask
real leaks introduced later in this test. If a suppression is still required,
prefer narrowing it to the specific statement/variable causing the warning (or
removing it entirely if the try-with-resources on `InMemoryCatalog` is
sufficient).
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -1399,7 +1400,8 @@ public void renameView(RenameTableRequest request) {
catalogHandlerUtils().renameView(viewCatalog, request);
}
- private @NonNull LoadTableResponse filterResponseToSnapshots(
+ @VisibleForTesting
+ @NonNull LoadTableResponse filterResponseToSnapshots(
LoadTableResponse loadTableResponse, String snapshots) {
Review Comment:
This change widens the method visibility from `private` to package-private.
If the intent is *only* to enable testing, consider keeping production surface
area unchanged by testing via the public entrypoint that calls this method (or
by moving the logic to a package-private helper class dedicated to snapshot
filtering). If you keep it package-private, it may be worth adding a short
comment clarifying it is not intended for production callers.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -1414,8 +1416,14 @@ public void renameView(RenameTableRequest request) {
TableMetadata filteredMetadata =
metadata.removeSnapshotsIf(s ->
!referencedSnapshotIds.contains(s.snapshotId()));
+ TableMetadata filteredMetadataWithLocation =
+ TableMetadata.buildFrom(filteredMetadata)
+ .withMetadataLocation(metadata.metadataFileLocation())
+ .discardChanges()
+ .build();
Review Comment:
The goal is to preserve the response’s `metadata-location`, but this uses
`metadata.metadataFileLocation()` from the *original table metadata* rather
than the `LoadTableResponse`’s `metadataLocation()`. If those ever diverge
(e.g., response explicitly sets `metadata-location`, proxying, or future
Iceberg behavior changes), the filtered response may not preserve the exact
original value. Prefer sourcing the location from
`loadTableResponse.metadataLocation()` (falling back to
`metadata.metadataFileLocation()` only if needed) so the filtered response
preserves what the client would have received.
--
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]
