Re: [PR] Core: Add Java reference implementation for relation load endpoints [iceberg]

["steveloughran (via GitHub)" <gi...@apache.org>]

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: 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