Polaris Catalog Spec: Generic Table APIs #1150

gh-yzou · 2025-03-10T21:24:41Z

This PR defines the open api spec for Generic Tables operations, corresponding definition defined in https://docs.google.com/document/d/1_R9jBIwoH3CV9G7gSoRJPQVEcOsROp4IEiGoeYeQE8A/edit?pli=1&tab=t.0

The set of APIs includes:
listGenericTables
createGenericTable
loadGenericTable
dropGenericTable

Another discussion is happening about whether we want to introduce a completely new service file for new prefix, we will do this as a refactor if we decided to.

HonahX · 2025-03-11T21:07:43Z

Could you please also run https://github.com/apache/polaris/tree/main/spec#generate-the-bundle to update the bundled-*.yaml, so that others can see new APIs on the website

HonahX · 2025-03-11T21:42:16Z

spec/polaris-catalog-apis/generic-tables-api.yaml

+    parameters:
+      - $ref: '../iceberg-rest-catalog-open-api.yaml#/components/parameters/prefix'
+      - $ref: '../iceberg-rest-catalog-open-api.yaml#/components/parameters/namespace'
+      - $ref: '../iceberg-rest-catalog-open-api.yaml#/components/parameters/table'


[non-blocking] Should this parameter named genericTable to differentiate from Iceberg's table?

I was trying to reuse as much as possible from iceberg, but i agree it is probably better to be more specific on this, and that parameter definition is also simple. Updated to have new parameter generic-table ( this is the same naming style as iceberg parameter)

flyrain

LGTM overall. Left some minor comments.

flyrain · 2025-03-11T22:57:32Z

spec/polaris-catalog-service.yaml

-  # Polaris-native API #
-  ######################
+  ###################################
+  # Polaris-native notification API #


Nit: should we be consistent with the name, the following section(line 138) doesn't say Polaris-native

Suggested change

# Polaris-native notification API #

# Polaris notification API #

I think this was changed to make it clear the notifications don't relate to generic tables (although I suppose they could). So in that way, you could say it's good to have the names mismatch.

However I still think Polaris Notification API & Polaris Generic Table API is fine. Polaris-native Generic Table API is a bit of a mouthful.

Polaris Notification API sounds good to me, updated

flyrain · 2025-03-11T23:17:22Z

spec/polaris-catalog-apis/generic-tables-api.yaml

+        419:
+          $ref: '../iceberg-rest-catalog-open-api.yaml#/components/responses/AuthenticationTimeoutResponse'


Can we remove it? Polaris doesn't implement it, more context is here #791. And Iceberg considers it optional and will deprecate it soon. apache/iceberg#12376

i see, thanks for the information! Removed

flyrain · 2025-03-11T23:17:34Z

spec/polaris-catalog-apis/generic-tables-api.yaml

+        419:
+          $ref: '../iceberg-rest-catalog-open-api.yaml#/components/responses/AuthenticationTimeoutResponse'


flyrain · 2025-03-11T23:22:14Z

spec/polaris-catalog-apis/generic-tables-api.yaml

+              examples:
+                TableToLoadDoesNotExist:
+                  $ref: '../iceberg-rest-catalog-open-api.yaml#/components/examples/NoSuchTableError'
+        419:


flyrain · 2025-03-11T23:22:27Z

spec/polaris-catalog-apis/generic-tables-api.yaml

+        419:
+          $ref: '../iceberg-rest-catalog-open-api.yaml#/components/responses/AuthenticationTimeoutResponse'


flyrain · 2025-03-11T23:23:41Z

spec/polaris-catalog-apis/generic-tables-api.yaml

+          type: object
+          additionalProperties:
+            type: string
+        register-at:


Should this be create-at?

Based on our design discussion, we settled on register-at to be more specific that this is when the entity is registered with the service.

To make things more clear, i added a description for the GenericTable to describe the meaning of each field

I wonder if we should call the endpoint register... instead of create... then

