You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "gabeiglio (via GitHub)" <gi...@apache.org> on 2025/09/23 02:06:20 UTC
[PR] Rest: Allow refresh() to load refs only metadata [iceberg]
gabeiglio opened a new pull request, #14166:
URL: https://github.com/apache/iceberg/pull/14166
`table.refresh()` shouldn't load all the snapshots if we have refs only configured in the catalog. I added 2 more constructors for RESTTableOperations to allow RESTSessionCatalog to pass the query params. I am open to other solutions like overloading refresh with `refresh(boolean loadRefsOnly`) for example.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Rest: Allow refresh() to load refs only metadata [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #14166:
URL: https://github.com/apache/iceberg/pull/14166#issuecomment-3524471665
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Rest: Allow refresh() to load refs only metadata [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #14166:
URL: https://github.com/apache/iceberg/pull/14166#issuecomment-4061798067
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Rest: Allow refresh() to load refs only metadata [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on code in PR #14166:
URL: https://github.com/apache/iceberg/pull/14166#discussion_r2374807470
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -887,6 +887,103 @@ public void testTableCredential(String oauth2ServerUri) {
oauth2ServerUri);
}
+ @Test
+ public void testTableRefreshLoadsRefsOnly() {
+ RESTCatalogAdapter adapter = Mockito.spy(new RESTCatalogAdapter(backendCatalog));
+
+ RESTCatalog catalog =
+ new RESTCatalog(SessionCatalog.SessionContext.createEmpty(), (config) -> adapter);
+ catalog.initialize(
+ "test",
+ ImmutableMap.of(
+ CatalogProperties.URI,
+ "ignored",
+ CatalogProperties.FILE_IO_IMPL,
+ "org.apache.iceberg.inmemory.InMemoryFileIO",
+ // default loading to refs only
+ RESTCatalogProperties.SNAPSHOT_LOADING_MODE,
+ SnapshotMode.REFS.name()));
+
+ if (requiresNamespaceCreate()) {
+ catalog.createNamespace(TABLE.namespace());
+ }
+
+ // Create a table with multiple snapshots
+ Table table = catalog.createTable(TABLE, SCHEMA);
+ table
+ .newFastAppend()
+ .appendFile(
+ DataFiles.builder(PartitionSpec.unpartitioned())
+ .withPath("/path/to/data-a.parquet")
+ .withFileSizeInBytes(10)
+ .withRecordCount(2)
+ .build())
+ .commit();
+
+ table
+ .newFastAppend()
+ .appendFile(
+ DataFiles.builder(PartitionSpec.unpartitioned())
+ .withPath("/path/to/data-b.parquet")
+ .withFileSizeInBytes(10)
+ .withRecordCount(2)
+ .build())
+ .commit();
+
+ ResourcePaths paths = ResourcePaths.forCatalogProperties(Maps.newHashMap());
+
+ // Respond with only referenced snapshots
+ Answer<?> refsAnswer =
+ invocation -> {
+ LoadTableResponse originalResponse = (LoadTableResponse) invocation.callRealMethod();
+ TableMetadata refsMetadata =
+ TableMetadata.buildFrom(originalResponse.tableMetadata())
+ .suppressHistoricalSnapshots()
+ .build();
+
+ // don't call snapshots() directly as that would cause to load all snapshots. Instead,
+ // make sure the snapshots field holds exactly 1 snapshot
+ assertThat(refsMetadata)
+ .extracting("snapshots")
+ .asInstanceOf(InstanceOfAssertFactories.list(Snapshot.class))
+ .hasSize(1);
+
+ return LoadTableResponse.builder()
+ .withTableMetadata(refsMetadata)
+ .addAllConfig(originalResponse.config())
+ .build();
+ };
+
+ Mockito.doAnswer(refsAnswer)
+ .when(adapter)
+ .execute(
+ reqMatcher(HTTPMethod.GET, paths.table(TABLE), Map.of(), Map.of("snapshots", "refs")),
+ eq(LoadTableResponse.class),
+ any(),
+ any());
+
+ Table refsTable = catalog.loadTable(TABLE);
+ refsTable.refresh();
Review Comment:
Partially scratch what I wrote: there is not much sense to do partial snapshot loading for createTable() since there won't be a list of snapshots anyway. But for subsequent refreshes() or commits() we could still do the partial loading. For this the adjustment for the RESTTableOperations is needed.
In that thread I linked above, @nastra said that partial snapshot loading could make sense for RESTTableOperations.refresh() and commit() but only in case there were no snapshots() call since the creation of the ops, or in other works in case current.snapshotsLoaded is false. This flag is not exposed now, but in case we exposed it, then there might be no need to pass SnapshotMode to the RESTTableOps, because snapshptsLoaded can only be false in case REFS were used for loading the table so would be somewhat redundant.
WDYT @nastra ?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Rest: Allow refresh() to load refs only metadata [iceberg]
Posted by "gabeiglio (via GitHub)" <gi...@apache.org>.
gabeiglio commented on PR #14166:
URL: https://github.com/apache/iceberg/pull/14166#issuecomment-3789211365
Yea I can do that no problem. By reviving the PR do we mean waiting for a someone to reopen the PR and rebase or just open a new PR?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Rest: Allow refresh() to load refs only metadata [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #14166: Rest: Allow refresh() to load refs only metadata
URL: https://github.com/apache/iceberg/pull/14166
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Rest: Allow refresh() to load refs only metadata [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14166:
URL: https://github.com/apache/iceberg/pull/14166#discussion_r2374897201
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -887,6 +887,103 @@ public void testTableCredential(String oauth2ServerUri) {
oauth2ServerUri);
}
+ @Test
+ public void testTableRefreshLoadsRefsOnly() {
+ RESTCatalogAdapter adapter = Mockito.spy(new RESTCatalogAdapter(backendCatalog));
+
+ RESTCatalog catalog =
+ new RESTCatalog(SessionCatalog.SessionContext.createEmpty(), (config) -> adapter);
+ catalog.initialize(
+ "test",
+ ImmutableMap.of(
+ CatalogProperties.URI,
+ "ignored",
+ CatalogProperties.FILE_IO_IMPL,
+ "org.apache.iceberg.inmemory.InMemoryFileIO",
+ // default loading to refs only
+ RESTCatalogProperties.SNAPSHOT_LOADING_MODE,
+ SnapshotMode.REFS.name()));
+
+ if (requiresNamespaceCreate()) {
+ catalog.createNamespace(TABLE.namespace());
+ }
+
+ // Create a table with multiple snapshots
+ Table table = catalog.createTable(TABLE, SCHEMA);
+ table
+ .newFastAppend()
+ .appendFile(
+ DataFiles.builder(PartitionSpec.unpartitioned())
+ .withPath("/path/to/data-a.parquet")
+ .withFileSizeInBytes(10)
+ .withRecordCount(2)
+ .build())
+ .commit();
+
+ table
+ .newFastAppend()
+ .appendFile(
+ DataFiles.builder(PartitionSpec.unpartitioned())
+ .withPath("/path/to/data-b.parquet")
+ .withFileSizeInBytes(10)
+ .withRecordCount(2)
+ .build())
+ .commit();
+
+ ResourcePaths paths = ResourcePaths.forCatalogProperties(Maps.newHashMap());
+
+ // Respond with only referenced snapshots
+ Answer<?> refsAnswer =
+ invocation -> {
+ LoadTableResponse originalResponse = (LoadTableResponse) invocation.callRealMethod();
+ TableMetadata refsMetadata =
+ TableMetadata.buildFrom(originalResponse.tableMetadata())
+ .suppressHistoricalSnapshots()
+ .build();
+
+ // don't call snapshots() directly as that would cause to load all snapshots. Instead,
+ // make sure the snapshots field holds exactly 1 snapshot
+ assertThat(refsMetadata)
+ .extracting("snapshots")
+ .asInstanceOf(InstanceOfAssertFactories.list(Snapshot.class))
+ .hasSize(1);
+
+ return LoadTableResponse.builder()
+ .withTableMetadata(refsMetadata)
+ .addAllConfig(originalResponse.config())
+ .build();
+ };
+
+ Mockito.doAnswer(refsAnswer)
+ .when(adapter)
+ .execute(
+ reqMatcher(HTTPMethod.GET, paths.table(TABLE), Map.of(), Map.of("snapshots", "refs")),
+ eq(LoadTableResponse.class),
+ any(),
+ any());
+
+ Table refsTable = catalog.loadTable(TABLE);
+ refsTable.refresh();
Review Comment:
> or in other works in case current.snapshotsLoaded is false
I actually didn't mean to refer to this flag and we don't want to expose this, since this is internal information. What I meant is that it's not helpful to have ref loading in a place when the calling code potentially triggers an implicit call of `snapshots()`, which would do another request to load the remaining snapshots. That also means if there are cases where `snapshots()` isn't triggered and the table has lots and lots of snapshots, then this would be a good use case to think about adding ref loading mode there
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Rest: Allow refresh() to load refs only metadata [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #14166: Rest: Allow refresh() to load refs only metadata
URL: https://github.com/apache/iceberg/pull/14166
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Rest: Allow refresh() to load refs only metadata [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #14166:
URL: https://github.com/apache/iceberg/pull/14166#issuecomment-4014948208
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Rest: Allow refresh() to load refs only metadata [iceberg]
Posted by "gabeiglio (via GitHub)" <gi...@apache.org>.
gabeiglio commented on code in PR #14166:
URL: https://github.com/apache/iceberg/pull/14166#discussion_r2373328903
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -887,6 +887,103 @@ public void testTableCredential(String oauth2ServerUri) {
oauth2ServerUri);
}
+ @Test
+ public void testTableRefreshLoadsRefsOnly() {
+ RESTCatalogAdapter adapter = Mockito.spy(new RESTCatalogAdapter(backendCatalog));
+
+ RESTCatalog catalog =
+ new RESTCatalog(SessionCatalog.SessionContext.createEmpty(), (config) -> adapter);
+ catalog.initialize(
+ "test",
+ ImmutableMap.of(
+ CatalogProperties.URI,
+ "ignored",
+ CatalogProperties.FILE_IO_IMPL,
+ "org.apache.iceberg.inmemory.InMemoryFileIO",
+ // default loading to refs only
+ RESTCatalogProperties.SNAPSHOT_LOADING_MODE,
+ SnapshotMode.REFS.name()));
+
+ if (requiresNamespaceCreate()) {
+ catalog.createNamespace(TABLE.namespace());
+ }
+
+ // Create a table with multiple snapshots
+ Table table = catalog.createTable(TABLE, SCHEMA);
+ table
+ .newFastAppend()
+ .appendFile(
+ DataFiles.builder(PartitionSpec.unpartitioned())
+ .withPath("/path/to/data-a.parquet")
+ .withFileSizeInBytes(10)
+ .withRecordCount(2)
+ .build())
+ .commit();
+
+ table
+ .newFastAppend()
+ .appendFile(
+ DataFiles.builder(PartitionSpec.unpartitioned())
+ .withPath("/path/to/data-b.parquet")
+ .withFileSizeInBytes(10)
+ .withRecordCount(2)
+ .build())
+ .commit();
+
+ ResourcePaths paths = ResourcePaths.forCatalogProperties(Maps.newHashMap());
+
+ // Respond with only referenced snapshots
+ Answer<?> refsAnswer =
Review Comment:
nice!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Rest: Allow refresh() to load refs only metadata [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on code in PR #14166:
URL: https://github.com/apache/iceberg/pull/14166#discussion_r2426610437
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -887,6 +887,103 @@ public void testTableCredential(String oauth2ServerUri) {
oauth2ServerUri);
}
+ @Test
+ public void testTableRefreshLoadsRefsOnly() {
+ RESTCatalogAdapter adapter = Mockito.spy(new RESTCatalogAdapter(backendCatalog));
+
+ RESTCatalog catalog =
+ new RESTCatalog(SessionCatalog.SessionContext.createEmpty(), (config) -> adapter);
+ catalog.initialize(
+ "test",
+ ImmutableMap.of(
+ CatalogProperties.URI,
+ "ignored",
+ CatalogProperties.FILE_IO_IMPL,
+ "org.apache.iceberg.inmemory.InMemoryFileIO",
+ // default loading to refs only
+ RESTCatalogProperties.SNAPSHOT_LOADING_MODE,
+ SnapshotMode.REFS.name()));
+
+ if (requiresNamespaceCreate()) {
+ catalog.createNamespace(TABLE.namespace());
+ }
+
+ // Create a table with multiple snapshots
+ Table table = catalog.createTable(TABLE, SCHEMA);
+ table
+ .newFastAppend()
+ .appendFile(
+ DataFiles.builder(PartitionSpec.unpartitioned())
+ .withPath("/path/to/data-a.parquet")
+ .withFileSizeInBytes(10)
+ .withRecordCount(2)
+ .build())
+ .commit();
+
+ table
+ .newFastAppend()
+ .appendFile(
+ DataFiles.builder(PartitionSpec.unpartitioned())
+ .withPath("/path/to/data-b.parquet")
+ .withFileSizeInBytes(10)
+ .withRecordCount(2)
+ .build())
+ .commit();
+
+ ResourcePaths paths = ResourcePaths.forCatalogProperties(Maps.newHashMap());
+
+ // Respond with only referenced snapshots
+ Answer<?> refsAnswer =
+ invocation -> {
+ LoadTableResponse originalResponse = (LoadTableResponse) invocation.callRealMethod();
+ TableMetadata refsMetadata =
+ TableMetadata.buildFrom(originalResponse.tableMetadata())
+ .suppressHistoricalSnapshots()
+ .build();
+
+ // don't call snapshots() directly as that would cause to load all snapshots. Instead,
+ // make sure the snapshots field holds exactly 1 snapshot
+ assertThat(refsMetadata)
+ .extracting("snapshots")
+ .asInstanceOf(InstanceOfAssertFactories.list(Snapshot.class))
+ .hasSize(1);
+
+ return LoadTableResponse.builder()
+ .withTableMetadata(refsMetadata)
+ .addAllConfig(originalResponse.config())
+ .build();
+ };
+
+ Mockito.doAnswer(refsAnswer)
+ .when(adapter)
+ .execute(
+ reqMatcher(HTTPMethod.GET, paths.table(TABLE), Map.of(), Map.of("snapshots", "refs")),
+ eq(LoadTableResponse.class),
+ any(),
+ any());
+
+ Table refsTable = catalog.loadTable(TABLE);
+ refsTable.refresh();
Review Comment:
Thanks for sharing your view, @nastra ! I got your comments wrong in the Slack conversation, sorry about that. So there are use-cases where this could help, but it's still not clear for me that this PR is a go or a no-go.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Rest: Allow refresh() to load refs only metadata [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on code in PR #14166:
URL: https://github.com/apache/iceberg/pull/14166#discussion_r2372349980
##########
core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java:
##########
@@ -68,7 +70,36 @@ enum UpdateType {
FileIO io,
TableMetadata current,
Set<Endpoint> endpoints) {
- this(client, path, headers, io, UpdateType.SIMPLE, Lists.newArrayList(), current, endpoints);
+ this(
+ client,
+ path,
+ headers,
+ io,
+ UpdateType.SIMPLE,
+ Lists.newArrayList(),
+ current,
+ endpoints,
+ ImmutableMap.of());
+ }
+
+ RESTTableOperations(
Review Comment:
See my other comment, but I feel we could extend the existing constructors with snapshotMode query param and not introduce more variants of the constructors.
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -887,6 +887,103 @@ public void testTableCredential(String oauth2ServerUri) {
oauth2ServerUri);
}
+ @Test
+ public void testTableRefreshLoadsRefsOnly() {
+ RESTCatalogAdapter adapter = Mockito.spy(new RESTCatalogAdapter(backendCatalog));
+
+ RESTCatalog catalog =
+ new RESTCatalog(SessionCatalog.SessionContext.createEmpty(), (config) -> adapter);
+ catalog.initialize(
+ "test",
+ ImmutableMap.of(
+ CatalogProperties.URI,
+ "ignored",
+ CatalogProperties.FILE_IO_IMPL,
+ "org.apache.iceberg.inmemory.InMemoryFileIO",
+ // default loading to refs only
+ RESTCatalogProperties.SNAPSHOT_LOADING_MODE,
+ SnapshotMode.REFS.name()));
+
+ if (requiresNamespaceCreate()) {
+ catalog.createNamespace(TABLE.namespace());
+ }
+
+ // Create a table with multiple snapshots
+ Table table = catalog.createTable(TABLE, SCHEMA);
+ table
+ .newFastAppend()
+ .appendFile(
+ DataFiles.builder(PartitionSpec.unpartitioned())
+ .withPath("/path/to/data-a.parquet")
+ .withFileSizeInBytes(10)
+ .withRecordCount(2)
+ .build())
+ .commit();
+
+ table
+ .newFastAppend()
+ .appendFile(
+ DataFiles.builder(PartitionSpec.unpartitioned())
+ .withPath("/path/to/data-b.parquet")
+ .withFileSizeInBytes(10)
+ .withRecordCount(2)
+ .build())
+ .commit();
+
+ ResourcePaths paths = ResourcePaths.forCatalogProperties(Maps.newHashMap());
+
+ // Respond with only referenced snapshots
+ Answer<?> refsAnswer =
Review Comment:
with [this PR](https://github.com/apache/iceberg/pull/14060) this part of the test won't be needed to simulate refs only answer.
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -456,7 +456,8 @@ public Table loadTable(SessionContext context, TableIdentifier identifier) {
Map::of,
tableFileIO(context, tableConf, response.credentials()),
tableMetadata,
- endpoints);
+ endpoints,
+ snapshotModeToParam(snapshotMode));
Review Comment:
I see other occasions where RESTTableOperations is constructed in RESTSessionCatalog, like create(), createTransaction(), and others. `snapshotMode` is already present there too. Do you think it makes sense to use the new constructor there too?
Also, if there are no other calls, then the old constructor of RESTTableOperations could be removed.
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -887,6 +887,103 @@ public void testTableCredential(String oauth2ServerUri) {
oauth2ServerUri);
}
+ @Test
+ public void testTableRefreshLoadsRefsOnly() {
+ RESTCatalogAdapter adapter = Mockito.spy(new RESTCatalogAdapter(backendCatalog));
+
+ RESTCatalog catalog =
+ new RESTCatalog(SessionCatalog.SessionContext.createEmpty(), (config) -> adapter);
+ catalog.initialize(
+ "test",
+ ImmutableMap.of(
+ CatalogProperties.URI,
+ "ignored",
+ CatalogProperties.FILE_IO_IMPL,
+ "org.apache.iceberg.inmemory.InMemoryFileIO",
+ // default loading to refs only
+ RESTCatalogProperties.SNAPSHOT_LOADING_MODE,
+ SnapshotMode.REFS.name()));
+
+ if (requiresNamespaceCreate()) {
+ catalog.createNamespace(TABLE.namespace());
+ }
+
+ // Create a table with multiple snapshots
+ Table table = catalog.createTable(TABLE, SCHEMA);
+ table
+ .newFastAppend()
+ .appendFile(
+ DataFiles.builder(PartitionSpec.unpartitioned())
+ .withPath("/path/to/data-a.parquet")
+ .withFileSizeInBytes(10)
+ .withRecordCount(2)
+ .build())
+ .commit();
+
+ table
+ .newFastAppend()
+ .appendFile(
+ DataFiles.builder(PartitionSpec.unpartitioned())
+ .withPath("/path/to/data-b.parquet")
+ .withFileSizeInBytes(10)
+ .withRecordCount(2)
+ .build())
+ .commit();
+
+ ResourcePaths paths = ResourcePaths.forCatalogProperties(Maps.newHashMap());
+
+ // Respond with only referenced snapshots
+ Answer<?> refsAnswer =
+ invocation -> {
+ LoadTableResponse originalResponse = (LoadTableResponse) invocation.callRealMethod();
+ TableMetadata refsMetadata =
+ TableMetadata.buildFrom(originalResponse.tableMetadata())
+ .suppressHistoricalSnapshots()
+ .build();
+
+ // don't call snapshots() directly as that would cause to load all snapshots. Instead,
+ // make sure the snapshots field holds exactly 1 snapshot
+ assertThat(refsMetadata)
+ .extracting("snapshots")
+ .asInstanceOf(InstanceOfAssertFactories.list(Snapshot.class))
+ .hasSize(1);
+
+ return LoadTableResponse.builder()
+ .withTableMetadata(refsMetadata)
+ .addAllConfig(originalResponse.config())
+ .build();
+ };
+
+ Mockito.doAnswer(refsAnswer)
+ .when(adapter)
+ .execute(
+ reqMatcher(HTTPMethod.GET, paths.table(TABLE), Map.of(), Map.of("snapshots", "refs")),
+ eq(LoadTableResponse.class),
+ any(),
+ any());
+
+ Table refsTable = catalog.loadTable(TABLE);
+ refsTable.refresh();
Review Comment:
I think the current implementation of lazy snapshot loading is inconsistent, probably not within the scope of this PR, but for the record:
```
Table table = catalog.createTable(TABLE, SCHEMA);
table.refresh();
```
The above code loads the table twice as this test, however both of them loads the full snapshot list not respecting the `REFS` snapshot loading mode.
This PR might want to solve this for the second load with passing the `snapshotMode` to `RESTTableOperations` all across `RESTSessionCatalog`.
Adjusting the first load (for createTable() and all) would require a REST spec change. See my question about this [on Slack](https://apache-iceberg.slack.com/archives/C03LG1D563F/p1758629299891649).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Rest: Allow refresh() to load refs only metadata [iceberg]
Posted by "gabeiglio (via GitHub)" <gi...@apache.org>.
gabeiglio commented on code in PR #14166:
URL: https://github.com/apache/iceberg/pull/14166#discussion_r2377156138
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -887,6 +887,103 @@ public void testTableCredential(String oauth2ServerUri) {
oauth2ServerUri);
}
+ @Test
+ public void testTableRefreshLoadsRefsOnly() {
+ RESTCatalogAdapter adapter = Mockito.spy(new RESTCatalogAdapter(backendCatalog));
+
+ RESTCatalog catalog =
+ new RESTCatalog(SessionCatalog.SessionContext.createEmpty(), (config) -> adapter);
+ catalog.initialize(
+ "test",
+ ImmutableMap.of(
+ CatalogProperties.URI,
+ "ignored",
+ CatalogProperties.FILE_IO_IMPL,
+ "org.apache.iceberg.inmemory.InMemoryFileIO",
+ // default loading to refs only
+ RESTCatalogProperties.SNAPSHOT_LOADING_MODE,
+ SnapshotMode.REFS.name()));
+
+ if (requiresNamespaceCreate()) {
+ catalog.createNamespace(TABLE.namespace());
+ }
+
+ // Create a table with multiple snapshots
+ Table table = catalog.createTable(TABLE, SCHEMA);
+ table
+ .newFastAppend()
+ .appendFile(
+ DataFiles.builder(PartitionSpec.unpartitioned())
+ .withPath("/path/to/data-a.parquet")
+ .withFileSizeInBytes(10)
+ .withRecordCount(2)
+ .build())
+ .commit();
+
+ table
+ .newFastAppend()
+ .appendFile(
+ DataFiles.builder(PartitionSpec.unpartitioned())
+ .withPath("/path/to/data-b.parquet")
+ .withFileSizeInBytes(10)
+ .withRecordCount(2)
+ .build())
+ .commit();
+
+ ResourcePaths paths = ResourcePaths.forCatalogProperties(Maps.newHashMap());
+
+ // Respond with only referenced snapshots
+ Answer<?> refsAnswer =
+ invocation -> {
+ LoadTableResponse originalResponse = (LoadTableResponse) invocation.callRealMethod();
+ TableMetadata refsMetadata =
+ TableMetadata.buildFrom(originalResponse.tableMetadata())
+ .suppressHistoricalSnapshots()
+ .build();
+
+ // don't call snapshots() directly as that would cause to load all snapshots. Instead,
+ // make sure the snapshots field holds exactly 1 snapshot
+ assertThat(refsMetadata)
+ .extracting("snapshots")
+ .asInstanceOf(InstanceOfAssertFactories.list(Snapshot.class))
+ .hasSize(1);
+
+ return LoadTableResponse.builder()
+ .withTableMetadata(refsMetadata)
+ .addAllConfig(originalResponse.config())
+ .build();
+ };
+
+ Mockito.doAnswer(refsAnswer)
+ .when(adapter)
+ .execute(
+ reqMatcher(HTTPMethod.GET, paths.table(TABLE), Map.of(), Map.of("snapshots", "refs")),
+ eq(LoadTableResponse.class),
+ any(),
+ any());
+
+ Table refsTable = catalog.loadTable(TABLE);
+ refsTable.refresh();
Review Comment:
@gaborkaszab @nastra thank you both for putting your thoughts into this! I also concur that we should add this where makes the most sense (mainly refresh, load, and commit). I can add the changes for commit in a separate PR since this will require rest spec changes.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Rest: Allow refresh() to load refs only metadata [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #14166:
URL: https://github.com/apache/iceberg/pull/14166#issuecomment-3560743365
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Rest: Allow refresh() to load refs only metadata [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on PR #14166:
URL: https://github.com/apache/iceberg/pull/14166#issuecomment-3784230863
Hi All,
I've been active on this part of the code recently, and I think this PR makes much sense. If the catalog is in REFS mode then the refresh() on the table should also follow this strategy. E.g. table is loaded with REFS only, then a refresh shouldn't load all the snapshots.
What could also be misleading that these 2 use-cases work differently now:
1) table loaded with REFS, there are no changes since load -> refresh won't load the rest of the snapshots (because metadata location is the same)
2) table loaded with REFS, there are some changes since load -> refresh will load all snapshots not just refs.
@gabeiglio do you think we can revive this PR? If you have no capacity ATM, I can take over, let me know!
@nastra Do you think you can take a look once revived?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Rest: Allow refresh() to load refs only metadata [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on PR #14166:
URL: https://github.com/apache/iceberg/pull/14166#issuecomment-3789376816
@gabeiglio I reopened it now. Would you mind rebasing?
@nastra Do you think we can continue the review on this?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Rest: Allow refresh() to load refs only metadata [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on code in PR #14166:
URL: https://github.com/apache/iceberg/pull/14166#discussion_r2763955445
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -1239,7 +1257,8 @@ protected RESTTableOperations newTableOps(
updateType,
createChanges,
current,
- supportedEndpoints);
+ supportedEndpoints,
+ ImmutableMap.of());
Review Comment:
This could call one of the existing constructors IMO. No need to add an empty map as param, RESTTableOperations can do that when calling different versions of the constructors internally.
##########
core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java:
##########
@@ -61,6 +62,7 @@ enum UpdateType {
private final List<MetadataUpdate> createChanges;
private final TableMetadata replaceBase;
private final Set<Endpoint> endpoints;
+ private final Map<String, String> queryParams;
Review Comment:
For now we only pass snapshot mode. I'm leaning towards only having that as a member and not a general queryParams, to have better understanding what this is.
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -1024,6 +1024,81 @@ public void testTableCredential(String oauth2ServerUri) {
oauth2ServerUri);
}
+ @Test
+ public void testTableRefreshLoadsRefsOnly() {
+ RESTCatalogAdapter adapter = Mockito.spy(new RESTCatalogAdapter(backendCatalog));
+
+ RESTCatalog catalog =
+ new RESTCatalog(SessionCatalog.SessionContext.createEmpty(), (config) -> adapter);
+ catalog.initialize(
+ "test",
+ ImmutableMap.of(
+ CatalogProperties.URI,
+ "ignored",
+ CatalogProperties.FILE_IO_IMPL,
+ "org.apache.iceberg.inmemory.InMemoryFileIO",
+ // default loading to refs only
+ RESTCatalogProperties.SNAPSHOT_LOADING_MODE,
+ SnapshotMode.REFS.name()));
+
+ if (requiresNamespaceCreate()) {
+ catalog.createNamespace(TABLE.namespace());
+ }
+
+ // Create a table with multiple snapshots
+ Table table = catalog.createTable(TABLE, SCHEMA);
+ table
Review Comment:
I know this is how other relevant tests set up such a table, but I think there is a way shorter way:
```
catalog.createNamespace(TABLE.namespace());
Table table = catalog.createTable(TABLE, SCHEMA);
table.newAppend().appendFile(FILE_A).commit();
table.newAppend().appendFile(FILE_B).commit();
```
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -3412,7 +3489,8 @@ protected RESTTableOperations newTableOps(
Supplier<Map<String, String>> mutationHeaders,
FileIO fileIO,
TableMetadata current,
- Set<Endpoint> supportedEndpoints) {
+ Set<Endpoint> supportedEndpoints,
+ Map<String, String> queryParams) {
Review Comment:
I know it's not the best design, but other projects might override `newTableOps` to inject their own way of creating RESTTableOps. If you change the signature, they probably have to change their code too. As written in my earlier comment, we can avoid changing this by directly using `snapshotMode` to the ops constructor.
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -3393,7 +3469,8 @@ class CustomTableOps extends RESTTableOperations {
FileIO io,
TableMetadata current,
Set<Endpoint> endpoints) {
- super(client, path, readHeaders, mutationHeaders, io, current, endpoints);
+ super(
+ client, path, readHeaders, mutationHeaders, io, current, endpoints, ImmutableMap.of());
Review Comment:
I don't think this is necessary
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -569,7 +575,8 @@ private Supplier<BaseTable> createTableSupplier(
SessionContext context,
RESTClient tableClient,
Map<String, String> tableConf,
- List<Credential> credentials) {
+ List<Credential> credentials,
+ Map<String, String> queryParams) {
Review Comment:
Since snapshotMode is a member of the class, I wouldn't extend this and the newTableOps functions with new params, I'd use the member directly where we create the ops object.
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -1024,6 +1024,81 @@ public void testTableCredential(String oauth2ServerUri) {
oauth2ServerUri);
}
+ @Test
+ public void testTableRefreshLoadsRefsOnly() {
+ RESTCatalogAdapter adapter = Mockito.spy(new RESTCatalogAdapter(backendCatalog));
+
+ RESTCatalog catalog =
+ new RESTCatalog(SessionCatalog.SessionContext.createEmpty(), (config) -> adapter);
+ catalog.initialize(
+ "test",
+ ImmutableMap.of(
+ CatalogProperties.URI,
+ "ignored",
+ CatalogProperties.FILE_IO_IMPL,
+ "org.apache.iceberg.inmemory.InMemoryFileIO",
+ // default loading to refs only
+ RESTCatalogProperties.SNAPSHOT_LOADING_MODE,
+ SnapshotMode.REFS.name()));
+
+ if (requiresNamespaceCreate()) {
Review Comment:
This is always tru in this test suite. no need to check
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -1024,6 +1024,81 @@ public void testTableCredential(String oauth2ServerUri) {
oauth2ServerUri);
}
+ @Test
+ public void testTableRefreshLoadsRefsOnly() {
Review Comment:
nit: no need for the 'test' prefix in the name.
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -1024,6 +1024,81 @@ public void testTableCredential(String oauth2ServerUri) {
oauth2ServerUri);
}
+ @Test
+ public void testTableRefreshLoadsRefsOnly() {
+ RESTCatalogAdapter adapter = Mockito.spy(new RESTCatalogAdapter(backendCatalog));
+
+ RESTCatalog catalog =
+ new RESTCatalog(SessionCatalog.SessionContext.createEmpty(), (config) -> adapter);
+ catalog.initialize(
+ "test",
+ ImmutableMap.of(
+ CatalogProperties.URI,
+ "ignored",
+ CatalogProperties.FILE_IO_IMPL,
+ "org.apache.iceberg.inmemory.InMemoryFileIO",
+ // default loading to refs only
+ RESTCatalogProperties.SNAPSHOT_LOADING_MODE,
+ SnapshotMode.REFS.name()));
+
+ if (requiresNamespaceCreate()) {
+ catalog.createNamespace(TABLE.namespace());
+ }
+
+ // Create a table with multiple snapshots
+ Table table = catalog.createTable(TABLE, SCHEMA);
+ table
+ .newFastAppend()
+ .appendFile(
+ DataFiles.builder(PartitionSpec.unpartitioned())
+ .withPath("/path/to/data-a.parquet")
+ .withFileSizeInBytes(10)
+ .withRecordCount(2)
+ .build())
+ .commit();
+
+ table
+ .newFastAppend()
+ .appendFile(
+ DataFiles.builder(PartitionSpec.unpartitioned())
+ .withPath("/path/to/data-b.parquet")
+ .withFileSizeInBytes(10)
+ .withRecordCount(2)
+ .build())
+ .commit();
+
+ ResourcePaths paths = ResourcePaths.forCatalogProperties(Maps.newHashMap());
+
+ Table refsTable = catalog.loadTable(TABLE);
+ refsTable.refresh();
+
+ // don't call snapshots() directly as that would cause to load all snapshots. Instead,
+ // make sure the snapshots field holds exactly 1 snapshot
+ assertThat(((BaseTable) refsTable).operations().current())
+ .extracting("snapshots")
+ .asInstanceOf(InstanceOfAssertFactories.list(Snapshot.class))
+ .hasSize(1);
+
+ assertThat(refsTable.currentSnapshot()).isEqualTo(table.currentSnapshot());
+
+ // verify that the table was loaded with the refs argument
+ verify(adapter, times(2))
+ .execute(
+ reqMatcher(HTTPMethod.GET, paths.table(TABLE), Map.of(), Map.of("snapshots", "refs")),
Review Comment:
`reqMatcher` was moved to another class. `matches` is how to use it now
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -1024,6 +1024,81 @@ public void testTableCredential(String oauth2ServerUri) {
oauth2ServerUri);
}
+ @Test
+ public void testTableRefreshLoadsRefsOnly() {
+ RESTCatalogAdapter adapter = Mockito.spy(new RESTCatalogAdapter(backendCatalog));
+
+ RESTCatalog catalog =
+ new RESTCatalog(SessionCatalog.SessionContext.createEmpty(), (config) -> adapter);
+ catalog.initialize(
+ "test",
+ ImmutableMap.of(
+ CatalogProperties.URI,
+ "ignored",
+ CatalogProperties.FILE_IO_IMPL,
+ "org.apache.iceberg.inmemory.InMemoryFileIO",
+ // default loading to refs only
+ RESTCatalogProperties.SNAPSHOT_LOADING_MODE,
+ SnapshotMode.REFS.name()));
+
+ if (requiresNamespaceCreate()) {
+ catalog.createNamespace(TABLE.namespace());
+ }
+
+ // Create a table with multiple snapshots
+ Table table = catalog.createTable(TABLE, SCHEMA);
+ table
+ .newFastAppend()
+ .appendFile(
+ DataFiles.builder(PartitionSpec.unpartitioned())
+ .withPath("/path/to/data-a.parquet")
+ .withFileSizeInBytes(10)
+ .withRecordCount(2)
+ .build())
+ .commit();
+
+ table
+ .newFastAppend()
+ .appendFile(
+ DataFiles.builder(PartitionSpec.unpartitioned())
+ .withPath("/path/to/data-b.parquet")
+ .withFileSizeInBytes(10)
+ .withRecordCount(2)
+ .build())
+ .commit();
+
+ ResourcePaths paths = ResourcePaths.forCatalogProperties(Maps.newHashMap());
+
+ Table refsTable = catalog.loadTable(TABLE);
+ refsTable.refresh();
+
+ // don't call snapshots() directly as that would cause to load all snapshots. Instead,
+ // make sure the snapshots field holds exactly 1 snapshot
+ assertThat(((BaseTable) refsTable).operations().current())
+ .extracting("snapshots")
+ .asInstanceOf(InstanceOfAssertFactories.list(Snapshot.class))
+ .hasSize(1);
+
+ assertThat(refsTable.currentSnapshot()).isEqualTo(table.currentSnapshot());
+
+ // verify that the table was loaded with the refs argument
+ verify(adapter, times(2))
+ .execute(
+ reqMatcher(HTTPMethod.GET, paths.table(TABLE), Map.of(), Map.of("snapshots", "refs")),
+ eq(LoadTableResponse.class),
+ any(),
+ any());
+
+ // verify that all snapshots are loaded when referenced
Review Comment:
Not entirely sure it has any added value to check this. Could be irrelevant for this change. What are we validating here?
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -947,7 +956,8 @@ public Table create() {
mutationHeaders,
tableFileIO(context, tableConf, response.credentials()),
response.tableMetadata(),
- endpoints);
+ endpoints,
+ ImmutableMap.of());
Review Comment:
no need for this change, calling the existing constructor would do the same for us,
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -1024,6 +1024,81 @@ public void testTableCredential(String oauth2ServerUri) {
oauth2ServerUri);
}
+ @Test
+ public void testTableRefreshLoadsRefsOnly() {
+ RESTCatalogAdapter adapter = Mockito.spy(new RESTCatalogAdapter(backendCatalog));
+
+ RESTCatalog catalog =
+ new RESTCatalog(SessionCatalog.SessionContext.createEmpty(), (config) -> adapter);
+ catalog.initialize(
+ "test",
+ ImmutableMap.of(
+ CatalogProperties.URI,
+ "ignored",
+ CatalogProperties.FILE_IO_IMPL,
+ "org.apache.iceberg.inmemory.InMemoryFileIO",
+ // default loading to refs only
+ RESTCatalogProperties.SNAPSHOT_LOADING_MODE,
+ SnapshotMode.REFS.name()));
+
+ if (requiresNamespaceCreate()) {
+ catalog.createNamespace(TABLE.namespace());
+ }
+
+ // Create a table with multiple snapshots
+ Table table = catalog.createTable(TABLE, SCHEMA);
+ table
+ .newFastAppend()
+ .appendFile(
+ DataFiles.builder(PartitionSpec.unpartitioned())
+ .withPath("/path/to/data-a.parquet")
+ .withFileSizeInBytes(10)
+ .withRecordCount(2)
+ .build())
+ .commit();
+
+ table
+ .newFastAppend()
+ .appendFile(
+ DataFiles.builder(PartitionSpec.unpartitioned())
+ .withPath("/path/to/data-b.parquet")
+ .withFileSizeInBytes(10)
+ .withRecordCount(2)
+ .build())
+ .commit();
+
+ ResourcePaths paths = ResourcePaths.forCatalogProperties(Maps.newHashMap());
Review Comment:
Is there a reason why `RESOURCE_PATHS` is not sufficient for this test?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Rest: Allow refresh() to load refs only metadata [iceberg]
Posted by "gabeiglio (via GitHub)" <gi...@apache.org>.
gabeiglio commented on code in PR #14166:
URL: https://github.com/apache/iceberg/pull/14166#discussion_r2373324859
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -456,7 +456,8 @@ public Table loadTable(SessionContext context, TableIdentifier identifier) {
Map::of,
tableFileIO(context, tableConf, response.credentials()),
tableMetadata,
- endpoints);
+ endpoints,
+ snapshotModeToParam(snapshotMode));
Review Comment:
I initially didn't change the other constructors since I wanted to keep the PR small, but now it seems like a good idea to extend this to all other calls. will make the change
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Rest: Allow refresh() to load refs only metadata [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14166:
URL: https://github.com/apache/iceberg/pull/14166#discussion_r2374897201
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -887,6 +887,103 @@ public void testTableCredential(String oauth2ServerUri) {
oauth2ServerUri);
}
+ @Test
+ public void testTableRefreshLoadsRefsOnly() {
+ RESTCatalogAdapter adapter = Mockito.spy(new RESTCatalogAdapter(backendCatalog));
+
+ RESTCatalog catalog =
+ new RESTCatalog(SessionCatalog.SessionContext.createEmpty(), (config) -> adapter);
+ catalog.initialize(
+ "test",
+ ImmutableMap.of(
+ CatalogProperties.URI,
+ "ignored",
+ CatalogProperties.FILE_IO_IMPL,
+ "org.apache.iceberg.inmemory.InMemoryFileIO",
+ // default loading to refs only
+ RESTCatalogProperties.SNAPSHOT_LOADING_MODE,
+ SnapshotMode.REFS.name()));
+
+ if (requiresNamespaceCreate()) {
+ catalog.createNamespace(TABLE.namespace());
+ }
+
+ // Create a table with multiple snapshots
+ Table table = catalog.createTable(TABLE, SCHEMA);
+ table
+ .newFastAppend()
+ .appendFile(
+ DataFiles.builder(PartitionSpec.unpartitioned())
+ .withPath("/path/to/data-a.parquet")
+ .withFileSizeInBytes(10)
+ .withRecordCount(2)
+ .build())
+ .commit();
+
+ table
+ .newFastAppend()
+ .appendFile(
+ DataFiles.builder(PartitionSpec.unpartitioned())
+ .withPath("/path/to/data-b.parquet")
+ .withFileSizeInBytes(10)
+ .withRecordCount(2)
+ .build())
+ .commit();
+
+ ResourcePaths paths = ResourcePaths.forCatalogProperties(Maps.newHashMap());
+
+ // Respond with only referenced snapshots
+ Answer<?> refsAnswer =
+ invocation -> {
+ LoadTableResponse originalResponse = (LoadTableResponse) invocation.callRealMethod();
+ TableMetadata refsMetadata =
+ TableMetadata.buildFrom(originalResponse.tableMetadata())
+ .suppressHistoricalSnapshots()
+ .build();
+
+ // don't call snapshots() directly as that would cause to load all snapshots. Instead,
+ // make sure the snapshots field holds exactly 1 snapshot
+ assertThat(refsMetadata)
+ .extracting("snapshots")
+ .asInstanceOf(InstanceOfAssertFactories.list(Snapshot.class))
+ .hasSize(1);
+
+ return LoadTableResponse.builder()
+ .withTableMetadata(refsMetadata)
+ .addAllConfig(originalResponse.config())
+ .build();
+ };
+
+ Mockito.doAnswer(refsAnswer)
+ .when(adapter)
+ .execute(
+ reqMatcher(HTTPMethod.GET, paths.table(TABLE), Map.of(), Map.of("snapshots", "refs")),
+ eq(LoadTableResponse.class),
+ any(),
+ any());
+
+ Table refsTable = catalog.loadTable(TABLE);
+ refsTable.refresh();
Review Comment:
> or in other works in case current.snapshotsLoaded is false
I actually didn't mean to refer to this flag and we don't want to expose this, since this is internal information. What I meant is that it's not helpful to have ref loading in a place when the calling code potentially triggers an implicit call of `snapshots()`, which would do another request to load the remaining snapshots. That also means if there are cases where `snapshots()` isn't triggered and the table has lots and lots of snapshots, then this would be a good use case to think about adding ref loading mode there.
I don't want us to add this feature to all places just for the sake of adding it but rather we want to add it where it's actually helpful and makes a difference in terms of loading times of a table
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org