Python: First version of the rest catalog #5287

Fokko · 2022-07-15T15:54:12Z

No description provided.

…st-catalog

python/pyiceberg/catalog/rest.py

core/src/main/java/org/apache/iceberg/SchemaParser.java

rdblue · 2022-07-15T21:31:26Z

python/pyiceberg/catalog/base.py

-        partition_spec: PartitionSpec | None = None,
+        partition_spec: PartitionSpec = UNPARTITIONED_PARTITION_SPEC,
+        sort_order: SortOrder = UNSORTED_SORT_ORDER,
        properties: Properties | None = None,


Should we similarly default properties to {}?

I would like that, but that's actually a code smell in Python (or a quirk in the language). The default {} will be a reference to a single object, if you mutate that one, the next time the default value is being assigned, it will give the reference to the same object. More info here: https://florimond.dev/en/posts/2018/08/python-mutable-defaults-are-the-source-of-all-evil/ So the recommended way is just to set it to null, and then do properties or {} in the code. This {} will then always initialize an empty dict.

There's no immutable dict?

Not in the standard lib, there is a package, or we can implement it ourselves.

Maybe later, but this is fine for now.

python/tests/table/test_metadata.py

python/pyiceberg/table/metadata.py

rdblue · 2022-07-15T21:38:12Z

python/pyiceberg/table/base.py

-    """
-
-    identifier: str | Identifier
+class Table(IcebergBaseModel):


Does Table need to be serializable? I'm curious why Table extends IcebergBaseModel. It seems like it should be a regular class to me.

Yes, it needs to be serializable from/to json. It is the response of the REST API: https://github.com/apache/iceberg/blob/master/open-api/rest-catalog-open-api.yaml#L1481-L1503

The CreateTableResponse is an alias of the LoadTableResponse.

I don't think the LoadTableResponse should be the same class as Table. Table is part of the public API and will expose a lot of methods and operations to users. LoadTableResponse is specific to the REST catalog and includes fields that aren't part of the table (config).

This is defined by the Catalog:

iceberg/python/pyiceberg/catalog/base.py

Lines 54 to 77 in 8495141

@abstractmethod

def create_table(

self,

identifier: str | Identifier,

schema: Schema,

location: str | None = None,

partition_spec: PartitionSpec | None = None,

properties: Properties | None = None,

) -> Table:

"""Create a table

Args:

identifier: Table identifier.

schema: Table's schema.

location: Location for the table. Optional Argument.

partition_spec: PartitionSpec for the table. Optional Argument.

properties: Table properties that can be a string based dictionary. Optional Argument.

Returns:

Table: the created table instance

Raises:

AlreadyExistsError: If a table with the name already exists