I think the operation methods should be create, since it is an operation called on table create and used to create table entities at the service, and this also aligns with how iceberg and other catalog services names the method also.

For that particular filed, the original proposal is create-at, but there is complain that the name kind of indicates it is when the actual table is created, and then we switched to use this name and hope this can make it clear that it is the time for the Polaris service, which I think should be fine as far as we can also make our doc description clear about this particular field.

indicates it is when the actual table is created

It is also possible that generic table is actually created in Polaris, right? Considering this use case, I'm leaning toward the name create-at, as it is a catalog property, not table one. Any users checking the real table creation time will have to check the real table's properties(e.g. metadata.json file for iceberg), instead of relying on a table property in catalog.

@flyrain would you mind give more details about "It is also possible that generic table is actually created in Polaris"? do you mean the generic table entity is created in Polaris, or do are you really talking about Polaris is creating a table, what is that use case?

For example, Spark create a brand new Delta table under one namespace in Polaris. Is it possible or not?

yes, that is case we are supporting, we are going to support the replacement of Spark Catalog. I also prefer create-at, i think as far as we make the doc clear that is the server side time we should be good. Let me update to create-at

Actually, since this field is not really needed to achieve functionality, we can remove it for now, and added it back later if this information is needed for client side to perform monitoring capability

collado-mike

Minor comments only

collado-mike · 2025-03-12T22:06:21Z

spec/polaris-catalog-service.yaml

+  /polaris/v1/{prefix}/namespaces/{namespace}/generic-tables:
+    $ref: './polaris-catalog-apis/generic-tables-api.yaml#/paths/~1polaris~1v1~1{prefix}~1namespaces~1{namespace}~1generic-tables'
+
+  /polaris/v1/{prefix}/namespaces/{namespace}/generic-tables/{generic-table}:
+    $ref: './polaris-catalog-apis/generic-tables-api.yaml#/paths/~1polaris~1v1~1{prefix}~1namespaces~1{namespace}~1generic-tables~1{generic-table}'


Shouldn't we keep these separate from the rest of the Iceberg APIs?

That is a good question. In fact, @HonahX is going to carry out a discussion about this later about do we want to have an independent service file for the APIs under new prefix.

I think we can keep this discussion separate with this PR, and if we decided to do that, we can just have one pr to put all new definitions to a separate service file, and there is some other things like (auth definition) need to be take care of, so that we can keep the purpose of each pr more clear. Meanwhile @HonahX and I can carry this discussion independently soon. Would that work?

Thanks @gh-yzou for summarizing. We can always re-organize these spec definitions if needed without affecting the implementaion. The current way allow us to share common open api definitions (servers, security scheme definition), and we can still generate models and services in separate module, like how they are generated in this PR. But let's discuss this in a separate thread to decide whether we want to separate them into completely different files.

collado-mike · 2025-03-12T22:12:45Z

spec/README.md

 - `polaris-catalog-service.yaml` - Defines the specification for the Polaris Catalog API, which encompasses both the Iceberg REST Catalog API
   and Polaris-native API.
-  - `polaris-catalog-apis` - Contains the specifications of Polaris-native API
+  - `polaris-catalog-apis` - Contains the specification for Polaris-native Catalog APIs


Alongside Yufei's comment, I kinda think that the text "Polaris-native", in reference to table types, should go away. Polaris doesn't have table types - unlike, for example, Polaris-native principals or Polaris-native roles. Maybe "Polaris-specific" or "Polaris proprietary" APIs?

Polaris specific APIs make sense to me, let me update that.

flyrain · 2025-03-13T16:40:34Z

Thanks a lot @gh-yzou for adding new APIs for generic table! This is going to add a lot of value to Polaris users. Thanks @eric-maynard @collado-mike @HonahX for the review.

snazy · 2025-03-14T09:40:29Z

Related to the discussion on the dev-ML: Concerns about changing and/or depending on the Iceberg REST spec have been raised multiple times on PRs and on the dev-mailing-list. This PR introduces a hard dependency on the Iceberg REST spec.

