-
Notifications
You must be signed in to change notification settings - Fork 402
Add iceberg endpoints #9615
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add iceberg endpoints #9615
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - added comment related to namespace type.
api/swagger.yml
Outdated
| namespace: | ||
| type: string | ||
| description: Remote table namespace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relevant to other places in this file - namespace is '[]string', there are places and we can represent it as 'string', suggest to add in the description or by example if we are using '.' as separator between level or the character 0x1f (Unit Separator).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aligned with iceberg spec behavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @N-o-Z -
Shouldn't we add experimental to these endpoints?
pkg/api/controller.go
Outdated
|
|
||
| func (c *Controller) PullIcebergTable(w http.ResponseWriter, r *http.Request, body apigen.PullIcebergTableJSONRequestBody, catalog string) { | ||
| // TODO implement me | ||
| panic("implement me") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| panic("implement me") | |
| writeError(w, r, http.StatusNotImplemented, "Not implemented") |
pkg/api/controller.go
Outdated
|
|
||
| func (c *Controller) PushIcebergTable(w http.ResponseWriter, r *http.Request, body apigen.PushIcebergTableJSONRequestBody, catalog string) { | ||
| // TODO implement me | ||
| panic("implement me") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| panic("implement me") | |
| writeError(w, r, http.StatusNotImplemented, "Not implemented") |
| description: Remote table namespace | ||
| table: | ||
| type: string | ||
| description: Remote table name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here - maybe add table name limitations here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure which limitations you're referring to.
Did you mean to provide a regex? If so I'm not sure it's necessary - especially since we don't control the Iceberg spec which can change at any given time
Experimental is usually added to functionality we are not sure we plan to deliver and want to leave the possibility of it removed open, or - when the functionality is not yet properly defined, unstable or is prone to changes. |
Add iceberg endpoints to lakeFS swagger