stevenzwu commented on code in PR #16144:
URL: https://github.com/apache/iceberg/pull/16144#discussion_r3204616715
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -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
+ example: [ "accounting", "tax", "paid" ]
+
+ CatalogObjectType:
+ type: string
+ description: |
+ The type of a catalog object.
+ enum:
+ - table
+ - view
+ - namespace
Review Comment:
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:
```json
[
{ "identifier": [...], "type": "table", "metadata": {...} },
{ "identifier": [...], "type": "view", "metadata": {...} }
]
```
**Shape B** — separate bucket per kind:
```json
{
"tables": [ { "identifier": [...], "metadata": {...} }, ... ],
"views": [ ... ],
"namespaces": [ ... ]
}
```
Dan's critique: Shape A's `type` is a closed enum, so adding a new value
(e.g. `function`) breaks generated parsers on old clients.
The Shape B counter has its own forward-compat hole — and a worse one. An
old client that doesn't know the `materialized_views` key never iterates that
field, so the identifier just *vanishes* from the resolved set; the client
can't even distinguish "didn't resolve" from "resolved to a kind I don't
recognize." A parse error at least tells you something is wrong.
The design doc actually already proposes a hybrid that handles the top-level
concern: per-category typed arrays (`relations: [...]`, future `functions:
[...]`) bucket the major categories Shape-B style, while inside each bucket the
result is a flat record that carries an `object-type` discriminator Shape-A
style:
```json
{
"relations": [
{
"identifier": ["analytics", "daily_sales_view"],
"status": "loaded",
"result": { "object-type": "view", "view": { /* LoadViewResult */ } }
}
]
}
```
Adding a new top-level category (`functions`) is safe: generators silently
ignore unknown top-level fields. The forward-compat risk only lives on
`result.object-type` — that's where adding `materialized-view` later could
break old codegen if it's a closed enum.
**Proposal:** spec `result.object-type` (and `CatalogObjectType` itself) as
`type: string` with documented values, not a closed `enum`.
```yaml
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:
- **No codegen breakage.** `datamodel-codegen` emits `object_type: str`, not
`Literal[...]`. Pydantic, Jackson with default config, Go's `encoding/json`,
etc. all accept arbitrary strings.
- **No silent data loss.** Every resolved identifier still has a row with
`identifier`, `status`, and `result`. A hand-written client switches on known
`object-type` values and falls through unknown ones to a "skip / report
unsupported" branch with full visibility.
- **Schema still documents the valid set.** The description carries the
enumeration; humans and IDE tooltips see it. We can validate server-side
conformance in our own CI without imposing closed-enum behavior on every
generated client.
- **Filter input keeps the closed enum.** `CatalogObjectType` as `enum` on
request bodies (e.g. the events filter) is fine — the client picks values from
its own schema; the server tolerates older filter sets.
Prior art for this pattern:
- The [Kubernetes API
conventions](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#typical-status-properties),
under *Typical status properties*, explicitly call closed enums a
forward-compat hazard: *"Phase was essentially a state-machine enumeration
field, that contradicted system-design principles and hampered evolution, since
adding new enum values breaks backward compatibility."*
- [Stripe's upgrade
policy](https://docs.stripe.com/upgrades#backward-compatible-changes) lists
*"Adding new event types"* as a backward-compatible change and tells clients:
*"Make sure that your webhook listener gracefully handles unfamiliar event
types."*
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
