laskoviymishka commented on code in PR #17256:
URL: https://github.com/apache/iceberg/pull/17256#discussion_r3616530655
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -926,6 +931,34 @@ public void testCreateTableWithDefaultColumnValue() {
assertThat(schemaWithDefault.asStruct()).isEqualTo(catalog.loadTable(TBL).schema().asStruct());
}
+ @Test
+ public void testCreateTableWithVariantColumn() {
+ assumeThat(supportsVariant()).as("Catalog does not support the variant
type").isTrue();
+
+ C catalog = catalog();
+
+ if (requiresNamespaceCreate()) {
+ catalog.createNamespace(TBL.namespace());
+ }
+
+ assertThat(catalog.tableExists(TBL)).as("Table should not
exist").isFalse();
+
+ Schema variantSchema =
+ new Schema(
+ required(1, "id", Types.LongType.get()), optional(2, "data",
Types.VariantType.get()));
+
+ catalog
+ .buildTable(TBL, variantSchema)
+ .withLocation(baseTableLocation(TBL))
+ .withProperty(TableProperties.FORMAT_VERSION, "3")
+ .create();
+
+ assertThat(catalog.tableExists(TBL)).as("Table should exist").isTrue();
+ assertThat(catalog.loadTable(TBL).schema().asStruct())
+ .as("Variant column should round-trip through the catalog")
+ .isEqualTo(variantSchema.asStruct());
Review Comment:
We pass `FORMAT_VERSION` `"3"` on create but only assert the struct
round-trips — never that the loaded table actually landed at v3. If a catalog
silently ignores or downgrades the hint, this still passes while the table is
v2, which is invalid for a variant column per spec. I'd add
`assertThat(TableUtil.formatVersion(catalog.loadTable(TBL))).isEqualTo(3)`.
A negative case — a v2 table with a variant column, gated on
`supportsVariant()`, asserting it's rejected — would make the v3-only rule even
more explicit, though that can be a follow-up.
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -926,6 +931,34 @@ public void testCreateTableWithDefaultColumnValue() {
assertThat(schemaWithDefault.asStruct()).isEqualTo(catalog.loadTable(TBL).schema().asStruct());
}
+ @Test
+ public void testCreateTableWithVariantColumn() {
+ assumeThat(supportsVariant()).as("Catalog does not support the variant
type").isTrue();
+
+ C catalog = catalog();
+
+ if (requiresNamespaceCreate()) {
+ catalog.createNamespace(TBL.namespace());
+ }
+
+ assertThat(catalog.tableExists(TBL)).as("Table should not
exist").isFalse();
+
+ Schema variantSchema =
+ new Schema(
+ required(1, "id", Types.LongType.get()), optional(2, "data",
Types.VariantType.get()));
Review Comment:
This only exercises a top-level variant column. The `oneOf` resolution for
variant also runs through `ListType.element` and `MapType.value` (both `$ref:
Type`), so a variant-as-list-element or variant-as-map-value case would confirm
the discrimination works end-to-end and not just at the top level. iceberg-go
already has those nested cases if you want a reference.
##########
open-api/rest-catalog-open-api.py:
##########
@@ -122,6 +122,10 @@ class PrimitiveType(RootModel[str]):
root: str = Field(..., examples=[['long', 'string', 'fixed[16]',
'decimal(10,2)']])
+class VariantType(RootModel[Literal['variant']]):
Review Comment:
Was this regenerated via `make generate`, or hand-edited? datamodel-codegen
orders classes by their definition order in the YAML, and `VariantType` is
defined down near `MapType` (~line 2513), so a regen would emit the class there
rather than right after `PrimitiveType` here — which makes me think this
placement (and the missing `Field(...)` descriptor the other classes have) is a
manual edit that'll drift on the next `make generate`.
If you move the `VariantType` schema in the YAML to just after
`PrimitiveType` and regenerate, the placement here falls out naturally and
stays stable. wdyt?
##########
open-api/rest-catalog-open-api.py:
##########
@@ -1902,8 +1906,8 @@ class Schema(StructType):
identifier_field_ids: list[int] | None = Field(None,
alias='identifier-field-ids')
-class Type(RootModel[PrimitiveType | StructType | ListType | MapType]):
- root: PrimitiveType | StructType | ListType | MapType
+class Type(RootModel[PrimitiveType | StructType | ListType | MapType |
VariantType]):
Review Comment:
The `not: {const: "variant"}` on `PrimitiveType` keeps this `oneOf`
unambiguous in the YAML, but datamodel-codegen drops `not:` on generation — so
the generated `PrimitiveType` stays `RootModel[str]` and accepts `"variant"`.
Since the union is evaluated left-to-right, `Type.model_validate("variant")`
resolves to `PrimitiveType`, not `VariantType`. The schema-layer invariant
doesn't hold in the generated code.
I'd reorder the `oneOf` to list `VariantType` before `PrimitiveType` (then
regenerate so the union order follows), or add a `@field_validator('root')` on
`PrimitiveType` that rejects `"variant"`. wdyt?
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -414,6 +414,11 @@ protected boolean supportsServerSideRetry() {
return true;
}
+ @Override
+ protected boolean supportsVariant() {
Review Comment:
Minor: this lands between `supportsServerSideRetry` and
`supportsNestedNamespaces`, which breaks the declaration order — in
`CatalogTests` `supportsVariant` sits at the end of the `supports*` group. I'd
move it down after the last `supports*` override to match.
##########
open-api/src/test/java/org/apache/iceberg/rest/RESTCompatibilityKitCatalogTests.java:
##########
@@ -85,6 +85,11 @@ protected boolean supportsServerSideRetry() {
restCatalog.properties(),
RESTCompatibilityKitSuite.RCK_SUPPORTS_SERVERSIDE_RETRY, true);
}
+ @Override
+ protected boolean supportsVariant() {
+ return true;
Review Comment:
Every other capability flag here reads an `RCK_SUPPORTS_*` property via
`PropertyUtil.propertyAsBoolean(...)` — this one hardcodes `true`. Any external
catalog run through the RCK that doesn't support v3/variant will now execute
`testCreateTableWithVariantColumn` unconditionally and fail, with no way to opt
out.
I'd add `RCK_SUPPORTS_VARIANT = "rck.supports-variant"` to
`RESTCompatibilityKitSuite` and read it here with a default of `false`,
matching the other flags.
--
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]
