danielcweeks commented on code in PR #17642:
URL: https://github.com/apache/iceberg/pull/17642#discussion_r4543305079


##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -1039,6 +1039,11 @@ paths:
         Load a table from the catalog.
 
 
+        This endpoint must not return a format version 4 table.
+        A server must not serve a format version 4 table through this 
endpoint, so that a client that does not support format version 4 is never 
handed a response it cannot read.
+        Format version 4 tables are loaded through the `/v2` endpoint instead.

Review Comment:
    The first two sentences also say the same thing, and the text doesn't say 
what a server should return instead (400?).
   
   Can we remove these lines?
   
   ```suggestion
           Servers MUST NOT return format version 4 tables from this endpoint.
   ```
   
   <!-- lenses: weeks-review, blue-review, russell-review -->



##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -1299,6 +1304,91 @@ paths:
         5XX:
           $ref: '#/components/responses/ServerErrorResponse'
 
+  /v2/{prefix}/namespaces/{namespace}/tables/{table}:
+    parameters:
+      - $ref: '#/components/parameters/prefix'
+      - $ref: '#/components/parameters/namespace'
+      - $ref: '#/components/parameters/table'
+
+    get:
+      tags:
+        - Catalog API
+      summary: Load a table from the catalog
+      operationId: loadTableV2
+      description:
+        Load a table from the catalog.
+
+
+        This endpoint returns the same response as the v1 `loadTable` endpoint 
and may return a table at any format version, including format version 4.
+        Clients that support format version 4 should load tables through this 
endpoint.
+
+
+        The response contains both configuration and table metadata. The 
configuration, if non-empty is used
+        as additional configuration for the table that overrides catalog 
configuration. For example, this
+        configuration may change the FileIO implementation to be used for the 
table.
+
+
+        The response also contains the table's full metadata, matching the 
table metadata JSON file.
+
+
+        The catalog configuration may contain credentials that should be used 
for subsequent requests for the
+        table. The configuration key "token" is used to pass an access token 
to be used as a bearer token
+        for table requests. Otherwise, a token may be passed using a RFC 8693 
token type as a configuration
+        key. For example, "urn:ietf:params:oauth:token-type:jwt=<JWT-token>".
+      parameters:
+        - $ref: '#/components/parameters/data-access'
+        - name: If-None-Match
+          in: header
+          description:
+            An optional header that allows the server to return 304 (Not 
Modified) if the metadata
+            is current. The content is the value of the ETag received in a 
CreateTableResponse,
+            LoadTableResponse or CommitTableResponse.
+          required: false
+          schema:
+            type: string
+        - in: query
+          name: snapshots
+          description:
+            The snapshots to return in the body of the metadata via the 
`snapshots` field. Setting
+            the value to `all` would return the full set of snapshots 
currently valid for the table.
+            Setting the value to `refs` would load all snapshots referenced by 
branches or tags.
+

Review Comment:
   nit: if this endpoint stays, the `If-None-Match` and `snapshots` parameters 
and the description block are copied from v1 and will drift as soon as someone 
edits the v1 text. Can we define the parameters once in 
`components/parameters`, `$ref` them from both load operations, and point to 
the v1 description instead of repeating it?
   
   This should be a compatible change even if we touch the older v1 endpoints.
   
   <!-- lenses: weeks-review, blue-review -->



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

Reply via email to