Core: Refactor: Test suite for freshness-aware loading #15082

gaborkaszab · 2026-01-19T13:51:51Z

Moves the freshness-aware loading tests from TestRESTCatalog to their own test suite. For this there are some additional changes:

Request matcher functions got their own util class for cross-suite usability
New test base for tests that launch a REST server
TestRESTScanPlannig shares the new test base
New TestRESTTableCache suite

Moves the freshness-aware loading tests from TestRESTCatalog to their own test suite. For this there are some additional changes: - Request matcher functions got their own util class for cross-suite usability - New test base for tests that launch a REST server - TestRESTScanPlannig shares the new test base - New TestRESTTableCache suite

gaborkaszab · 2026-01-19T13:53:43Z

This PR addresses the test-related review comments from #14398. cc @nastra Not sure what would be the best way to carry-over the comments

gaborkaszab

I addressed all of the comments from the other PR with one exception. I added a comment containing the original comment too.

gaborkaszab · 2026-01-19T13:56:25Z

core/src/test/java/org/apache/iceberg/rest/TestFreshnessAwareLoading.java

+        .containsOnlyKeys(
+            RESTTableCache.SessionIdTableId.of(DEFAULT_SESSION_CONTEXT.sessionId(), TABLE));
+
+    assertThat(table).isNotEqualTo(metadataTable.table());


Original comment: "I don't think this will ever be true anyway, because tables don't implement equals"

This checks that the table objects aren't the same.
Note, there is a similar test in testFreshnessAwareLoading too, and it's on purpose to verify that subsequent loadTable calls don't return the same table object. The intent here too but to check the same with the table object inside the metadata table.

but in that case you'd probably want to use isNotSameAs(..) to be more explicit here

Ahh, indeed, thx

nastra · 2026-01-19T14:54:20Z

core/src/test/java/org/apache/iceberg/rest/RESTRequestMatcher.java

