amogh-jahagirdar commented on code in PR #9695:
URL: https://github.com/apache/iceberg/pull/9695#discussion_r1545049249
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2838,6 +2991,76 @@ components:
additionalProperties:
type: string
+ PreplanTableRequest:
+ type: object
+ required:
+ - select
+ - filter
+ properties:
+ select:
+ description: A list of the selected column names
+ type: array
+ items:
+ type: string
+ filter:
+ $ref: '#/components/schemas/Expression'
+ case-sensitive:
+ description: Indicates whether column selection and filtering should
be case sensitive
+ type: boolean
+ default: true
+ snapshot-id:
+ description: an int64 snapshot ID (if snapshot-range is not
present); optional and defaults to the table's current snapshot
+ type: integer
+ format: int64
+ snapshot-range:
+ description: A JSON list containing exactly 2 snapshot IDs
representing the start (exclusive) and end (inclusive) snapshots. This option
is not allowed when `snapshot-id` is present in the request.
+ type: array
+ items:
+ type: integer
+ format: int64
+ oneOf:
+ - required: [snapshot-id]
+ - required: [snapshot-range]
+
+ PlanTableRequest:
+ type: object
+ required:
+ - select
+ properties:
+ select:
+ description: A list of the selected column names
+ type: array
+ items:
+ type: string
+ filter:
+ $ref: '#/components/schemas/Expression'
+ case-sensitive:
+ description: Indicates whether column selection and filtering should
be case sensitive
+ type: boolean
+ default: true
+ stats-fields:
+ description: A list of string field names for which stats should be
included
Review Comment:
Nit: Since the type is already defined in the spec (items:type:string below)
I don't think it's neccessary to say A list of string field names. I think we
could just say `A list of field names`
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3789,6 +4012,21 @@ components:
EmptyResponse:
$ref: '#/components/examples/ListNamespacesEmptyExample'
+ PayloadTooLargeWithPlanResponse:
+ description:
+ Payload is too large. Informs user to leverage /preplan before doing
/plan
Review Comment:
Should this be more direct in the spec? `This failure indicates the payload
is too large. If this is encountered a client should perform /preplan followed
by a /plan`
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3804,6 +4042,21 @@ components:
}
}
+ UnprocessableEntityResponse:
+ description:
+ Service can not process entity.
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/IcebergErrorResponse'
+ example: {
+ "error": {
+ "message": " Cannot plan scan: plan-task is required; use
/preplan to split planning into plan tasks",
Review Comment:
Nit: I think there's an unneccessary space before Cannot?
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3789,6 +4012,21 @@ components:
EmptyResponse:
$ref: '#/components/examples/ListNamespacesEmptyExample'
+ PayloadTooLargeWithPlanResponse:
+ description:
+ Payload is too large. Informs user to leverage /preplan before doing
/plan
Review Comment:
I take this back, we probably don't want to dictate what a client should do
here. I think either `informs` as is fine or just remove it and let the error
message indicate the suggestion
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -537,6 +537,113 @@ paths:
5XX:
$ref: '#/components/responses/ServerErrorResponse'
+ /v1/{prefix}/namespaces/{namespace}/tables/{table}/preplan:
Review Comment:
More fundamental question: Since we're going down a route of adding a REST
catalog defining which capabilities are supported, does every server which
supports planning also need to support preplanning? Or are these different
capabilities? @nastra @rdblue
IMO I think these should be separate capabilities. Pre-planning seems like
it would require a lot more state to maintain on the server and it wouldn't
make sense to me to lump the 2 capabilities because then the barrier for a REST
catalog to technically support planning is significantly higher.
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2838,6 +2991,76 @@ components:
additionalProperties:
type: string
+ PreplanTableRequest:
+ type: object
+ required:
+ - select
+ - filter
+ properties:
+ select:
+ description: A list of the selected column names
+ type: array
+ items:
+ type: string
+ filter:
+ $ref: '#/components/schemas/Expression'
+ case-sensitive:
+ description: Indicates whether column selection and filtering should
be case sensitive
+ type: boolean
+ default: true
+ snapshot-id:
+ description: an int64 snapshot ID (if snapshot-range is not
present); optional and defaults to the table's current snapshot
+ type: integer
+ format: int64
+ snapshot-range:
+ description: A JSON list containing exactly 2 snapshot IDs
representing the start (exclusive) and end (inclusive) snapshots. This option
is not allowed when `snapshot-id` is present in the request.
+ type: array
+ items:
+ type: integer
+ format: int64
+ oneOf:
+ - required: [snapshot-id]
+ - required: [snapshot-range]
+
+ PlanTableRequest:
+ type: object
+ required:
+ - select
+ properties:
+ select:
+ description: A list of the selected column names
+ type: array
+ items:
+ type: string
+ filter:
+ $ref: '#/components/schemas/Expression'
+ case-sensitive:
+ description: Indicates whether column selection and filtering should
be case sensitive
+ type: boolean
+ default: true
+ stats-fields:
+ description: A list of string field names for which stats should be
included
+ type: array
+ items:
+ type: string
+ plan-task:
+ $ref: '#/components/schemas/PlanTask'
+ snapshot-id:
+ description: an int64 snapshot ID (if snapshot-range is not
present); optional and defaults to the table's current snapshot. This option is
not allowed when 'plan-task` is present in the request.
Review Comment:
same nit as earlier, since the types are already defined I think it's a bit
redundant to say `an int64 snapshot ID`. Also I think this should say `This
option is not allowed when snapshot-range or plan-task is present in the
request`?
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3804,6 +4042,21 @@ components:
}
}
+ UnprocessableEntityResponse:
+ description:
+ Service can not process entity.
Review Comment:
Nit: `can not` -> `cannot` for consistency in the spec
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