"""

How about subclassing the table:

class RestTable(Table): config: Dict[str, str] = Field(default_factory=dict)

And moving the specific items to there.

I think we will want to evolve this in the future, but it is okay for now.

We will need to think about whether we want to use the same TableOperations abstraction that Java uses. That allows us to have a base table implementation that can work with any catalog. In that design, the table wraps a TableMetadata that is handled (refreshed, committed) by TableOperations.

python/pyiceberg/catalog/rest.py

…st-catalog

python/pyiceberg/table/metadata.py

python/tests/utils/test_bin_packing.py

…st-catalog

rdblue · 2022-07-27T19:20:24Z

python/pyiceberg/catalog/rest.py

+        identifier = self.identifier_to_tuple(identifier)
+        return {"namespace": NAMESPACE_SEPARATOR.join(identifier[:-1]), "table": identifier[-1]}
+
+    def _split_identifier_for_json(self, identifier: Union[str, Identifier]) -> Dict[str, Union[Tuple[str, ...], str]]:


Nit: The return type could be Dict[str, Union[str, Identifier]] instead of using Tuple[str, ...].

Nice one, thanks!

rdblue · 2022-07-27T19:24:46Z

python/pyiceberg/catalog/rest.py

+            self._handle_non_200_response(exc, {404: NoSuchNamespaceError})
+        response = ListTablesResponse(**response.json())
+
+        return [(self.name, *entry.namespace, entry.name) for entry in response.identifiers]


I don't think this should include self.name because we want to be able to pass any of the returned identifiers back into the catalog. The only place where we want to add the catalog name for context is when we create the Table.

Got it, I've reverted this change.

rdblue · 2022-07-27T19:29:53Z

python/pyiceberg/catalog/rest.py

+
+
+class TableResponse(IcebergBaseModel):
+    metadata_location: Optional[str] = Field(alias="metadata-location", default=None)


I see that this is optional in the spec, but I think we want to make it required in the spec. Otherwise, you wouldn't be able to tell when the metadata changes. The refresh call will call the load route again and update the metadata if it has changed.

Having no metadata-location doesn't make much sense to me:

This is from the spec:

The table metadata JSON is returned in the metadata field. The corresponding file location of table metadata should be returned in the metadata-location field, unless the metadata is not yet committed. For example, a create transaction may return metadata that is staged but not committed.

Also, the location is in the metadata itself, where it is a required field, so I don't see how this can be optional. I can create a PR to the spec if you like (but we could also omit it since it is part of the metadata).

rdblue · 2022-07-27T19:30:53Z

python/pyiceberg/catalog/rest.py

+        try:
+            response.raise_for_status()
+        except HTTPError as exc:
+            self._handle_non_200_response(exc, {404: NoSuchNamespaceError})


I think this should be NoSuchTableException. If you try to drop a table and get a 404, then the table doesn't exist.

https://github.com/apache/iceberg/blob/master/open-api/rest-catalog-open-api.yaml#L673

rdblue · 2022-07-27T19:32:51Z

python/pyiceberg/catalog/rest.py

+        try:
+            response.raise_for_status()
+        except HTTPError as exc:
+            self._handle_non_200_response(exc, {404: NoSuchNamespaceError, 409: TableAlreadyExistsError})


This should also be NoSuchTableError: https://github.com/apache/iceberg/blob/master/open-api/rest-catalog-open-api.yaml#L752-L753

Looks like the spec has an incorrect example there, but the 404 for this route means there was no such table to rename.

I'm confused. Here you suggested using NoSuchNamespaceError: #5287 (comment) Let me go over the spec and see which one to throw

Yeah, 404 is context-specific. My comment was for the list_tables method, where a 404 indicates that the namespace does not exist and can't be listed. That shouldn't raise NoSuchTableError because 404 doesn't mean the namespace was empty -- that would result in an empty list of identifiers -- 404 means that the namespace itself doesn't exist.

Sorry for the confusion, I know it's a bit odd to have the meaning of 404 change based on the route, but it makes a bit more sense when you think about routes as identifying resources. /namespace/ns/tables primarily identifies a namespace and is interacting with the table set of that namespace.

python/pyiceberg/table/base.py

python/tests/catalog/test_base.py

rdblue · 2022-07-30T18:48:01Z

python/pyiceberg/exceptions.py

 #  under the License.


-class NoSuchTableError(Exception):


I don't think that we want to change these errors, since they are independent of the REST catalog. I'd leave the existing ones unchanged and just make the REST-specific errors extend RESTError

I had the same concern, also others could be applicable outside of the REST contest. I'll move NoSuchNamespaceError and NoSuchTableError back to inherit from Exception.

rdblue · 2022-07-30T18:58:31Z

python/pyiceberg/catalog/rest.py

+    token: str
+    config: Properties
+
+    host: str


I still think this should be uri, since that better describes the behavior: this is the base URI, not a host that will be embedded in https://{host}:443/ to build URIs. That also matches the common options used by Java and are documented as common catalog config.

Good one, I've renamed this one! 👍🏻

rdblue · 2022-07-30T18:59:56Z

python/pyiceberg/catalog/rest.py

+            token: The bearer token
+        """
+        self.host = host
+        if client_id and client_secret:


Can these be passed as a single string? I'm fine supporting both credential and client_id/client_secret. I'd just like to be consistent across implementations.

Sure thing 👍🏻

rdblue · 2022-07-30T19:09:14Z

python/pyiceberg/catalog/rest.py

+        elif token:
+            self.token = token
+        else:
+            raise ValueError("Either set the client_id and client_secret, or provide a valid token")


I don't think we should fail if there isn't a valid token. Not all environments will require auth. Instead of failing, this should just fall back to not passing the Authentication header.

I didn't think of that, updated the code, thanks! 👍🏻

rdblue · 2022-07-30T19:26:06Z

python/pyiceberg/catalog/rest.py

