steveloughran commented on code in PR #15831:
URL: https://github.com/apache/iceberg/pull/15831#discussion_r3021770646
##########
core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java:
##########
@@ -738,6 +744,64 @@ public static LoadViewResponse loadView(ViewCatalog
catalog, TableIdentifier vie
return viewResponse(view);
}
+ public static LoadRelationResponse loadRelation(
+ Catalog catalog, ViewCatalog viewCatalog, TableIdentifier ident,
SnapshotMode mode) {
+ try {
+ LoadTableResponse tableResponse = loadTable(catalog, ident, mode);
+ return
LoadRelationResponse.builder().withTableResponse(tableResponse).build();
+ } catch (NoSuchTableException e) {
+ if (viewCatalog != null) {
+ try {
+ LoadViewResponse viewResponse = loadView(viewCatalog, ident);
+ return
LoadRelationResponse.builder().withViewResponse(viewResponse).build();
+ } catch (NoSuchViewException ignored) {
+ // fall through to throw not-found
+ }
+ }
+ }
+
+ throw new NoSuchTableException("No table or view exists for identifier:
%s", ident);
+ }
+
+ public static BatchLoadRelationsResponse batchLoadRelations(
+ Catalog catalog, ViewCatalog viewCatalog, BatchLoadRelationsRequest
request) {
+ BatchLoadRelationsResponse.Builder responseBuilder =
BatchLoadRelationsResponse.builder();
+
+ for (BatchLoadRelationRequestItem item : request.identifiers()) {
+ TableIdentifier ident = item.identifier();
+ SnapshotMode mode = snapshotModeFromString(item.snapshots());
+
+ try {
+ LoadRelationResponse result = loadRelation(catalog, viewCatalog,
ident, mode);
+ BatchLoadRelationResultItem.Builder itemBuilder =
+ BatchLoadRelationResultItem.builder()
+ .withIdentifier(ident)
+ .withStatus(200)
+ .withResult(result);
+
+ if (result.objectType() == CatalogObjectType.TABLE
+ && result.tableResponse().metadataLocation() != null) {
+ itemBuilder.withEtag(result.tableResponse().metadataLocation());
+ }
+
+ responseBuilder.addResult(itemBuilder.build());
+ } catch (NoSuchTableException | NoSuchViewException e) {
+ responseBuilder.addResult(
+
BatchLoadRelationResultItem.builder().withIdentifier(ident).withStatus(404).build());
Review Comment:
would be good to provide an explicit text/plain message for the humans here
so that other causes of a 404 can be eliminated.
there's always a tradeoff here w.r.t leaking internal service state and
legitimate diagnostics, but I do think there's a good case here for some text
##########
core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java:
##########
@@ -213,6 +217,27 @@ public void accept(ErrorResponse error) {
}
}
+ /** Relation level error handler (for universal load that resolves to table
or view). */
+ private static class RelationErrorHandler extends DefaultErrorHandler {
+ private static final ErrorHandler INSTANCE = new RelationErrorHandler();
+
+ @Override
+ public void accept(ErrorResponse error) {
+ switch (error.code()) {
+ case 404:
+ if
(NoSuchNamespaceException.class.getSimpleName().equals(error.type())) {
Review Comment:
and here there's code which appears to be looking into the error for
classnames, which is exactly what they shouldn't be doing if it's running
client side.
I think it'd be better to have a file defining those error type strings as
part of the public API, give them implementation independent names
""BadRequest" over "BadRequestException" etc, and refer to them client and
server. Third-party catalog impls are going to have to be generating responses
which match.
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -1471,6 +1478,113 @@ public View loadView(SessionContext context,
TableIdentifier identifier) {
return new BaseView(ops, ViewUtil.fullViewName(name(), identifier));
}
+ private LoadRelationResponse loadRelationInternal(
+ SessionContext context,
+ TableIdentifier identifier,
+ Map<String, String> headers,
+ Consumer<Map<String, String>> responseHeaders) {
+ Endpoint.check(endpoints, Endpoint.V1_LOAD_RELATION);
+ AuthSession contextualSession = authManager.contextualSession(context,
catalogAuth);
+ return client
+ .withAuthSession(contextualSession)
+ .get(
+ paths.relation(identifier),
+ ImmutableMap.of(),
+ LoadRelationResponse.class,
+ headers,
+ ErrorHandlers.relationErrorHandler(),
+ responseHeaders);
+ }
+
+ /**
+ * Load a relation (table or view) using the universal load endpoint and
construct the
+ * corresponding {@link Table} or {@link View} object.
+ */
+ public Relation loadRelation(SessionContext context, TableIdentifier
identifier) {
+ Map<String, String> responseHeaders = Maps.newHashMap();
+ TableWithETag cachedTable = tableCache.getIfPresent(context.sessionId(),
identifier);
+
+ LoadRelationResponse response =
+ loadRelationInternal(
+ context, identifier, headersForLoadTable(cachedTable),
responseHeaders::putAll);
+
+ if (response == null) {
+ Preconditions.checkNotNull(cachedTable, "Invalid load relation response:
null");
+ return Relation.forTable(identifier, cachedTable.supplier().get());
+ }
+
+ if (response.objectType() == CatalogObjectType.TABLE) {
+ return buildTableRelation(context, identifier, response.tableResponse(),
responseHeaders);
+ } else {
+ return buildViewRelation(context, identifier, response.viewResponse());
+ }
+ }
+
+ private BatchLoadRelationsResponse batchLoadRelationsInternal(
+ SessionContext context, BatchLoadRelationsRequest request) {
+ Endpoint.check(endpoints, Endpoint.V1_BATCH_LOAD_RELATIONS);
+ AuthSession contextualSession = authManager.contextualSession(context,
catalogAuth);
+ return client
+ .withAuthSession(contextualSession)
+ .post(
+ paths.batchLoadRelations(),
+ request,
+ BatchLoadRelationsResponse.class,
+ Map.of(),
+ ErrorHandlers.relationErrorHandler());
+ }
+
+ /**
+ * Batch load relations (tables and views) using the batch load endpoint.
Constructs {@link Table}
+ * and {@link View} objects for each result. Not-found identifiers (404) are
skipped.
+ */
+ public List<Relation> loadRelations(SessionContext context,
Set<TableIdentifier> identifiers) {
+ BatchLoadRelationsRequest.Builder requestBuilder =
BatchLoadRelationsRequest.builder();
+ for (TableIdentifier ident : identifiers) {
+ BatchLoadRelationRequestItem.Builder itemBuilder =
+ BatchLoadRelationRequestItem.builder().withIdentifier(ident);
+ TableWithETag cached = tableCache.getIfPresent(context.sessionId(),
ident);
+ if (cached != null) {
+ itemBuilder.withEtag(cached.eTag());
+ }
+
+ requestBuilder.addIdentifier(itemBuilder.build());
+ }
+
+ BatchLoadRelationsResponse response =
+ batchLoadRelationsInternal(context, requestBuilder.build());
+
+ List<Relation> relations = Lists.newArrayList();
+ for (BatchLoadRelationResultItem item : response.results()) {
+ TableIdentifier ident = item.identifier();
+ switch (item.status()) {
+ case 200:
Review Comment:
very subjective, but I'm a personal advocate of putting all refs to the http
error codes in its own reference class too, so you can then use the ide to see
where "SC_200" is picked up without having to search for the number.
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -1471,6 +1478,113 @@ public View loadView(SessionContext context,
TableIdentifier identifier) {
return new BaseView(ops, ViewUtil.fullViewName(name(), identifier));
}
+ private LoadRelationResponse loadRelationInternal(
+ SessionContext context,
+ TableIdentifier identifier,
+ Map<String, String> headers,
+ Consumer<Map<String, String>> responseHeaders) {
+ Endpoint.check(endpoints, Endpoint.V1_LOAD_RELATION);
+ AuthSession contextualSession = authManager.contextualSession(context,
catalogAuth);
+ return client
+ .withAuthSession(contextualSession)
+ .get(
+ paths.relation(identifier),
+ ImmutableMap.of(),
+ LoadRelationResponse.class,
+ headers,
+ ErrorHandlers.relationErrorHandler(),
+ responseHeaders);
+ }
+
+ /**
+ * Load a relation (table or view) using the universal load endpoint and
construct the
+ * corresponding {@link Table} or {@link View} object.
+ */
+ public Relation loadRelation(SessionContext context, TableIdentifier
identifier) {
+ Map<String, String> responseHeaders = Maps.newHashMap();
+ TableWithETag cachedTable = tableCache.getIfPresent(context.sessionId(),
identifier);
+
+ LoadRelationResponse response =
+ loadRelationInternal(
+ context, identifier, headersForLoadTable(cachedTable),
responseHeaders::putAll);
+
+ if (response == null) {
+ Preconditions.checkNotNull(cachedTable, "Invalid load relation response:
null");
+ return Relation.forTable(identifier, cachedTable.supplier().get());
+ }
+
+ if (response.objectType() == CatalogObjectType.TABLE) {
+ return buildTableRelation(context, identifier, response.tableResponse(),
responseHeaders);
+ } else {
+ return buildViewRelation(context, identifier, response.viewResponse());
+ }
+ }
+
+ private BatchLoadRelationsResponse batchLoadRelationsInternal(
+ SessionContext context, BatchLoadRelationsRequest request) {
+ Endpoint.check(endpoints, Endpoint.V1_BATCH_LOAD_RELATIONS);
+ AuthSession contextualSession = authManager.contextualSession(context,
catalogAuth);
+ return client
+ .withAuthSession(contextualSession)
+ .post(
+ paths.batchLoadRelations(),
+ request,
+ BatchLoadRelationsResponse.class,
+ Map.of(),
+ ErrorHandlers.relationErrorHandler());
+ }
+
+ /**
+ * Batch load relations (tables and views) using the batch load endpoint.
Constructs {@link Table}
+ * and {@link View} objects for each result. Not-found identifiers (404) are
skipped.
+ */
+ public List<Relation> loadRelations(SessionContext context,
Set<TableIdentifier> identifiers) {
+ BatchLoadRelationsRequest.Builder requestBuilder =
BatchLoadRelationsRequest.builder();
+ for (TableIdentifier ident : identifiers) {
+ BatchLoadRelationRequestItem.Builder itemBuilder =
+ BatchLoadRelationRequestItem.builder().withIdentifier(ident);
+ TableWithETag cached = tableCache.getIfPresent(context.sessionId(),
ident);
+ if (cached != null) {
+ itemBuilder.withEtag(cached.eTag());
+ }
+
+ requestBuilder.addIdentifier(itemBuilder.build());
+ }
+
+ BatchLoadRelationsResponse response =
+ batchLoadRelationsInternal(context, requestBuilder.build());
+
+ List<Relation> relations = Lists.newArrayList();
+ for (BatchLoadRelationResultItem item : response.results()) {
+ TableIdentifier ident = item.identifier();
+ switch (item.status()) {
Review Comment:
java 17 switches?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
