From e61479aacdbb5dd00777fa265ef0a5075f14cc38 Mon Sep 17 00:00:00 2001 From: Mingliang Liu Date: Mon, 21 Mar 2022 17:45:05 -0700 Subject: [PATCH 1/4] API: Fix NPE when validating identifier fields --- api/src/main/java/org/apache/iceberg/Schema.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/api/src/main/java/org/apache/iceberg/Schema.java b/api/src/main/java/org/apache/iceberg/Schema.java index fcb4930e5593..880369601f47 100644 --- a/api/src/main/java/org/apache/iceberg/Schema.java +++ b/api/src/main/java/org/apache/iceberg/Schema.java @@ -109,6 +109,8 @@ public Schema(int schemaId, List columns, Map alia static void validateIdentifierField(int fieldId, Map idToField, Map idToParent) { Types.NestedField field = idToField.get(fieldId); + Preconditions.checkArgument(field != null, + "Can not add filedId %d as an identifier field: field not exists", fieldId); Preconditions.checkArgument(field.type().isPrimitiveType(), "Cannot add field %s as an identifier field: not a primitive type field", field.name()); Preconditions.checkArgument(field.isRequired(), From c0395e7272094139b21c6f867ce47e3cebc5ec30 Mon Sep 17 00:00:00 2001 From: Mingliang Liu Date: Tue, 22 Mar 2022 11:33:01 -0700 Subject: [PATCH 2/4] Address comments --- api/src/main/java/org/apache/iceberg/Schema.java | 2 +- .../test/java/org/apache/iceberg/TestSchemaUpdate.java | 10 +++++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/api/src/main/java/org/apache/iceberg/Schema.java b/api/src/main/java/org/apache/iceberg/Schema.java index 880369601f47..10097d2daece 100644 --- a/api/src/main/java/org/apache/iceberg/Schema.java +++ b/api/src/main/java/org/apache/iceberg/Schema.java @@ -110,7 +110,7 @@ static void validateIdentifierField(int fieldId, Map Map idToParent) { Types.NestedField field = idToField.get(fieldId); Preconditions.checkArgument(field != null, - "Can not add filedId %d as an identifier field: field not exists", fieldId); + "Cannot add fieldId %d as an identifier field: field not exists", fieldId); Preconditions.checkArgument(field.type().isPrimitiveType(), "Cannot add field %s as an identifier field: not a primitive type field", field.name()); Preconditions.checkArgument(field.isRequired(), diff --git a/core/src/test/java/org/apache/iceberg/TestSchemaUpdate.java b/core/src/test/java/org/apache/iceberg/TestSchemaUpdate.java index 4b0bed1cde48..bf96546a3634 100644 --- a/core/src/test/java/org/apache/iceberg/TestSchemaUpdate.java +++ b/core/src/test/java/org/apache/iceberg/TestSchemaUpdate.java @@ -1334,24 +1334,28 @@ public void testRemoveIdentifierFields() { @Test public void testSetIdentifierFieldsFails() { - Schema testSchema = new Schema( optional(1, "id", Types.IntegerType.get()), required(2, "float", Types.FloatType.get()), required(3, "double", Types.DoubleType.get()) ); + AssertHelpers.assertThrows("Creating schema with nonexistent identifier fieldId should fail", + IllegalArgumentException.class, + "Cannot add fieldId %d as an identifier field: field not exists", + () -> new Schema(testSchema.asStruct().fields(), ImmutableSet.of(999))); + AssertHelpers.assertThrows("Creating schema with optional identifier field should fail", IllegalArgumentException.class, "Cannot add field id as an identifier field: not a required field", () -> new Schema(testSchema.asStruct().fields(), ImmutableSet.of(1))); - AssertHelpers.assertThrows("Creating schema with optional identifier field should fail", + AssertHelpers.assertThrows("Creating schema with float identifier field should fail", IllegalArgumentException.class, "Cannot add field float as an identifier field: must not be float or double field", () -> new Schema(testSchema.asStruct().fields(), ImmutableSet.of(2))); - AssertHelpers.assertThrows("Creating schema with optional identifier field should fail", + AssertHelpers.assertThrows("Creating schema with double identifier field should fail", IllegalArgumentException.class, "Cannot add field double as an identifier field: must not be float or double field", () -> new Schema(testSchema.asStruct().fields(), ImmutableSet.of(3))); From 2ed28fea801b62afd6ba4da4690ea0751d73f7e7 Mon Sep 17 00:00:00 2001 From: Mingliang Liu Date: Tue, 22 Mar 2022 11:41:30 -0700 Subject: [PATCH 3/4] %d -> %s --- api/src/main/java/org/apache/iceberg/Schema.java | 2 +- core/src/test/java/org/apache/iceberg/TestSchemaUpdate.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/api/src/main/java/org/apache/iceberg/Schema.java b/api/src/main/java/org/apache/iceberg/Schema.java index 10097d2daece..da1f85b1a273 100644 --- a/api/src/main/java/org/apache/iceberg/Schema.java +++ b/api/src/main/java/org/apache/iceberg/Schema.java @@ -110,7 +110,7 @@ static void validateIdentifierField(int fieldId, Map Map idToParent) { Types.NestedField field = idToField.get(fieldId); Preconditions.checkArgument(field != null, - "Cannot add fieldId %d as an identifier field: field not exists", fieldId); + "Cannot add fieldId %s as an identifier field: field not exists", fieldId); Preconditions.checkArgument(field.type().isPrimitiveType(), "Cannot add field %s as an identifier field: not a primitive type field", field.name()); Preconditions.checkArgument(field.isRequired(), diff --git a/core/src/test/java/org/apache/iceberg/TestSchemaUpdate.java b/core/src/test/java/org/apache/iceberg/TestSchemaUpdate.java index bf96546a3634..fae4fa9880da 100644 --- a/core/src/test/java/org/apache/iceberg/TestSchemaUpdate.java +++ b/core/src/test/java/org/apache/iceberg/TestSchemaUpdate.java @@ -1342,7 +1342,7 @@ public void testSetIdentifierFieldsFails() { AssertHelpers.assertThrows("Creating schema with nonexistent identifier fieldId should fail", IllegalArgumentException.class, - "Cannot add fieldId %d as an identifier field: field not exists", + "Cannot add fieldId 999 as an identifier field: field not exists", () -> new Schema(testSchema.asStruct().fields(), ImmutableSet.of(999))); AssertHelpers.assertThrows("Creating schema with optional identifier field should fail", From e919cb32479c126079245f55e57888f9facd0089 Mon Sep 17 00:00:00 2001 From: Mingliang Liu Date: Tue, 22 Mar 2022 16:19:30 -0700 Subject: [PATCH 4/4] not exists -> does not exist --- api/src/main/java/org/apache/iceberg/Schema.java | 2 +- core/src/test/java/org/apache/iceberg/TestSchemaUpdate.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/api/src/main/java/org/apache/iceberg/Schema.java b/api/src/main/java/org/apache/iceberg/Schema.java index da1f85b1a273..876ac7193480 100644 --- a/api/src/main/java/org/apache/iceberg/Schema.java +++ b/api/src/main/java/org/apache/iceberg/Schema.java @@ -110,7 +110,7 @@ static void validateIdentifierField(int fieldId, Map Map idToParent) { Types.NestedField field = idToField.get(fieldId); Preconditions.checkArgument(field != null, - "Cannot add fieldId %s as an identifier field: field not exists", fieldId); + "Cannot add fieldId %s as an identifier field: field does not exist", fieldId); Preconditions.checkArgument(field.type().isPrimitiveType(), "Cannot add field %s as an identifier field: not a primitive type field", field.name()); Preconditions.checkArgument(field.isRequired(), diff --git a/core/src/test/java/org/apache/iceberg/TestSchemaUpdate.java b/core/src/test/java/org/apache/iceberg/TestSchemaUpdate.java index fae4fa9880da..3e0656cfecfb 100644 --- a/core/src/test/java/org/apache/iceberg/TestSchemaUpdate.java +++ b/core/src/test/java/org/apache/iceberg/TestSchemaUpdate.java @@ -1342,7 +1342,7 @@ public void testSetIdentifierFieldsFails() { AssertHelpers.assertThrows("Creating schema with nonexistent identifier fieldId should fail", IllegalArgumentException.class, - "Cannot add fieldId 999 as an identifier field: field not exists", + "Cannot add fieldId 999 as an identifier field: field does not exist", () -> new Schema(testSchema.asStruct().fields(), ImmutableSet.of(999))); AssertHelpers.assertThrows("Creating schema with optional identifier field should fail",