laskoviymishka commented on code in PR #17500:
URL: https://github.com/apache/iceberg/pull/17500#discussion_r3719090066
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -966,6 +971,71 @@ public void testLoadTable() {
.containsAll(properties.entrySet());
}
+ @Test
+ public void testCreateTableWithVariantColumn() {
+ assumeThat(supportsVariant()).as("Catalog supports 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()),
+ optional(3, "list_data", Types.ListType.ofOptional(5,
Types.VariantType.get())),
+ optional(
+ 4,
+ "map_data",
+ Types.MapType.ofOptional(6, 7, Types.StringType.get(),
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();
+
+ Table loaded = catalog.loadTable(TBL);
+ assertThat(loaded.schema().asStruct())
+ .as("Variant columns should round-trip through the catalog")
+ .isEqualTo(variantSchema.asStruct());
+ assertThat(TableUtil.formatVersion(loaded))
+ .as("Table with a variant column must be format version 3")
+ .isEqualTo(3);
+ }
+
+ @Test
+ public void testCreateV2TableWithVariantColumnFails() {
+ assumeThat(supportsVariant()).as("Catalog supports the variant
type").isTrue();
+
+ C catalog = catalog();
+
+ if (requiresNamespaceCreate()) {
+ catalog.createNamespace(TBL.namespace());
+ }
+
+ Schema variantSchema =
+ new Schema(
+ required(1, "id", Types.LongType.get()), optional(2, "data",
Types.VariantType.get()));
+
+ assertThatThrownBy(
+ () ->
+ catalog
+ .buildTable(TBL, variantSchema)
+ .withLocation(baseTableLocation(TBL))
+ .withProperty(TableProperties.FORMAT_VERSION, "2")
+ .create())
+ .hasMessageContaining("is not supported until v3");
Review Comment:
This passes for any exception that happens to carry that substring,
including an NPE from a path we never meant to reach. Every other
`assertThatThrownBy` in this file pins the type first, e.g.
`.isInstanceOf(AlreadyExistsException.class).hasMessageContaining(...)`.
Worth being deliberate about which type, though: on the direct path this is
`IllegalStateException` from `Schema.checkCompatibility`, but through
`RESTCatalogAdapter` that class isn't in `EXCEPTION_ERROR_CODES`, so it comes
back as a 500 → `ServiceFailureException` with our original message embedded.
That embedding is the only reason the substring matches today.
And `is not supported until v3` is our Java wording — an external RCK server
that correctly rejects this with a 400 and its own message would fail here.
`tableExists(TBL)` being `false` on the next line is the real conformance
invariant, so I'd pin the type and either drop the message match or keep it
scoped to the reference run.
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -966,6 +971,71 @@ public void testLoadTable() {
.containsAll(properties.entrySet());
}
+ @Test
+ public void testCreateTableWithVariantColumn() {
+ assumeThat(supportsVariant()).as("Catalog supports the variant
type").isTrue();
Review Comment:
Tiny thing, but this description reads as a claim rather than a reason for
skipping, which is what shows up in the report when the assumption fails.
The sibling guards phrase it as the condition — `.as("Only valid when the
catalog supports nested namespaces")`. I'd match that here and in
`testCreateV2TableWithVariantColumnFails`.
##########
open-api/README.md:
##########
@@ -86,6 +86,7 @@ are strictly defined by the REST Specification. The
following are currently con
|-------------------------------|---------|
| rck.requires-namespace-create | true |
| rck.supports-serverside-retry | true |
+| rck.supports-variant | false |
Review Comment:
Drive-by while this file is open: the table is also missing
`rck.overrides-requested-location` and `rck.supports-names-with-dot`, both
`false`. Pre-existing gap and not yours — but it's two rows, and this table is
the only place external implementors go looking for these.
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -966,6 +971,71 @@ public void testLoadTable() {
.containsAll(properties.entrySet());
}
+ @Test
+ public void testCreateTableWithVariantColumn() {
+ assumeThat(supportsVariant()).as("Catalog supports 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()),
+ optional(3, "list_data", Types.ListType.ofOptional(5,
Types.VariantType.get())),
+ optional(
+ 4,
+ "map_data",
+ Types.MapType.ofOptional(6, 7, Types.StringType.get(),
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();
+
+ Table loaded = catalog.loadTable(TBL);
+ assertThat(loaded.schema().asStruct())
+ .as("Variant columns should round-trip through the catalog")
+ .isEqualTo(variantSchema.asStruct());
Review Comment:
This asserts against the same `Schema` object that went into `buildTable`,
and the ids here (1–7) are exactly what `assignFreshIds` produces for this
shape — so it's passing on an ID coincidence rather than on a stated
expectation.
`newTableMetadata` unconditionally reassigns through
`TypeUtil.assignFreshIds`, and this file already separates the two cases:
`SCHEMA` goes in with ids 3,4 and `TABLE_SCHEMA` with ids 1,2 is what gets
asserted, carrying the comment "This is the actual schema for the table, with
column IDs reassigned".
I'd mirror that split — pass an input schema with different ids, assert
against an explicit expected one. Makes the reassignment part of the contract
instead of an accident, and keeps a catalog that assigns ids from a different
base from false-failing the kit.
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -966,6 +971,71 @@ public void testLoadTable() {
.containsAll(properties.entrySet());
}
+ @Test
+ public void testCreateTableWithVariantColumn() {
+ assumeThat(supportsVariant()).as("Catalog supports 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()),
+ optional(3, "list_data", Types.ListType.ofOptional(5,
Types.VariantType.get())),
+ optional(
+ 4,
+ "map_data",
+ Types.MapType.ofOptional(6, 7, Types.StringType.get(),
Types.VariantType.get())));
Review Comment:
Top-level, list element, and map value cover the interesting positions
except variant as a struct field — something like `optional(8, "struct_data",
Types.StructType.of(optional(9, "v", Types.VariantType.get())))`.
`checkCompatibility` already walks nested struct fields via `IndexById`, so
this isn't a hole in the Java validation. It's a hole in what the RCK tells an
external implementor about serializing `struct<v: variant>` through the schema
JSON. Cheap to add while we're here — wdyt?
--
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]
