test: Add Iceberg test coverage for expire snapshots in Nessie catalog#28155
Open
hantangwangd wants to merge 1 commit into
Open
test: Add Iceberg test coverage for expire snapshots in Nessie catalog#28155hantangwangd wants to merge 1 commit into
hantangwangd wants to merge 1 commit into
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds Nessie-specific test coverage for Iceberg snapshot expiration and makes the generic expiration test explicitly enable GC so it works consistently across catalogs. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
hantangwangd
requested review from
a team,
ZacBlanco and
imjalpreet
as code owners
![sourcery-ai[bot]](https://cdn.github.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new Nessie-specific test hardcodes the
lineitemtable and assumes its presence; consider creating and using a dedicated test table within the test setup to avoid coupling to external data and improve isolation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new Nessie-specific test hardcodes the `lineitem` table and assumes its presence; consider creating and using a dedicated test table within the test setup to avoid coupling to external data and improve isolation.
## Individual Comments
### Comment 1
<location path="presto-iceberg/src/test/java/com/facebook/presto/iceberg/nessie/TestIcebergDistributedNessie.java" line_range="32-33" />
<code_context>
-import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static java.lang.String.format;
@Test
public class TestIcebergDistributedNessie
</code_context>
<issue_to_address>
**suggestion (testing):** Consider expanding the Nessie-specific test to also cover the success path when GC is explicitly enabled
To better exercise Nessie behavior, add a second test that: (1) creates a table, (2) sets `gc.enabled = true` via table properties, then (3) calls `iceberg.system.expire_snapshots` and asserts it succeeds. This will both document the supported way to enable snapshot expiration on Nessie and verify the connector behavior when GC is turned on.
Suggested implementation:
```java
import static com.facebook.presto.iceberg.CatalogType.NESSIE;
import static com.facebook.presto.iceberg.nessie.NessieTestUtil.nessieConnectorProperties;
import static java.lang.String.format;
@Test
public class TestIcebergDistributedNessie
```
Please insert the following new test method into `TestIcebergDistributedNessie` (in the same class as `testExpireSnapshotNotSupportedInNessieByDefault`, e.g. right after it). The method relies on existing helpers like `assertUpdate`, `getQueryRunner()`, and the newly imported `format`:
```java
@Test
public void testExpireSnapshotSupportedWhenGcEnabledInNessie()
{
String schema = getSession().getSchema().get();
String tableName = "test_expire_snapshots_gc_enabled";
// 1. Create a simple Iceberg table in the Nessie catalog
assertUpdate(format("CREATE TABLE %s.%s (key varchar, value bigint)", schema, tableName));
// 2. Explicitly enable GC via table properties
assertUpdate(format("ALTER TABLE %s.%s SET PROPERTIES \"gc.enabled\" = true", schema, tableName));
// 3. Call expire_snapshots and assert it succeeds (no exception thrown)
getQueryRunner().execute(format(
"CALL iceberg.system.expire_snapshots('%s.%s', TIMESTAMP '2000-01-01 00:00:00', 1)",
schema,
tableName));
// Cleanup
assertUpdate(format("DROP TABLE %s.%s", schema, tableName));
}
```
Notes:
1. The exact signature of `iceberg.system.expire_snapshots` should match whatever is used in `testExpireSnapshotNotSupportedInNessieByDefault`. If that test uses a different argument list (e.g. named arguments or a different timestamp), mirror that here.
2. If your test base class uses a different helper than `assertUpdate`/`getQueryRunner().execute` (e.g. `assertQuerySucceeds`), adjust the calls accordingly.
3. Keep `gc.enabled` exactly as spelled here (quoted property name) to align with Iceberg’s GC toggle semantics.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
32
to
33
| @Test | ||
| public class TestIcebergDistributedNessie |
Contributor
There was a problem hiding this comment.
suggestion (testing): Consider expanding the Nessie-specific test to also cover the success path when GC is explicitly enabled
To better exercise Nessie behavior, add a second test that: (1) creates a table, (2) sets gc.enabled = true via table properties, then (3) calls iceberg.system.expire_snapshots and asserts it succeeds. This will both document the supported way to enable snapshot expiration on Nessie and verify the connector behavior when GC is turned on.
Suggested implementation:
import static com.facebook.presto.iceberg.CatalogType.NESSIE;
import static com.facebook.presto.iceberg.nessie.NessieTestUtil.nessieConnectorProperties;
import static java.lang.String.format;
@Test
public class TestIcebergDistributedNessiePlease insert the following new test method into TestIcebergDistributedNessie (in the same class as testExpireSnapshotNotSupportedInNessieByDefault, e.g. right after it). The method relies on existing helpers like assertUpdate, getQueryRunner(), and the newly imported format:
@Test
public void testExpireSnapshotSupportedWhenGcEnabledInNessie()
{
String schema = getSession().getSchema().get();
String tableName = "test_expire_snapshots_gc_enabled";
// 1. Create a simple Iceberg table in the Nessie catalog
assertUpdate(format("CREATE TABLE %s.%s (key varchar, value bigint)", schema, tableName));
// 2. Explicitly enable GC via table properties
assertUpdate(format("ALTER TABLE %s.%s SET PROPERTIES \"gc.enabled\" = true", schema, tableName));
// 3. Call expire_snapshots and assert it succeeds (no exception thrown)
getQueryRunner().execute(format(
"CALL iceberg.system.expire_snapshots('%s.%s', TIMESTAMP '2000-01-01 00:00:00', 1)",
schema,
tableName));
// Cleanup
assertUpdate(format("DROP TABLE %s.%s", schema, tableName));
}Notes:
- The exact signature of
iceberg.system.expire_snapshotsshould match whatever is used intestExpireSnapshotNotSupportedInNessieByDefault. If that test uses a different argument list (e.g. named arguments or a different timestamp), mirror that here. - If your test base class uses a different helper than
assertUpdate/getQueryRunner().execute(e.g.assertQuerySucceeds), adjust the calls accordingly. - Keep
gc.enabledexactly as spelled here (quoted property name) to align with Iceberg’s GC toggle semantics.
imjalpreet
approved these changes
Jul 15, 2026
imjalpreet
left a comment
imjalpreet
left a comment
Member
There was a problem hiding this comment.
Thank you, @hantangwangd. LGTM.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR adds test coverage for the
expire_snapshotsfunctionality in Iceberg connector with Nessie catalog.Motivation and Context
Follow-up from #28083 (comment)
Impact
N/A
Test Plan
Added test cases for expire snapshots in Iceberg with Nessie catalog.
Contributor checklist
Release Notes
Summary by Sourcery
Add Nessie-specific coverage and make generic Iceberg snapshot expiration tests work across catalogs with differing GC settings.
Tests:
Summary by Sourcery
Add targeted test coverage for Iceberg expire_snapshots behavior in Nessie and ensure generic snapshot expiration tests work consistently across catalogs.
Tests: