You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@iceberg.apache.org by sz...@apache.org on 2022/04/25 18:21:55 UTC
[iceberg] branch master updated: Core: Disallow setting fields nested in optional structs as identifier fields
This is an automated email from the ASF dual-hosted git repository.
szehon pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/iceberg.git
The following commit(s) were added to refs/heads/master by this push:
new 6188c778d Core: Disallow setting fields nested in optional structs as identifier fields
6188c778d is described below
commit 6188c778dffe0dc1dce75faa3dd24bfdaae1b630
Author: ZhongYujiang <42...@users.noreply.github.com>
AuthorDate: Tue Apr 26 02:21:48 2022 +0800
Core: Disallow setting fields nested in optional structs as identifier fields
---
api/src/main/java/org/apache/iceberg/Schema.java | 17 +++++-
.../main/java/org/apache/iceberg/SchemaUpdate.java | 10 +++-
.../java/org/apache/iceberg/TestSchemaUpdate.java | 70 ++++++++++++++++++----
3 files changed, 80 insertions(+), 17 deletions(-)
diff --git a/api/src/main/java/org/apache/iceberg/Schema.java b/api/src/main/java/org/apache/iceberg/Schema.java
index 876ac7193..987046a27 100644
--- a/api/src/main/java/org/apache/iceberg/Schema.java
+++ b/api/src/main/java/org/apache/iceberg/Schema.java
@@ -22,6 +22,7 @@ package org.apache.iceberg;
import java.io.Serializable;
import java.util.Arrays;
import java.util.Collection;
+import java.util.Deque;
import java.util.List;
import java.util.Locale;
import java.util.Map;
@@ -33,6 +34,7 @@ import org.apache.iceberg.relocated.com.google.common.collect.BiMap;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableBiMap;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.apache.iceberg.relocated.com.google.common.collect.Sets;
import org.apache.iceberg.relocated.com.google.common.primitives.Ints;
import org.apache.iceberg.types.Type;
@@ -119,13 +121,22 @@ public class Schema implements Serializable {
!Types.FloatType.get().equals(field.type()),
"Cannot add field %s as an identifier field: must not be float or double field", field.name());
- // check whether the nested field is in a chain of struct fields
+ // check whether the nested field is in a chain of required struct fields
+ // exploring from root for better error message for list and map types
Integer parentId = idToParent.get(field.fieldId());
+ Deque<Integer> deque = Lists.newLinkedList();
while (parentId != null) {
- Types.NestedField parent = idToField.get(parentId);
+ deque.push(parentId);
+ parentId = idToParent.get(parentId);
+ }
+
+ while (!deque.isEmpty()) {
+ Types.NestedField parent = idToField.get(deque.pop());
Preconditions.checkArgument(parent.type().isStructType(),
"Cannot add field %s as an identifier field: must not be nested in %s", field.name(), parent);
- parentId = idToParent.get(parent.fieldId());
+ Preconditions.checkArgument(parent.isRequired(),
+ "Cannot add field %s as an identifier field: must not be nested in an optional field %s",
+ field.name(), parent);
}
}
diff --git a/core/src/main/java/org/apache/iceberg/SchemaUpdate.java b/core/src/main/java/org/apache/iceberg/SchemaUpdate.java
index d62147cc2..753c7811c 100644
--- a/core/src/main/java/org/apache/iceberg/SchemaUpdate.java
+++ b/core/src/main/java/org/apache/iceberg/SchemaUpdate.java
@@ -439,12 +439,21 @@ class SchemaUpdate implements UpdateSchema {
Multimap<Integer, Move> moves,
Set<String> identifierFieldNames) {
// validate existing identifier fields are not deleted
+ Map<Integer, Integer> idToParent = TypeUtil.indexParents(schema.asStruct());
+
for (String name : identifierFieldNames) {
Types.NestedField field = schema.findField(name);
if (field != null) {
Preconditions.checkArgument(!deletes.contains(field.fieldId()),
"Cannot delete identifier field %s. To force deletion, " +
"also call setIdentifierFields to update identifier fields.", field);
+ Integer parentId = idToParent.get(field.fieldId());
+ while (parentId != null) {
+ Preconditions.checkArgument(!deletes.contains(parentId),
+ "Cannot delete field %s as it will delete nested identifier field %s",
+ schema.findField(parentId), field);
+ parentId = idToParent.get(parentId);
+ }
}
}
@@ -462,7 +471,6 @@ class SchemaUpdate implements UpdateSchema {
freshIdentifierFieldIds.add(nameToId.get(name));
}
- Map<Integer, Integer> idToParent = TypeUtil.indexParents(schema.asStruct());
Map<Integer, Types.NestedField> idToField = TypeUtil.indexById(struct);
freshIdentifierFieldIds.forEach(id -> Schema.validateIdentifierField(id, idToField, idToParent));
diff --git a/core/src/test/java/org/apache/iceberg/TestSchemaUpdate.java b/core/src/test/java/org/apache/iceberg/TestSchemaUpdate.java
index 3e0656cfe..00678c4ff 100644
--- a/core/src/test/java/org/apache/iceberg/TestSchemaUpdate.java
+++ b/core/src/test/java/org/apache/iceberg/TestSchemaUpdate.java
@@ -1261,17 +1261,24 @@ public class TestSchemaUpdate {
@Test
public void testAddNestedIdentifierFieldColumns() {
Schema newSchema = new SchemaUpdate(SCHEMA, SCHEMA_LAST_COLUMN_ID)
- .setIdentifierFields("preferences.feature1")
+ .allowIncompatibleChanges()
+ .addRequiredColumn("required_struct", Types.StructType.of(
+ Types.NestedField.required(SCHEMA_LAST_COLUMN_ID + 2, "field", Types.StringType.get())
+ ))
+ .apply();
+
+ newSchema = new SchemaUpdate(newSchema, SCHEMA_LAST_COLUMN_ID + 2)
+ .setIdentifierFields("required_struct.field")
.apply();
Assert.assertEquals("set existing nested field as identifier should succeed",
- Sets.newHashSet(newSchema.findField("preferences.feature1").fieldId()),
+ Sets.newHashSet(newSchema.findField("required_struct.field").fieldId()),
newSchema.identifierFieldIds());
newSchema = new SchemaUpdate(SCHEMA, SCHEMA_LAST_COLUMN_ID)
.allowIncompatibleChanges()
.addRequiredColumn("new", Types.StructType.of(
- Types.NestedField.required(SCHEMA_LAST_COLUMN_ID + 1, "field", Types.StringType.get())
+ Types.NestedField.required(SCHEMA_LAST_COLUMN_ID + 2, "field", Types.StringType.get())
))
.setIdentifierFields("new.field")
.apply();
@@ -1283,8 +1290,8 @@ public class TestSchemaUpdate {
newSchema = new SchemaUpdate(SCHEMA, SCHEMA_LAST_COLUMN_ID)
.allowIncompatibleChanges()
.addRequiredColumn("new", Types.StructType.of(
- Types.NestedField.required(SCHEMA_LAST_COLUMN_ID + 1, "field", Types.StructType.of(
- Types.NestedField.required(SCHEMA_LAST_COLUMN_ID + 2, "nested", Types.StringType.get())))))
+ Types.NestedField.required(SCHEMA_LAST_COLUMN_ID + 2, "field", Types.StructType.of(
+ Types.NestedField.required(SCHEMA_LAST_COLUMN_ID + 3, "nested", Types.StringType.get())))))
.setIdentifierFields("new.field.nested")
.apply();
@@ -1400,22 +1407,34 @@ public class TestSchemaUpdate {
.addRequiredColumn("col_float", Types.FloatType.get())
.addRequiredColumn("col_double", Types.DoubleType.get())
.addRequiredColumn("new", Types.StructType.of(
- Types.NestedField.required(SCHEMA_LAST_COLUMN_ID + 3, "fields", Types.ListType.ofOptional(
- SCHEMA_LAST_COLUMN_ID + 4, Types.StructType.of(
- Types.NestedField.required(SCHEMA_LAST_COLUMN_ID + 5, "nested", Types.StringType.get())
+ Types.NestedField.required(SCHEMA_LAST_COLUMN_ID + 4, "fields", Types.ListType.ofRequired(
+ SCHEMA_LAST_COLUMN_ID + 5, Types.StructType.of(
+ Types.NestedField.required(SCHEMA_LAST_COLUMN_ID + 6, "nested", Types.StringType.get())
))
)
))
- .addRequiredColumn("new_map", Types.MapType.ofRequired(SCHEMA_LAST_COLUMN_ID + 6, 11,
+ .addRequiredColumn("new_map", Types.MapType.ofRequired(SCHEMA_LAST_COLUMN_ID + 8, SCHEMA_LAST_COLUMN_ID + 9,
Types.StructType.of(
- required(SCHEMA_LAST_COLUMN_ID + 7, "key_col", Types.StringType.get())
+ required(SCHEMA_LAST_COLUMN_ID + 10, "key_col", Types.StringType.get())
),
Types.StructType.of(
- required(SCHEMA_LAST_COLUMN_ID + 8, "val_col", Types.StringType.get())
+ required(SCHEMA_LAST_COLUMN_ID + 11, "val_col", Types.StringType.get())
)), "map of address to coordinate")
+ .addRequiredColumn("required_list", Types.ListType.ofRequired(SCHEMA_LAST_COLUMN_ID + 13,
+ Types.StructType.of(
+ required(SCHEMA_LAST_COLUMN_ID + 14, "x", Types.LongType.get()),
+ required(SCHEMA_LAST_COLUMN_ID + 15, "y", Types.LongType.get())
+ )))
.apply();
- int lastColId = SCHEMA_LAST_COLUMN_ID + 8;
+ int lastColId = SCHEMA_LAST_COLUMN_ID + 15;
+
+ AssertHelpers.assertThrows("add a nested field in list should fail",
+ IllegalArgumentException.class,
+ "must not be nested in " + newSchema.findField("required_list"),
+ () -> new SchemaUpdate(newSchema, lastColId)
+ .setIdentifierFields("required_list.element.x")
+ .apply());
AssertHelpers.assertThrows("add a double field should fail",
IllegalArgumentException.class,
@@ -1438,12 +1457,19 @@ public class TestSchemaUpdate {
.setIdentifierFields("new_map.value.val_col")
.apply());
- AssertHelpers.assertThrows("add a nested field in struct of a map should fail",
+ AssertHelpers.assertThrows("add a nested field in struct of a list should fail",
IllegalArgumentException.class,
"must not be nested in " + newSchema.findField("new.fields"),
() -> new SchemaUpdate(newSchema, lastColId)
.setIdentifierFields("new.fields.element.nested")
.apply());
+
+ AssertHelpers.assertThrows("add a nested field in an optional struct should fail",
+ IllegalArgumentException.class,
+ "must not be nested in an optional field " + newSchema.findField("preferences"),
+ () -> new SchemaUpdate(newSchema, lastColId)
+ .setIdentifierFields("preferences.feature1")
+ .apply());
}
@Test
@@ -1478,6 +1504,24 @@ public class TestSchemaUpdate {
() -> new SchemaUpdate(schemaWithIdentifierFields, SCHEMA_LAST_COLUMN_ID).deleteColumn("id").apply());
}
+ @Test
+ public void testDeleteContainingNestedIdentifierFieldColumnsFails() {
+ Schema newSchema = new SchemaUpdate(SCHEMA, SCHEMA_LAST_COLUMN_ID)
+ .allowIncompatibleChanges()
+ .addRequiredColumn("out", Types.StructType.of(
+ Types.NestedField.required(SCHEMA_LAST_COLUMN_ID + 2, "nested", Types.StringType.get())
+ ))
+ .setIdentifierFields("out.nested")
+ .apply();
+
+ AssertHelpers.assertThrows(
+ "delete a struct with a nested identifier column without setting identifier fields should fail",
+ IllegalArgumentException.class,
+ "Cannot delete field 24: out: required struct<25: nested: required string> " +
+ "as it will delete nested identifier field 25: nested: required string",
+ () -> new SchemaUpdate(newSchema, SCHEMA_LAST_COLUMN_ID + 2).deleteColumn("out").apply());
+ }
+
@Test
public void testRenameIdentifierFields() {
Schema schemaWithIdentifierFields = new SchemaUpdate(SCHEMA, SCHEMA_LAST_COLUMN_ID)