add the process of snapshots queryParam for RESTCatalogAdapter #8519
Conversation
| return tableMetadata; | ||
| } | ||
|
|
||
| public static LoadTableResponse loadTable( |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@nastra , is it necessary to process snapshot mode in CatalogHandlers ? if yes, maybe we could add another interface.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
ok, I'll implement it on the server, thanks for your review @nastra
| RESTSessionCatalog.SnapshotMode snapshotMode = | ||
| RESTSessionCatalog.SnapshotMode.valueOf( | ||
| PropertyUtil.propertyAsString( | ||
| vars, "snapshots", RESTSessionCatalog.SnapshotMode.ALL.name())); |
There was a problem hiding this comment.
shouldn't this be called snapshot-loading-mode rather than snapshots?
There was a problem hiding this comment.
snapshot-loading-mode is the property name in RESTSessionCatalog to init snapshotMode. snapshots is the name of queryParams when REST client sending requests.
enum SnapshotMode {
ALL,
REFS;
// REST clients use it as queryParams
Map<String, String> params() {
return ImmutableMap.of("snapshots", this.name().toLowerCase(Locale.US));
}
}
| \ 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" |
There was a problem hiding this comment.
we don't want to break any existing APIs. See also https://github.com/apache/iceberg/blob/master/CONTRIBUTING.md#semantic-versioning
#6850 add lazy snapshot loading for RESTCatalog, but RESTCatalogAdapter doesn't process the 'snapshots' queryParam.