feat(catalog): Add pagination for list table operation across different catalog types #306
Conversation
Signed-off-by: dttung2905 <[email protected]>
zeroshade
changed the title
catalog/glue/glue.go
Outdated
| } | ||
| } | ||
|
|
||
| func (c *Catalog) ListTablesPage(ctx context.Context, namespace table.Identifier) ([]table.Identifier, *string, error) { |
There was a problem hiding this comment.
Any particular reason to export this?
Also I'm not seeing how the next page token is being passed back to it for it to be used?
In addition, any reason to use a pointer to the string rather than just using an empty string to indicate there are no further pages?
There was a problem hiding this comment.
Thanks for the quick review. I really appreciate it. 🙏
Any particular reason to export this?
No just typo on my side. It should be private method
Also I'm not seeing how the next page token is being passed back to it for it to be used?
Cmiiw, from the current code, the c.glueSvc.GetTables does not use any next page token. Mainly I think it is meant for checking the end of the page in ListTables
catalog/glue/glue_test.go
Outdated
| @@ -20,6 +20,7 @@ package glue | |||
| import ( | |||
| "context" | |||
| "errors" | |||
| "github.com/apache/iceberg-go/table" | |||
There was a problem hiding this comment.
Please use gofmt to reorder the imports. We should probably add a linter step to identify and fix this....
There was a problem hiding this comment.
Let me handle that linter step. I will do some time later this week
There was a problem hiding this comment.
Might be tmr if I can squeeze in some time to do it 🙏
catalog/rest/rest.go
Outdated
|
|
||
| func (r *Catalog) ListTablesPage(ctx context.Context, namespace table.Identifier, pageToken string, pageSize int) ([]table.Identifier, string, error) { |
There was a problem hiding this comment.
Same question here, is there any reason to export this?
catalog/rest/rest_test.go
Outdated
| TestToken = "some_jwt_token" | ||
| TestCreds = "client:secret" | ||
| TestToken = "some_jwt_token" | ||
| defaultPageToken = 20 |
There was a problem hiding this comment.
Wouldn't this be defaultPageSize?
| tbls := make([]table.Identifier, 0) | ||
| iter := c.ListTables(ctx, namespace) | ||
|
|
||
| for tbl, err := range iter { |
There was a problem hiding this comment.
No need to gather all the tables, as so as you get 1 table you can break and return the error below
There was a problem hiding this comment.
Nice catch. Thank you for pointing it out 🙏
catalog/sql/sql.go
Outdated
| } | ||
| } | ||
|
|
||
| func (c *Catalog) ListTablesAll(ctx context.Context, namespace table.Identifier) ([]table.Identifier, error) { |
There was a problem hiding this comment.
Same question as before, why export this?
Signed-off-by: dttung2905 <[email protected]>
catalog/glue/glue.go
Outdated
| func (c *Catalog) listTablesPage(ctx context.Context, namespace table.Identifier) ([]table.Identifier, string, error) { | ||
| database, err := identifierToGlueDatabase(namespace) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| return nil, "", err | ||
| } | ||
| params := &glue.GetTablesInput{CatalogId: c.catalogId, DatabaseName: aws.String(database)} | ||
| tblsRes, err := c.glueSvc.GetTables(ctx, params) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("failed to list tables in namespace %s: %w", database, err) | ||
| return nil, "", fmt.Errorf("failed to list tables in namespace %s: %w", database, err) | ||
| } | ||
| var icebergTables []table.Identifier | ||
| icebergTables = append(icebergTables, | ||
| filterTableListByType(database, tblsRes.TableList, glueTypeIceberg)...) | ||
|
|
||
| return icebergTables, tblsRes.NextToken, nil | ||
| var nextToken string | ||
| if tblsRes.NextToken != nil { | ||
| nextToken = *tblsRes.NextToken | ||
| } | ||
| return icebergTables, nextToken, nil | ||
| } |
There was a problem hiding this comment.
looking at the docs for GetTablesInput it looks like the nextToken should be being passed back to listTablesPage and put into the GetTablesInput.
Personally, it might make more sense to just use NewGetTablesPaginator instead, rather than implementing the pagination ourselves 😄
There was a problem hiding this comment.
Hi @zeroshade ,
Nice. That's something that I have not known about. Thanks for pointing it out. I have tried to create a different version with NewGetTablesPaginator and creating few extra test cases in glue catalog to verify its functionality 🙏
Signed-off-by: dttung2905 <[email protected]>
Goal
To support pagination for ListTables method. Similar to
ListViewsoperation in PR #290. We need to change the method interface incatalog.go, cascading changes intoglue.goandsql.go. I'm not sure about the pagination insql.goso I write a wrapper to convert the existing function to iter.Seq2[] typeTODO:
ListTablesmethod insql.go