+import java.util.Map;
+import java.util.Objects;
+
+class RESTRequestMatcher {


nit: I'd just rename this to RequestMatcher, since the REST part could be a bit misleading given that we deal with HTTP requests

nastra · 2026-01-19T14:57:48Z

core/src/test/java/org/apache/iceberg/rest/TestBaseWithRESTServer.java

+      if (payload instanceof RESTMessage) {
+        RESTMessage message = (RESTMessage) payload;
+        ObjectReader reader = MAPPER.readerFor(message.getClass());
+        if (parserContext != null && !parserContext.isEmpty()) {


this is really only relevant for REST scan planning. We don't have any other uses for this

I added the general version into the base class and an override into TestRESTScanPlanning

nastra · 2026-01-19T15:05:34Z

core/src/test/java/org/apache/iceberg/rest/RESTRequestMatcher.java

+class RESTRequestMatcher {
+  private RESTRequestMatcher() {}
+
+  public static HTTPRequest match(HTTPRequest.HTTPMethod method) {


should all of those be called matches?

Done
It's a bit weird with the contains headers version: matchesContainsHeaders, matchesContainHeaders, not sure

nastra · 2026-01-19T15:07:26Z

core/src/test/java/org/apache/iceberg/rest/TestBaseWithRESTServer.java

+  }
+
+  @AfterEach
+  public void teardownCatalogs() throws Exception {


this should probably be singular to align with setupCatalog. The fact that there's a backing catalog is an implementation detail and doesn't have to be exposed in this method name here

maybe just keep it more generic by having setup/teardown

changed to setup/teardown

In fact check style fails with these names. I renamed them to before()/after()

nastra · 2026-01-19T15:11:39Z

core/src/test/java/org/apache/iceberg/rest/TestBaseWithRESTServer.java

+          ImmutableMap.of());
+
+  protected InMemoryCatalog backendCatalog;
+  protected RESTCatalogAdapter adapterForRESTServer;


nit: I know we're calling it like this in a few places currently but I'm not sure it's actually adding value to have ForRESTServer in the name. Maybe we should just rename it to adapter here and in the method that initializes it

Adapter could be introduced 2 ways and I wanted to call out that this is the adapter that lives in the REST server. The other option would be when the adapter mocks the REST server and the REST client directly gets the answers from the adapter without using the server. I can rename this to adapter but in TestRESTCatalog I found it useful to have this differentiation with the naming to understand better what adapter I use in the tests.

nastra · 2026-01-19T15:14:06Z

core/src/test/java/org/apache/iceberg/rest/TestRESTTableCache.java

+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage("Invalid max entries: negative");
+  }
+}


you might want to add some additional tests that verify basic add/remove/invalidate works for that cache. We don't care about freshness-aware tables here but we really only care about testing the basic features of what the table cache is offering

I was hesitating to keep this PR refactor only or to add new tests here. I added them now. I think that the expiration related tests in TestFreshnessAwareLoading are still relevant and should be kept. WDYT?

gaborkaszab

Thanks for taking a look @nastra

gaborkaszab · 2026-01-20T08:06:23Z

core/src/test/java/org/apache/iceberg/rest/RESTRequestMatcher.java

+import java.util.Map;
+import java.util.Objects;
+
+class RESTRequestMatcher {


gaborkaszab · 2026-01-20T08:07:05Z

core/src/test/java/org/apache/iceberg/rest/RESTRequestMatcher.java

+class RESTRequestMatcher {
+  private RESTRequestMatcher() {}
+
+  public static HTTPRequest match(HTTPRequest.HTTPMethod method) {


Done
It's a bit weird with the contains headers version: matchesContainsHeaders, matchesContainHeaders, not sure

gaborkaszab · 2026-01-20T08:10:26Z

core/src/test/java/org/apache/iceberg/rest/TestBaseWithRESTServer.java

+          ImmutableMap.of());
+
+  protected InMemoryCatalog backendCatalog;
+  protected RESTCatalogAdapter adapterForRESTServer;


Adapter could be introduced 2 ways and I wanted to call out that this is the adapter that lives in the REST server. The other option would be when the adapter mocks the REST server and the REST client directly gets the answers from the adapter without using the server. I can rename this to adapter but in TestRESTCatalog I found it useful to have this differentiation with the naming to understand better what adapter I use in the tests.

gaborkaszab · 2026-01-20T08:15:08Z

core/src/test/java/org/apache/iceberg/rest/TestBaseWithRESTServer.java

+  }
+
+  @AfterEach
+  public void teardownCatalogs() throws Exception {


changed to setup/teardown

gaborkaszab · 2026-01-20T08:23:27Z

core/src/test/java/org/apache/iceberg/rest/TestBaseWithRESTServer.java

+      if (payload instanceof RESTMessage) {
+        RESTMessage message = (RESTMessage) payload;
+        ObjectReader reader = MAPPER.readerFor(message.getClass());
+        if (parserContext != null && !parserContext.isEmpty()) {


I added the general version into the base class and an override into TestRESTScanPlanning

gaborkaszab · 2026-01-20T08:27:11Z

core/src/test/java/org/apache/iceberg/rest/TestFreshnessAwareLoading.java

+        .containsOnlyKeys(
+            RESTTableCache.SessionIdTableId.of(DEFAULT_SESSION_CONTEXT.sessionId(), TABLE));
+
+    assertThat(table).isNotEqualTo(metadataTable.table());


Ahh, indeed, thx

gaborkaszab · 2026-01-20T10:08:21Z

core/src/test/java/org/apache/iceberg/rest/TestRESTTableCache.java

+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage("Invalid max entries: negative");
+  }
+}


I was hesitating to keep this PR refactor only or to add new tests here. I added them now. I think that the expiration related tests in TestFreshnessAwareLoading are still relevant and should be kept. WDYT?

gaborkaszab · 2026-01-20T10:27:09Z

core/src/test/java/org/apache/iceberg/rest/TestBaseWithRESTServer.java

+  }
+
+  @AfterEach
+  public void teardownCatalogs() throws Exception {


In fact check style fails with these names. I renamed them to before()/after()

nastra · 2026-01-20T12:02:45Z

core/src/test/java/org/apache/iceberg/rest/RequestMatcher.java

+                && Objects.equals(req.body(), body));
+  }
+
+  public static HTTPRequest matchesContainsHeaders(


yeah that one I would just name containsHeaders

nastra · 2026-01-20T14:50:15Z

core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java

  }

-  private RESTCatalog catalog(
+  public static RESTCatalog catalog(


why do we need this change?

TestFreshnessAwareLoading has a test for the interworking between customized table ops and freshness-aware loading that uses this utility function. I didn't wan't to copy-paste the code so made some changes so that I can reach it from the other test suite.

in that case I would just duplicate this for now and only refactor once we have 3 or more places where this is needed. I believe this is how we typically handled it in the project

Sure, copy-pasted and reverted the changes on the original version

nastra

LGTM

github-actions bot added the core label Jan 19, 2026

gaborkaszab commented Jan 19, 2026

View reviewed changes

gaborkaszab requested a review from nastra January 19, 2026 13:58

nastra reviewed Jan 19, 2026

View reviewed changes

nastra requested review from amogh-jahagirdar and singhpk234 January 19, 2026 15:09

nastra reviewed Jan 19, 2026

View reviewed changes

gaborkaszab commented Jan 20, 2026

View reviewed changes

review comments

4b37afb

gaborkaszab force-pushed the main_freshness_aware_test_suite branch from e0c8810 to 4b37afb Compare January 20, 2026 10:53

nastra reviewed Jan 20, 2026

View reviewed changes

Rename matchesContainsHeaders to containsHeaders

c544da4

nastra reviewed Jan 20, 2026

View reviewed changes

nastra approved these changes Jan 20, 2026

View reviewed changes

Change visibility of util function

bbb99a6

Core: Refactor: Test suite for freshness-aware loading #15082

Are you sure you want to change the base?

Core: Refactor: Test suite for freshness-aware loading #15082

Conversation

gaborkaszab commented Jan 19, 2026

Uh oh!

gaborkaszab commented Jan 19, 2026

Uh oh!

gaborkaszab left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaborkaszab left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nastra left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers