OpenAPI, Core: Add VariantType to the REST catalog spec#17256
Conversation
Co-authored-by: Amogh Jahagirdar <amoghj@apache.org>
laskoviymishka
left a comment
There was a problem hiding this comment.
Really like that this lands the spec change, the CatalogTests round-trip, and the RCK coverage together. That feels like the right shape for adding a new type to the REST contract, and the bare-string "variant" encoding matches SchemaParser and Appendix C.
I’d still hold this before merge on two points.
The main one is code generation. The not: {const: "variant"} constraint on PrimitiveType is correct and keeps the oneOf unambiguous in JSON Schema, but datamodel-codegen drops not:. As a result, the generated PrimitiveType remains RootModel[str] and still accepts "variant".
Since the Type union is evaluated left-to-right and PrimitiveType comes first, Type.model_validate("variant") resolves to PrimitiveType, not VariantType. So the schema has the right invariant, but the generated client does not. I’d either put VariantType before PrimitiveType in the oneOf and regenerate, or add a validator that rejects "variant" from PrimitiveType.
The other blocker is the RCK behavior. RESTCompatibilityKitCatalogTests.supportsVariant() should read an RCK_SUPPORTS_VARIANT property, defaulting to false, rather than returning true unconditionally. That lets catalogs without v3 or variant support opt out.
Since this changes the REST spec, it should also get the usual dev-list note. Amogh raised the same point on #14242. Please link the thread here once it is posted so the change remains traceable.
The smaller comments are inline: add a format-version assertion to the round-trip test, cover a nested variant, and confirm whether the generated .py file came from make generate rather than being edited manually. The class placement made me wonder.
Separately, neither iceberg-rust nor PyIceberg has a VariantType in its own type system yet. Tracking issues for round-trip support in those implementations seem like the right follow-up, rather than expanding this PR further.
Once the codegen and RCK points are addressed, I’m happy to take another pass.
|
Thanks for the review @laskoviymishka. I'll address the comments and send a note to dev@ as well. |
Once the spec change is complete, I'll do the follow ups as well (RCK, etc) and open the tracking for each impl. Currently, this PR has been scoped down to the REST API spec alone. Thanks @laskoviymishka . |
Rationale for the change
Adds the variant type to the REST catalog OpenAPI spec so that variant columns are representable in table schemas exchanged over the REST protocol.
Builds on #14242. That change modeled variant as an object (
{"type": "variant"}); this change instead models it as a bare stringconst: "variant"to match howSchemaParserserializes it on the wire (SchemaParserwrites the type as the string"variant")Changes
open-api/rest-catalog-open-api.yamlAdded aVariantTypeschemaopen-api/rest-catalog-open-api.py- regenerated viamake generateTesting
cd open-api && make install && make lint && make generate && git diff --exit-code- spec validates and the generated.pyis up to date.Type.model_validate("variant")resolves toVariantType, and other type strings (int,string,long,decimal(10,2),fixed[16]) still resolve toPrimitiveType.