Core, OpenAPI: Add RCK coverage for variant columns - #17500
Conversation
laskoviymishka
left a comment
There was a problem hiding this comment.
This is clean, and the three column placements — top-level, list element, map value — are the right cuts for catalog round-trip coverage. The flag wiring matches every sibling capability flag in CatalogTests, and asserting TableUtil.formatVersion(loaded) is 3 is a nice touch — it catches a catalog that silently downgrades instead of honoring the request.
On the default-false question, I'm with @nssalian. Every other optional capability flag is opt-in, and flipping this one turns skips into failures for existing RCK consumers until they opt out. Keeping it false is the consistent call.
Two things I'd tighten, both small:
assertThatThrownByin the v2 test doesn't pin the exception type, so it passes for any exception carrying that substring. Every otherassertThatThrownByin this file pairs.isInstanceOf(...)with the message match. Worth following here especially, sinceis not supported until v3is our own Java wording and a spec-compliant server rejecting with its own message would fail it.- The round-trip assertion compares against the same
Schemathat went in, and its ids 1–7 are exactly whatassignFreshIdsproduces.testBasicCreateTablesplits these deliberately —SCHEMAwith ids 3,4 in,TABLE_SCHEMAwith ids 1,2 asserted — so I'd mirror that and make the reassignment explicit.
Neither is a merge blocker for me and I don't want to hold up coverage that's clearly net-positive. Happy to approve once those are in, or as follow-ups if you'd rather land this now.
|
|
||
| @Test | ||
| public void testCreateTableWithVariantColumn() { | ||
| assumeThat(supportsVariant()).as("Catalog supports the variant type").isTrue(); |
There was a problem hiding this 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.
| optional( | ||
| 4, | ||
| "map_data", | ||
| Types.MapType.ofOptional(6, 7, Types.StringType.get(), Types.VariantType.get()))); |
There was a problem hiding this 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?
| Table loaded = catalog.loadTable(TBL); | ||
| assertThat(loaded.schema().asStruct()) | ||
| .as("Variant columns should round-trip through the catalog") | ||
| .isEqualTo(variantSchema.asStruct()); |
There was a problem hiding this 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.
| .withLocation(baseTableLocation(TBL)) | ||
| .withProperty(TableProperties.FORMAT_VERSION, "2") | ||
| .create()) | ||
| .hasMessageContaining("is not supported until v3"); |
There was a problem hiding this 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.
There was a problem hiding this comment.
I'm not sure if this is what @laskoviymishka is saying but fundamentally this test feels a bit weird to me. Is it even failing in the catalog? the validation that's being triggered here should just be a purely client side validation and so I'm not sure what we're really testing from a compatibility angle here.
There was a problem hiding this comment.
If it is failing in the catalog side, feels like we should just improve the client to detect this case and just reject it before even attempting the table create.
| |-------------------------------|---------| | ||
| | rck.requires-namespace-create | true | | ||
| | rck.supports-serverside-retry | true | | ||
| | rck.supports-variant | false | |
There was a problem hiding this 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.
There was a problem hiding this comment.
Will add. Good catch.
| .withLocation(baseTableLocation(TBL)) | ||
| .withProperty(TableProperties.FORMAT_VERSION, "2") | ||
| .create()) | ||
| .hasMessageContaining("is not supported until v3"); |
There was a problem hiding this comment.
I'm not sure if this is what @laskoviymishka is saying but fundamentally this test feels a bit weird to me. Is it even failing in the catalog? the validation that's being triggered here should just be a purely client side validation and so I'm not sure what we're really testing from a compatibility angle here.
| } | ||
|
|
||
| @Test | ||
| public void testCreateTableWithVariantColumn() { |
There was a problem hiding this comment.
What about a schema evolution case? V3 table without variant and then later adding a variant column?
Rationale for the change
#17256 added the
varianttype to the REST catalog spec. This follows up that change with an RCK test.Changes
CatalogTests: adds asupportsVariant()flag (defaultfalse);testCreateTableWithVariantColumn(variant at top level, list element, and map value; asserts round-trip + format version 3);testCreateV2TableWithVariantColumnFails(variant is v3-only).TestRESTCatalog: enablessupportsVariant()so the tests run against the REST reference implementation.RESTCompatibilityKitCatalogTests/RESTCompatibilityKitSuite): adds opt-inrck.supports-variantproperty (defaultfalse);open-api/README.md