zeroshade commented on code in PR #306:
URL: https://github.com/apache/iceberg-go/pull/306#discussion_r1960199017
##########
catalog/glue/glue.go:
##########
@@ -155,33 +156,42 @@ func NewCatalog(opts ...Option) *Catalog {
// ListTables returns a list of Iceberg tables in the given Glue database.
//
// The namespace should just contain the Glue database name.
-func (c *Catalog) ListTables(ctx context.Context, namespace table.Identifier)
([]table.Identifier, error) {
+
+func (c *Catalog) ListTables(ctx context.Context, namespace table.Identifier)
iter.Seq2[table.Identifier, error] {
+ return func(yield func(table.Identifier, error) bool) {
+ for {
+ tbls, nextPageToken, err := c.ListTablesPage(ctx,
namespace)
+ if err != nil {
+ yield(table.Identifier{}, err)
+ return
+ }
+ for _, tbl := range tbls {
+ if !yield(tbl, nil) {
+ return
+ }
+ }
+ if nextPageToken == nil {
+ return
+ }
+ }
+ }
+}
+
+func (c *Catalog) ListTablesPage(ctx context.Context, namespace
table.Identifier) ([]table.Identifier, *string, error) {
Review 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?
##########
catalog/rest/rest_test.go:
##########
@@ -37,8 +37,9 @@ import (
)
const (
- TestCreds = "client:secret"
- TestToken = "some_jwt_token"
+ TestCreds = "client:secret"
+ TestToken = "some_jwt_token"
+ defaultPageToken = 20
Review Comment:
Wouldn't this be `defaultPageSize`?
##########
catalog/rest/rest.go:
##########
@@ -649,27 +649,60 @@ func (r *Catalog) tableFromResponse(ctx context.Context,
identifier []string, me
return table.New(id, metadata, loc, iofs), nil
}
-func (r *Catalog) ListTables(ctx context.Context, namespace table.Identifier)
([]table.Identifier, error) {
- if err := checkValidNamespace(namespace); err != nil {
- return nil, err
+func (r *Catalog) ListTables(ctx context.Context, namespace table.Identifier)
iter.Seq2[table.Identifier, error] {
+ return func(yield func(table.Identifier, error) bool) {
+ pageSize := r.getPageSize(ctx)
+ var pageToken string
+
+ for {
+ tables, nextPageToken, err := r.ListTablesPage(ctx,
namespace, pageToken, pageSize)
+ if err != nil {
+ yield(table.Identifier{}, err)
+ return
+ }
+ for _, tbl := range tables {
+ if !yield(tbl, nil) {
+ return
+ }
+ }
+ if nextPageToken == "" {
+ return
+ }
+ pageToken = nextPageToken
+ }
}
+}
+func (r *Catalog) ListTablesPage(ctx context.Context, namespace
table.Identifier, pageToken string, pageSize int) ([]table.Identifier, string,
error) {
Review Comment:
Same question here, is there any reason to export this?
##########
catalog/sql/sql.go:
##########
@@ -524,9 +525,14 @@ func (c *Catalog) DropNamespace(ctx context.Context,
namespace table.Identifier)
return fmt.Errorf("%w: %s", catalog.ErrNoSuchNamespace,
nsToDelete)
}
- tbls, err := c.ListTables(ctx, namespace)
- if err != nil {
- return err
+ tbls := make([]table.Identifier, 0)
+ iter := c.ListTables(ctx, namespace)
+
+ for tbl, err := range iter {
Review Comment:
No need to gather all the tables, as so as you get 1 table you can break and
return the error below
##########
catalog/glue/glue_test.go:
##########
@@ -20,6 +20,7 @@ package glue
import (
"context"
"errors"
+ "github.com/apache/iceberg-go/table"
Review Comment:
Please use gofmt to reorder the imports. We should probably add a linter
step to identify and fix this....
##########
catalog/sql/sql.go:
##########
@@ -576,7 +582,23 @@ func (c *Catalog) LoadNamespaceProperties(ctx
context.Context, namespace table.I
})
}
-func (c *Catalog) ListTables(ctx context.Context, namespace table.Identifier)
([]table.Identifier, error) {
+func (c *Catalog) ListTables(ctx context.Context, namespace table.Identifier)
iter.Seq2[table.Identifier, error] {
+ tables, err := c.ListTablesAll(ctx, namespace)
+ if err != nil {
+ return func(yield func(table.Identifier, error) bool) {
+ yield(table.Identifier{}, err)
+ }
+ }
+ return func(yield func(table.Identifier, error) bool) {
+ for _, t := range tables {
+ if !yield(t, nil) {
+ return
+ }
+ }
+ }
+}
+
+func (c *Catalog) ListTablesAll(ctx context.Context, namespace
table.Identifier) ([]table.Identifier, error) {
Review Comment:
Same question as before, why export this?
--
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