The approach take in this PR and #808 should have discussed explicitly on the dev mailing list, as stated in the project's contributing guidelines, which explicitly say: "Change of public interface (or more generally speaking Polaris extension point) should be discussed and approved on the dev mailing list. The discussion on the dev mailing list should happen before having a “ready-for-review” Pull Request."

gh-yzou · 2025-03-14T21:02:14Z

Hi @snazy, I see your point about the dependency on the Iceberg REST spec. However, I believe the position Polaris taken is IRC compatible, in that case, i think it make sense for us to reuse the Iceberg definitions, such as namespace, or errors. cc @RussellSpitzer @flyrain
If you still have concern about the dependency on Iceberg REST spec definition, please feel free to bring this up as a discussion on the dev mailing list, we will be happy to accommodate any further change needed to fit into how Polaris works.

sfc-gh-yzou added 2 commits March 10, 2025 13:20

add generic table apis

56bdeec

add new line

586acab

gh-yzou requested review from adutra, ashvina, collado-mike, dennishuo, dimas-b, eric-maynard, jackye1995, jbonofre, snazy and vvcephei as code owners March 10, 2025 21:24

github-project-automation bot added this to Basic Kanban Board Mar 10, 2025

gh-yzou requested review from MonkeyCanCode, RussellSpitzer, ebyhr, flyrain and takidau as code owners March 10, 2025 21:24

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Mar 10, 2025

sfc-gh-yzou added 2 commits March 10, 2025 15:10

update table to GenericTable

20d1368

add example

14929a5

eric-maynard approved these changes Mar 11, 2025

View reviewed changes

github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Mar 11, 2025

sfc-gh-yzou added 3 commits March 11, 2025 14:31

add updated readme and bundle yaml

74318d1

add license

3b2730c

update README file

0e180ba

HonahX approved these changes Mar 11, 2025

View reviewed changes

flyrain reviewed Mar 11, 2025

View reviewed changes

address feedback

18c9ac6

sfc-gh-yzou added 3 commits March 11, 2025 17:37

replace parameter table with generic-table

4445ecc

add license back

8c41774

add doc for GenericTable

1f550be

gh-yzou requested a review from flyrain March 12, 2025 00:51

capitialization

82127f0

flyrain approved these changes Mar 12, 2025

View reviewed changes

gh-yzou requested a review from sfc-gh-yzou March 12, 2025 21:19

sfc-gh-yzou added 2 commits March 12, 2025 14:29

fix error

fd96bd2

add license back

ce1031d

collado-mike approved these changes Mar 12, 2025

View reviewed changes

sfc-gh-yzou added 2 commits March 12, 2025 15:22

fix readme

34cf463

fix model name

9792d70

HonahX mentioned this pull request Mar 12, 2025

Polaris Catalog Spec: Policy Management Apis #808

Merged

flyrain approved these changes Mar 13, 2025

View reviewed changes

flyrain merged commit 9425a6e into apache:main Mar 13, 2025
5 checks passed

github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board Mar 13, 2025

	# Polaris-native notification API #
	# Polaris notification API #

		419:
		$ref: '../iceberg-rest-catalog-open-api.yaml#/components/responses/AuthenticationTimeoutResponse'

Polaris Catalog Spec: Generic Table APIs #1150

Polaris Catalog Spec: Generic Table APIs #1150

Uh oh!

Conversation

gh-yzou commented Mar 10, 2025

Uh oh!

HonahX commented Mar 11, 2025

Uh oh!

HonahX Mar 11, 2025 • 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!

flyrain left a comment

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!

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!

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!

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!

collado-mike left a comment

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!

flyrain commented Mar 13, 2025

Uh oh!

snazy commented Mar 14, 2025

Uh oh!

gh-yzou commented Mar 14, 2025

Uh oh!

Reviewers

Assignees

Labels

HonahX Mar 11, 2025 •

edited

Loading