feat(catalog): add initial rest catalog impl #58

zeroshade · 2024-01-31T19:14:15Z

Adding an initial implementation and unit tests for the Rest catalog.

zeroshade · 2024-01-31T19:14:36Z

CC @Fokko @wolfeidau @nastra @HonahX @jackye1995

wolfeidau

Great effort getting this in, just some small suggestions.

wolfeidau · 2024-02-01T11:17:04Z

catalog/catalog.go

@@ -47,19 +52,136 @@ func WithAwsConfig(cfg aws.Config) Option {
 	}
 }

+func WithCredential(cred string) Option {


It feels like these options are being overloaded with probably only minimal overlap, do you think we should move these catalog implementations into their own packages, each with their own options?

I got a feeling this would happen with "common options", interested in your thoughts.

Yea I was thinking the same. Though my question is how many of these options will overlap with the other catalog implementations. Several of them probably could overlap. (This is probably why all the options in pyiceberg are all passed via property mappings)

@zeroshade yeah I think it is important to remember that a little duplication is OK, as long as the user of the API gets a neat typed interface.

Sure internally there maybe some overlap, but that typically erodes over time as corner cases evolve for each implementation.

As a user of the API I am using Glue + S3, am i likely to switch to hive + REST?

Personally I prefer a typed interface in Go APIs, over a bag of properties.

@wolfeidau take a look at the way I've set up some interesting little generic stuff in the most recent commit making the Options typed so that we can specify options which are explicit for one catalog or the other without having to do a ton of duplication. Let me know what you think.

wolfeidau · 2024-02-01T11:23:20Z

catalog/glue.go

@@ -91,7 +92,7 @@ func (c *GlueCatalog) ListTables(ctx context.Context, namespace table.Identifier
 // LoadTable loads a table from the catalog table details.
 //
 // The identifier should contain the Glue database name, then glue table name.
-func (c *GlueCatalog) LoadTable(ctx context.Context, identifier table.Identifier, props map[string]string) (*table.Table, error) {
+func (c *GlueCatalog) LoadTable(ctx context.Context, identifier table.Identifier, props iceberg.Properties) (*table.Table, error) {
 	database, tableName, err := identifierToGlueTable(identifier)


we need a better name than identifierToGlueTable eh.

nastra · 2024-02-12T13:50:16Z

dev/Dockerfile

@@ -0,0 +1,66 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more


where is this dockerfile being used?

It's used by the spark-iceberg container in the docker-compose.yml file. It uses build: . to tell it to use this Dockerfile

ah I missed that. In that case we should rename the image name, because I was assuming it's using tabulario/spark-iceberg. Also do we even need the image customization at this point?

honestly i was just following the configuration that iceberg-python is using (the Dockerfile is from that repo) I'll see if it works without the customization and report back.

nastra · 2024-02-12T13:52:52Z

cmd/iceberg/main.go

+const usage = `iceberg.
+
+Usage:
+  iceberg list [options] [PARENT]


it would be good to have proper help text, similar to https://py.iceberg.apache.org/cli/ but this can be done in a follow-up PR

If you use -h or --help it'll print out this whole usage string

Yes I saw that, but

Usage: iceberg list [options] [PARENT] iceberg describe [options] [namespace | table] IDENTIFIER iceberg (schema | spec | uuid | location) [options] TABLE_ID iceberg drop [options] (namespace | table) IDENTIFIER iceberg files [options] TABLE_ID [--history] iceberg rename [options] <from> <to> iceberg properties [options] get (namespace | table) IDENTIFIER [PROPNAME] iceberg properties [options] set (namespace | table) IDENTIFIER PROPNAME VALUE iceberg properties [options] remove (namespace | table) IDENTIFIER PROPNAME iceberg -h | --help | --version Arguments: PARENT Catalog parent namespace IDENTIFIER fully qualified namespace or table TABLE_ID full path to a table PROPNAME name of a property VALUE value to set Options: -h --help show this helpe messages and exit --catalog TEXT specify the catalog type [default: rest] --uri TEXT specify the catalog URI --output TYPE output type (json/text) [default: text] --credential TEXT specify credentials for the catalog

isn't super informative. What I meant is having some sort of better description of the different commands, similar to

Commands: describe Describes a namespace xor table drop Operations to drop a namespace or table list Lists tables or namespaces location Returns the location of the table properties Properties on tables/namespaces rename Renames a table schema Gets the schema of the table spec Returns the partition spec of the table uuid Returns the UUID of the table

Again, this isn't in the scope of this PR and would be nice to improve eventually

cmd/iceberg/main.go

nastra · 2024-02-12T14:19:36Z

catalog/rest_test.go

+		})
+	})
+
+	cat, err := catalog.NewRestCatalog("rest", r.srv.URL,


I believe this needs to make sure that the correct authorization header is set, similar to https://github.com/apache/iceberg-python/blob/main/tests/catalog/test_rest.py#L116

It's verified in the TestListTablesPrefixed200 test, it uses the token check and then makes a request afterwards which confirms the authorization header is sent with the subsequent request.

given that we're testing a different endpoint here, I would say it's better to also make sure that the correct Authorization header is set

nastra · 2024-02-12T14:21:05Z

catalog/rest_test.go

+
+	authUri, err := url.Parse(r.srv.URL)
+	r.Require().NoError(err)
+	cat, err := catalog.NewRestCatalog("rest", r.srv.URL,


same as above, this needs to verify the authorization header

Since we test the authorization header in the TestListTablesPrefixed200 I didn't think it necessary to add that same test in these. particularly given that we don't expose the client/session from the catalog object. I can add a test that isn't in the _test package so that I can verify the header though if you think we still need it.

we're calling a different endpoint here, so I think we should still make sure that the authorization header is properly configured

nastra · 2024-02-12T14:26:31Z

catalog/rest_test.go

+	cat, err := catalog.NewRestCatalog("rest", r.srv.URL, catalog.WithOAuthToken(TestToken))
+	r.Require().NoError(err)
+
+	r.Require().NoError(cat.CreateNamespace(context.Background(), catalog.ToRestIdentifier("leden"), iceberg.Properties{"foo": "bar", "super": "duper"}))


can we also verify that the properties have actually be set on the namespace? Just verifying that there wasn't an error could be misleading and stuff could just silently pass

This is all mocked in this test, there isn't any namespace to verify properties for. The r.Equal validations inside the handler verifies that the request was sent correctly and contained the properties as expected (see line 428)

catalog/rest_test.go

nastra · 2024-02-13T07:47:38Z

catalog/catalog.go

+	tlsConfig         *tls.Config
+	credential        string
+	oauthToken        string
+	warehouseLocation string


how can these be configured from the cmd line? I would assume we'd have a config file similar to what pyiceberg has?

currently only credential is configurable on the CLI.

I can add the warehouse and token easily as options. Unless we think that the config file would be better in general

I think a config file makes more sense, because then configs can be specifig to catalogs. Otherwise you'd have to maintain all the available config options as CLI options

nastra · 2024-02-13T09:00:26Z

cmd/iceberg/main.go

+		output.Text("Renamed table from " + cfg.RenameFrom + " to " + cfg.RenameTo)
+	case cfg.Drop:
+		switch {
+		case cfg.Namespace:


given that we can drop a namespace, should there be an option to create a namespace? This can be done in a follow-up

nastra · 2024-02-13T09:40:27Z

I think what's currently missing is having a way to configure the warehouse (which I hardcoded for testing) but also handling the signing part of requests against S3, similar to https://github.com/apache/iceberg-python/blob/f66e3652fdf9720d6c63a6fcec7bcd08d5bb186c/pyiceberg/io/fsspec.py#L70-L95

Listing files via go run ./cmd/iceberg files iceberg124.foobar --catalog rest --uri https://api.dev.tabular.io/ws/ --credential <creds> will fail with

2024/02/13 10:24:07 could not open manifest file: operation error S3: GetObject, https response error StatusCode: 403, RequestID: 066G7WZD23KHZCBJ, HostID: d4V0iCd2uzvp9gZJWDOWmljaREgSaL9Iro0XxOFsv38ECJpdCd/JHWG8Y6/i7oSal8cONZ87Tis=, api error AccessDenied: Access Denied
exit status 1

I believe this is because FileIO isn't configured with the TOKEN in the authorization header that's coming back from the config inside tblResponse here. Reading all other metadata of tables work via CLI, but this is because those never use FileIO and only files does that atm.

zeroshade · 2024-02-13T15:37:53Z

@nastra

Hmm. So, setting the env vars AWS_REGION, AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY should all work and get picked up by the FileIO. But I haven't tried testing with https://api.dev.tabular.io/ws/ before.

There is the ability to set a session token via the s3.session-token property but you're right that I don't think it gets propagated. Is there any special configuration I need to set up in order to try testing out the api.dev.tabular.io/ws/ uri myself?

zeroshade · 2024-02-13T17:59:56Z

@nastra So I've figured out the issue:

The properties are correctly being propagated to the FileIO object, however it looks like the tabular api doesn't like the Go Iceberg user-agent.

I loaded up pyiceberg to see what it does differently and how it works, and saw that the request for the table included in its response a series of s3 properties including an access-key-id, session-token, and secret-access-key in the config. When I looked at the same request from the Go cli those properties weren't there. If I hardcode and change the User-Agent that the Go CLI passes to be PyIceberg/0.5.1 suddenly those properties are returned and loading the manifests works just fine. So the problem is definitely the fact that the User-Agent isn't recognized by the tabular rest catalog enough for it to send the s3 key properties.

Anything we can do on the tabular side?
During RestCatalog.LoadTable

catalog/catalog.go

dev/spark-defaults.conf

dev/run-minio.sh

dev/provision.py

dev/entrypoint.sh

nastra

LGTM once the files have been removed that were previously used in the custom Dockerfile.

@zeroshade could you also please open new issues for:

improving help text
config handling
implementing remaining catalog operations for REST / Glue / ...
... (whatever else you think needs to be improved/done)

Having open issues increases the visibility and would also give other people in the community the chance to contribute by seeing what work needs to be done

zeroshade · 2024-02-14T16:42:43Z

@nastra Added several issues as suggested

feat(catalog): add initial rest catalog impl

ebce777

bump to go 1.21 and 1.22

59fe743

github-actions bot added the INFRA label Jan 31, 2024

just go 1.21 to avoid extra package

fb2df75

wolfeidau suggested changes Feb 1, 2024

View reviewed changes

zeroshade added 5 commits February 9, 2024 13:11

add small CLI and catalog integration example

e530020

forgot the license

22aed86

go mod tidy

3637d88

oops left in a bad test

6e90b60

fix test

0f4b029

nastra reviewed Feb 12, 2024

View reviewed changes

cmd/iceberg/main.go Outdated Show resolved Hide resolved

nastra reviewed Feb 12, 2024

View reviewed changes

catalog/rest_test.go Outdated Show resolved Hide resolved

zeroshade added 2 commits February 12, 2024 11:02

add credential to cli

296e58d

remove extraneous config

49748de

nastra reviewed Feb 13, 2024

View reviewed changes

zeroshade added 5 commits February 13, 2024 13:00

updates from feedback and adding user-agent

77e6c5f

fix tests

ac72a4f

add requested tests

ca215b7

remove unnecessary dockerfile

7618bd0

check "client.region" prop

4919648

zeroshade added 3 commits February 13, 2024 13:37

change user-agent back

849e205

put user-agents back

1391f3b

specialize options via generics

254c6a3