Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add the process of snapshots queryParam for RESTCatalogAdapter #8519

Closed
wants to merge 1 commit into from
Closed

add the process of snapshots queryParam for RESTCatalogAdapter #8519

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .palantir/revapi.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -829,6 +829,12 @@ acceptedBreaks:
- code: "java.class.removed"
old: "interface org.apache.iceberg.actions.RewritePositionDeleteStrategy"
justification: "Removing deprecated code"
- code: "java.method.numberOfParametersChanged"
old: "method org.apache.iceberg.rest.responses.LoadTableResponse org.apache.iceberg.rest.CatalogHandlers::loadTable(org.apache.iceberg.catalog.Catalog,\
\ org.apache.iceberg.catalog.TableIdentifier)"
new: "method org.apache.iceberg.rest.responses.LoadTableResponse org.apache.iceberg.rest.CatalogHandlers::loadTable(org.apache.iceberg.catalog.Catalog,\
\ org.apache.iceberg.catalog.TableIdentifier, org.apache.iceberg.rest.RESTSessionCatalog.SnapshotMode)"
justification: "add snapshot mode when load Table"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't want to break any existing APIs. See also https://github.com/apache/iceberg/blob/master/CONTRIBUTING.md#semantic-versioning

- code: "java.method.removed"
old: "method java.util.List<org.apache.iceberg.DataFile> org.apache.iceberg.MergingSnapshotProducer<ThisT>::addedFiles()\
\ @ org.apache.iceberg.BaseOverwriteFiles"
Expand Down
27 changes: 25 additions & 2 deletions core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import org.apache.iceberg.BaseTransaction;
import org.apache.iceberg.PartitionSpec;
import org.apache.iceberg.Schema;
import org.apache.iceberg.SnapshotRef;
import org.apache.iceberg.SortOrder;
import org.apache.iceberg.Table;
import org.apache.iceberg.TableMetadata;
Expand Down Expand Up @@ -253,12 +254,34 @@ public static void purgeTable(Catalog catalog, TableIdentifier ident) {
}
}

public static LoadTableResponse loadTable(Catalog catalog, TableIdentifier ident) {
private static TableMetadata getTableMetaData(
BaseTable baseTable, RESTSessionCatalog.SnapshotMode snapshotMode) {
TableMetadata tableMetadata = baseTable.operations().current();

switch (snapshotMode) {
case ALL:
return tableMetadata;
case REFS:
{
TableMetadata refsMetaData = TableMetadata.buildFrom(tableMetadata).build();
Set<Long> referencedSnapshotIds =
refsMetaData.refs().values().stream()
.map(SnapshotRef::snapshotId)
.collect(Collectors.toSet());
refsMetaData.removeSnapshotsIf(s -> !referencedSnapshotIds.contains(s.snapshotId()));
return refsMetaData;
}
}
return tableMetadata;
}

public static LoadTableResponse loadTable(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can just change a public method as that will break API compatibility: https://github.com/apache/iceberg/blob/master/CONTRIBUTING.md#semantic-versioning

@sandflee what's the expectation on your end for this PR? Are you expecting a backing catalog (such as JDBC) to also return either a single snapshot vs all snapshots when the snapshot-loading-mode is set?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm building a Iceberg RESTCatalog server with the ability of CatalogHandlers, and found CatalogHandlers#loadTable doesn't support snapshot queryParams, so I post this PR.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nastra , is it necessary to process snapshot mode in CatalogHandlers ? if yes, maybe we could add another interface.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm building a Iceberg RESTCatalog server with the ability of CatalogHandlers, and found CatalogHandlers#loadTable doesn't support snapshot queryParams, so I post this PR.

In that case wouldn't it be better to implement this functionality on the server rather than in CatalogHandlers?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I'll implement it on the server, thanks for your review @nastra

Catalog catalog, TableIdentifier ident, RESTSessionCatalog.SnapshotMode snapshotMode) {
Table table = catalog.loadTable(ident);

if (table instanceof BaseTable) {
return LoadTableResponse.builder()
.withTableMetadata(((BaseTable) table).operations().current())
.withTableMetadata(getTableMetaData((BaseTable) table, snapshotMode))
.build();
} else if (table instanceof BaseMetadataTable) {
// metadata tables are loaded on the client side, return NoSuchTableException for now
Expand Down
9 changes: 8 additions & 1 deletion core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import java.io.IOException;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.function.Consumer;
import org.apache.iceberg.BaseTable;
Expand Down Expand Up @@ -347,8 +348,14 @@ public <T extends RESTResponse> T handleRequest(

case LOAD_TABLE:
{
RESTSessionCatalog.SnapshotMode snapshotMode =
RESTSessionCatalog.SnapshotMode.valueOf(
PropertyUtil.propertyAsString(
vars, "snapshots", RESTSessionCatalog.SnapshotMode.ALL.name())
.toUpperCase(Locale.ENGLISH));
TableIdentifier ident = identFromPathVars(vars);
return castResponse(responseType, CatalogHandlers.loadTable(catalog, ident));
return castResponse(
responseType, CatalogHandlers.loadTable(catalog, ident, snapshotMode));
}

case REGISTER_TABLE:
Expand Down