+
+    def load_table(self, identifier: Union[str, Identifier]) -> Table:
+        response = requests.get(
+            self.url(Endpoints.load_table, prefixed=True, **self._split_identifier_for_path(identifier)), headers=self.headers


Nit: prefixed=True is the default.

Less is more 👍🏻

rdblue · 2022-07-30T19:39:38Z

python/tests/catalog/test_rest.py

+        json={
+            "error": {
+                "message": "Table does not exist: fokko.fokko2 in warehouse 8bcb0838-50fc-472d-9ddb-8feb89ef5f1e",
+                "type": "NoSuchNamespaceErrorException",


Nit: NoSuchTableException?

Not sure where that comes from 🤔

rdblue · 2022-07-30T19:41:23Z

Awesome work, @Fokko! There are still a few minor comments, but there are no blockers so I merged this. Thanks!

Some small comments pending from: apache#5287

This does not implement table commits, but does implement the catalog portion of the REST API.

Fokko added 7 commits June 20, 2022 14:48

Python: Add REST Catalog

01842e3

Merge branch 'master' of https://github.com/apache/iceberg into fd-re…

5a73bd4

…st-catalog

First few are working

776a9ea

Merge branch 'master' of https://github.com/apache/iceberg into fd-re…

ba29add

…st-catalog

.

d55fc81

Add load table

7568a3b

Merge branch 'master' of https://github.com/apache/iceberg into fd-re…

671cc07

…st-catalog

github-actions bot added core INFRA python labels Jul 15, 2022

Python: First version of the REST catalog

f1a986e

Fokko force-pushed the fd-rest-catalog branch from d8688fe to f1a986e Compare July 15, 2022 17:00

kbendick reviewed Jul 15, 2022

View reviewed changes

python/pyiceberg/catalog/rest.py Outdated Show resolved Hide resolved

rdblue reviewed Jul 15, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/SchemaParser.java Outdated Show resolved Hide resolved

rdblue reviewed Jul 15, 2022

View reviewed changes

python/tests/table/test_metadata.py Outdated Show resolved Hide resolved

rdblue reviewed Jul 15, 2022

View reviewed changes

python/pyiceberg/table/metadata.py Outdated Show resolved Hide resolved

rdblue reviewed Jul 15, 2022

View reviewed changes

python/pyiceberg/catalog/rest.py Outdated Show resolved Hide resolved

Fokko added 4 commits July 17, 2022 23:16

Process comments

a51f853

Remove the rough edges

931c3e3

Merge branch 'master' of https://github.com/apache/iceberg into fd-re…

736df3f

…st-catalog

Revert unrelated changes

49acfb0

Fokko force-pushed the fd-rest-catalog branch from c913887 to 49acfb0 Compare July 17, 2022 21:49

Fokko commented Jul 17, 2022

View reviewed changes

python/pyiceberg/table/metadata.py Show resolved Hide resolved

Fokko commented Jul 17, 2022

View reviewed changes

python/tests/utils/test_bin_packing.py Show resolved Hide resolved

Fokko mentioned this pull request Jul 20, 2022

Python: Add __version__ to the package #5315

Merged

Fokko added 2 commits July 21, 2022 11:11

Merge branch 'master' of https://github.com/apache/iceberg into fd-re…

c6dc7cd

…st-catalog

Add the missing alias

4f373f2

Fokko force-pushed the fd-rest-catalog branch from 70579bf to 4f373f2 Compare July 21, 2022 12:49

Fokko mentioned this pull request Jul 26, 2022

Spec: Always send the properties #5358

Closed

Fokko added 5 commits July 26, 2022 20:32

Add version to the headers

37763bf

Merge branch 'master' of https://github.com/apache/iceberg into fd-re…

c27ef21

…st-catalog

Bit more cleanup

5283691

Just return the table

62d823b

Allow both client_{id,secret} and token

b48722f

rdblue reviewed Jul 27, 2022

View reviewed changes

python/pyiceberg/table/base.py Outdated Show resolved Hide resolved

rdblue reviewed Jul 27, 2022

View reviewed changes

python/tests/catalog/test_base.py Outdated Show resolved Hide resolved

rdblue reviewed Jul 27, 2022

View reviewed changes

python/tests/catalog/test_base.py Outdated Show resolved Hide resolved

Comments

dc79b27

Fokko force-pushed the fd-rest-catalog branch from 501b949 to dc79b27 Compare July 28, 2022 07:38

rdblue reviewed Jul 30, 2022

View reviewed changes

rdblue approved these changes Jul 30, 2022

View reviewed changes

rdblue merged commit 9927572 into apache:master Jul 30, 2022

Fokko deleted the fd-rest-catalog branch July 31, 2022 19:56

Fokko added a commit to Fokko/iceberg that referenced this pull request Jul 31, 2022

Python: Follow up on REST comments

88f5552

Some small comments pending from: apache#5287

Fokko added a commit to Fokko/iceberg that referenced this pull request Jul 31, 2022

Python: Follow up on REST comments

31f58ee

Some small comments pending from: apache#5287

Fokko mentioned this pull request Jul 31, 2022

Python: Follow up on REST comments #5402

Merged

abmo-x pushed a commit to abmo-x/iceberg that referenced this pull request Aug 2, 2022

Python: Add REST catalog implementation (apache#5287)

3f210ca

This does not implement table commits, but does implement the catalog portion of the REST API.

	@abstractmethod
	def create_table(
	self,
	identifier: str \| Identifier,
	schema: Schema,
	location: str \| None = None,
	partition_spec: PartitionSpec \| None = None,
	properties: Properties \| None = None,
	) -> Table:
	"""Create a table

	Args:
	identifier: Table identifier.
	schema: Table's schema.
	location: Location for the table. Optional Argument.
	partition_spec: PartitionSpec for the table. Optional Argument.
	properties: Table properties that can be a string based dictionary. Optional Argument.

	Returns:
	Table: the created table instance

	Raises:
	AlreadyExistsError: If a table with the name already exists
	"""



		class TableResponse(IcebergBaseModel):
		metadata_location: Optional[str] = Field(alias="metadata-location", default=None)

Python: First version of the rest catalog #5287

Python: First version of the rest catalog #5287

Uh oh!

Conversation

Fokko commented Jul 15, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fokko Jul 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fokko Jul 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fokko Jul 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fokko Jul 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdblue Jul 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fokko Jul 26, 2022 •

edited

Loading

Fokko Jul 17, 2022 •

edited

Loading

Fokko Jul 22, 2022 •

edited

Loading

Fokko Jul 28, 2022 •

edited

Loading

rdblue Jul 30, 2022 •

edited

Loading

rdblue Jul 30, 2022 •

edited

Loading