-
Notifications
You must be signed in to change notification settings - Fork 3.2k
OpenAPI: Add CatalogObjectIdentifier schema #16144
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2267,6 +2267,27 @@ components: | |
| type: string | ||
| nullable: false | ||
|
|
||
| CatalogObjectIdentifier: | ||
| description: | ||
| Reference to a catalog object (table, view, or namespace) as an | ||
| ordered list of hierarchical levels. | ||
| The object kind is determined by context (e.g. the endpoint or a | ||
| companion CatalogObjectType discriminator), not by the identifier | ||
| structure alone. | ||
| type: array | ||
| items: | ||
| type: string | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we apply any constraints on the table/view/namespace name? For example, no slash(/) is allowed. Given we didn't specify any constraint on the table identifier, it's not a blocker for this PR. We can work on that as a followup. We can also discuss whether we could avoid any constraints in IRC, and relying on the implementations(catalogs, engines) to cast their options.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have previously let folks do whatever they want in string fields and left it up to the implementation to decide whether or not that string is invalid. |
||
| example: [ "accounting", "tax", "paid" ] | ||
|
|
||
| CatalogObjectType: | ||
| type: string | ||
| description: | | ||
| The type of a catalog object. | ||
| enum: | ||
| - table | ||
| - view | ||
| - namespace | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there was an open question about how and where this would actually be used. Introducing For example, you could have the resolve endpoint return: [
[ identifier, type, metadata ]
[ identifier, type, metadata ]
...
]or: tables: <identifier, metadata>
views: <identifier, metadata>
namespaces: <identifier, metadata>The first approach requires I'd like to see how it would be referenced in context.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I updated the design doc for the resolve endpoint and included example request and response payload json Here is the usage from the events endpoint spec PR. It is used as an event filter in the request body.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the Events Endpoint having this explicit type filter is valuable I believe which is why we introduced this type there.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it's as easy as saying the clients should tolerate it. If you add new types, generated parsers will break when they encounter something that wasn't originally enumerated. That's why the type approach feels brittle. @rdblue might have an opinion here since he originally brought it up in the discussion.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To clarify Dan's framing, here are the two response shapes he sketched: Shape A — flat array of typed records, each carrying its own discriminator: [
{ "identifier": [...], "type": "table", "metadata": {...} },
{ "identifier": [...], "type": "view", "metadata": {...} }
]Shape B — separate bucket per kind: {
"tables": [ { "identifier": [...], "metadata": {...} }, ... ],
"views": [ ... ],
"namespaces": [ ... ]
}Dan's critique: Shape A's The Shape B counter has its own forward-compat hole — and a worse one. An old client that doesn't know the The design doc actually already proposes a hybrid that handles the top-level concern: per-category typed arrays ( {
"relations": [
{
"identifier": ["analytics", "daily_sales_view"],
"status": "loaded",
"result": { "object-type": "view", "view": { /* LoadViewResult */ } }
}
]
}Adding a new top-level category ( Proposal: spec ResolveResult:
type: object
required: [object-type]
properties:
object-type:
type: string
description: |
Object kind. Currently one of: table, view, materialized-view.
Clients should fall through to a default handler for unrecognized values.
table:
$ref: '#/components/schemas/LoadTableResult'
view:
$ref: '#/components/schemas/LoadViewResult'What this buys:
Prior art for this pattern:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated proposal: drop the shared Resolve response: closed enum Events filter: closed enum on the request body. Client picks values from its own schema; server rejects unknowns with a clear error. No codegen-breakage concern, since the field never carries server-originated values back to old clients. Events response: no separate Naming: two independent named schemas — |
||
|
|
||
| PrimitiveType: | ||
| type: string | ||
| example: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add "for example" to the list, so we don't need to maintain it if we add functions, indexes etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Reworded to "Reference to a catalog object (for example, table, view, or namespace)" so the list reads as illustrative and doesn't need to be maintained as new kinds (functions, indexes, etc.) are added. Pushed in 9c2ed0a.