laskoviymishka commented on code in PR #17728:
URL: https://github.com/apache/iceberg/pull/17728#discussion_r3872862193
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -440,6 +443,30 @@ protected boolean supportsVariant() {
/* RESTCatalog specific tests */
+ @Test
+ public void testCreateV2TableWithVariantColumnFailsClientSide() {
Review Comment:
Only the reject path is pinned here. The guard has two other behaviors worth
locking in: `format-version=3` + variant should pass client-side and reach the
server, and an absent `format-version` should defer to the server rather than
being rejected locally.
A couple of `assertThatCode(...).doesNotThrowAnyException()` +
`verify(times(1))` cases would catch a future change that starts over-rejecting
— e.g. if someone assumes a v2 default in the client.
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -1155,6 +1159,14 @@ private LoadTableResponse stageCreate() {
}
}
+ private static void checkFormatVersion(Schema schema, Map<String, String>
properties) {
+ Integer formatVersion =
+ PropertyUtil.propertyAsNullableInt(properties,
TableProperties.FORMAT_VERSION);
Review Comment:
`propertyAsNullableInt` parses with `Integer.parseInt` under the hood, so a
non-integer `format-version` — `"latest"`, `"2.0"`, a typo — now throws a raw
`NumberFormatException` on the client with nothing pointing at `format-version`
as the cause. Before this change that string would've gone to the server.
Cheapest fix that keeps the existing skip-when-unset contract is to catch it
and either defer to the server or rethrow naming the property. wdyt?
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -440,6 +443,30 @@ protected boolean supportsVariant() {
/* RESTCatalog specific tests */
+ @Test
+ public void testCreateV2TableWithVariantColumnFailsClientSide() {
+ if (requiresNamespaceCreate()) {
+ restCatalog.createNamespace(TBL.namespace());
+ }
+
+ Schema variantSchema =
+ new Schema(
+ required(1, "id", Types.LongType.get()), optional(2, "data",
Types.VariantType.get()));
+
+ assertThatThrownBy(
+ () ->
+ restCatalog
+ .buildTable(TBL, variantSchema)
+ .withProperty(TableProperties.FORMAT_VERSION, "2")
+ .create())
Review Comment:
This covers `create()`, but the same `checkFormatVersion` was added to
`stageCreate()` too (reached via `createTransaction()`), and nothing exercises
that path — a regression there stays green.
I'd add a sibling test that builds the same variant + `format-version=2`
table, calls `.createTransaction()`, and asserts the `IllegalStateException`
fires with no POST, mirroring the `verify(never())` here.
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -1155,6 +1159,14 @@ private LoadTableResponse stageCreate() {
}
}
+ private static void checkFormatVersion(Schema schema, Map<String, String>
properties) {
+ Integer formatVersion =
+ PropertyUtil.propertyAsNullableInt(properties,
TableProperties.FORMAT_VERSION);
+ if (formatVersion != null) {
+ Schema.checkCompatibility(schema, formatVersion);
Review Comment:
The server path we're mirroring here (`newTableMetadata`) runs
`EncryptionUtil.checkCompatibility(properties, formatVersion)` right after the
schema check, so encryption properties on v1/v2 still slip past this guard and
only fail server-side.
If we want this to be a faithful early mirror I'd add that call alongside
the schema one. If encryption is out of scope on purpose, a one-line comment
saying so would save the next reader the double-take.
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -440,6 +443,30 @@ protected boolean supportsVariant() {
/* RESTCatalog specific tests */
+ @Test
+ public void testCreateV2TableWithVariantColumnFailsClientSide() {
+ if (requiresNamespaceCreate()) {
+ restCatalog.createNamespace(TBL.namespace());
+ }
+
+ Schema variantSchema =
+ new Schema(
+ required(1, "id", Types.LongType.get()), optional(2, "data",
Types.VariantType.get()));
+
+ assertThatThrownBy(
+ () ->
+ restCatalog
+ .buildTable(TBL, variantSchema)
+ .withProperty(TableProperties.FORMAT_VERSION, "2")
+ .create())
+ .isInstanceOf(IllegalStateException.class)
+ .hasMessageContaining("variant is not supported until v3");
Review Comment:
This anchors on a fragment produced deep inside `Schema.checkCompatibility`,
so if that message gets reworded the test breaks without telling us whether the
behavior or just the wording changed.
I'd pin the field too — `.hasMessageContaining("Invalid type for data:
variant is not supported until v3")` — or drop to the coarser
`IllegalStateException` + `"Invalid schema for v2"`. 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]
