Add support for field documentation. (#59)
authorRyan Blue <rdblue@users.noreply.github.com>
Thu, 20 Dec 2018 20:05:58 +0000 (12:05 -0800)
committerGitHub <noreply@github.com>
Thu, 20 Dec 2018 20:05:58 +0000 (12:05 -0800)
This adds an optional documentation string to the field level in Iceberg schemas.

Support for doc strings is added to Spark field conversion and to the UpdateSchema API.

To store field documentation, this adds a "doc" field to each nested field in the JSON representation of types. The spec has also been updated. This is a forward compatible change. Older readers will ignore the new doc field.

14 files changed:
api/src/main/java/com/netflix/iceberg/Schema.java
api/src/main/java/com/netflix/iceberg/UpdateSchema.java
api/src/main/java/com/netflix/iceberg/types/AssignFreshIds.java
api/src/main/java/com/netflix/iceberg/types/PruneColumns.java
api/src/main/java/com/netflix/iceberg/types/ReassignIds.java
api/src/main/java/com/netflix/iceberg/types/Types.java
core/src/main/java/com/netflix/iceberg/SchemaParser.java
core/src/main/java/com/netflix/iceberg/SchemaUpdate.java
core/src/main/java/com/netflix/iceberg/util/JsonUtil.java
core/src/test/java/com/netflix/iceberg/TestSchemaUpdate.java
core/src/test/java/com/netflix/iceberg/TestTableMetadataJson.java
core/src/test/java/com/netflix/iceberg/hadoop/HadoopTableTestBase.java
spark/src/main/java/com/netflix/iceberg/spark/SparkTypeToType.java
spark/src/main/java/com/netflix/iceberg/spark/TypeToSparkType.java

index 2ef872a..5662c12 100644 (file)
@@ -226,7 +226,7 @@ public class Schema implements Serializable {
   public String toString() {
     return String.format("table {\n%s\n}",
         NEWLINE.join(struct.fields().stream()
-            .map(f -> "  " + f)
+            .map(f -> "  " + f + (f.doc() == null ? "" : " COMMENT '" + f.doc() + "'"))
             .collect(Collectors.toList())));
   }
 }
index e1a83d0..70aac5e 100644 (file)
@@ -44,7 +44,26 @@ public interface UpdateSchema extends PendingUpdate<Schema> {
    * @return this for method chaining
    * @throws IllegalArgumentException If name contains "."
    */
-  UpdateSchema addColumn(String name, Type type);
+  default UpdateSchema addColumn(String name, Type type) {
+    return addColumn(name, type, null);
+  }
+
+  /**
+   * Add a new top-level column.
+   * <p>
+   * Because "." may be interpreted as a column path separator or may be used in field names, it is
+   * not allowed in names passed to this method. To add to nested structures or to add fields with
+   * names that contain ".", use {@link #addColumn(String, String, Type)}.
+   * <p>
+   * If type is a nested type, its field IDs are reassigned when added to the existing schema.
+   *
+   * @param name name for the new column
+   * @param type type for the new column
+   * @param doc documentation string for the new column
+   * @return this for method chaining
+   * @throws IllegalArgumentException If name contains "."
+   */
+  UpdateSchema addColumn(String name, Type type, String doc);
 
   /**
    * Add a new column to a nested struct.
@@ -66,7 +85,32 @@ public interface UpdateSchema extends PendingUpdate<Schema> {
    * @return this for method chaining
    * @throws IllegalArgumentException If parent doesn't identify a struct
    */
-  UpdateSchema addColumn(String parent, String name, Type type);
+  default UpdateSchema addColumn(String parent, String name, Type type) {
+    return addColumn(parent, name, type, null);
+  }
+
+  /**
+   * Add a new column to a nested struct.
+   * <p>
+   * The parent name is used to find the parent using {@link Schema#findField(String)}. If the
+   * parent name is null, the new column will be added to the root as a top-level column. If parent
+   * identifies a struct, a new column is added to that struct. If it identifies a list, the column
+   * is added to the list element struct, and if it identifies a map, the new column is added to
+   * the map's value struct.
+   * <p>
+   * The given name is used to name the new column and names containing "." are not handled
+   * differently.
+   * <p>
+   * If type is a nested type, its field IDs are reassigned when added to the existing schema.
+   *
+   * @param parent name of the parent struct to the column will be added to
+   * @param name name for the new column
+   * @param type type for the new column
+   * @param doc documentation string for the new column
+   * @return this for method chaining
+   * @throws IllegalArgumentException If parent doesn't identify a struct
+   */
+  UpdateSchema addColumn(String parent, String name, Type type, String doc);
 
   /**
    * Rename a column in the schema.
@@ -104,6 +148,43 @@ public interface UpdateSchema extends PendingUpdate<Schema> {
   UpdateSchema updateColumn(String name, Type.PrimitiveType newType);
 
   /**
+   * Update a column in the schema to a new primitive type.
+   * <p>
+   * The name is used to find the column to update using {@link Schema#findField(String)}.
+   * <p>
+   * Columns may be updated and renamed in the same schema update.
+   *
+   * @param name name of the column to rename
+   * @param newDoc replacement documentation string for the column
+   * @return this for method chaining
+   * @throws IllegalArgumentException If name doesn't identify a column in the schema or if this
+   *                                  change introduces a type incompatibility or if it conflicts
+   *                                  with other additions, renames, or updates.
+   */
+  UpdateSchema updateColumnDoc(String name, String newDoc);
+
+  /**
+   * Update a column in the schema to a new primitive type.
+   * <p>
+   * The name is used to find the column to update using {@link Schema#findField(String)}.
+   * <p>
+   * Only updates that widen types are allowed.
+   * <p>
+   * Columns may be updated and renamed in the same schema update.
+   *
+   * @param name name of the column to rename
+   * @param newType replacement type for the column
+   * @param newDoc replacement documentation string for the column
+   * @return this for method chaining
+   * @throws IllegalArgumentException If name doesn't identify a column in the schema or if this
+   *                                  change introduces a type incompatibility or if it conflicts
+   *                                  with other additions, renames, or updates.
+   */
+  default UpdateSchema updateColumn(String name, Type.PrimitiveType newType, String newDoc) {
+    return updateColumn(name, newType).updateColumnDoc(name, newDoc);
+  }
+
+  /**
    * Delete a column in the schema.
    * <p>
    * The name is used to find the column to delete using {@link Schema#findField(String)}.
index 2a35b04..0c5f65c 100644 (file)
@@ -53,9 +53,9 @@ class AssignFreshIds extends TypeUtil.CustomOrderSchemaVisitor<Type> {
       Types.NestedField field = fields.get(i);
       Type type = types.next();
       if (field.isOptional()) {
-        newFields.add(Types.NestedField.optional(newIds.get(i), field.name(), type));
+        newFields.add(Types.NestedField.optional(newIds.get(i), field.name(), type, field.doc()));
       } else {
-        newFields.add(Types.NestedField.required(newIds.get(i), field.name(), type));
+        newFields.add(Types.NestedField.required(newIds.get(i), field.name(), type, field.doc()));
       }
     }
 
index c4300e7..7df09bb 100644 (file)
@@ -53,10 +53,10 @@ class PruneColumns extends TypeUtil.SchemaVisitor<Type> {
         sameTypes = false; // signal that some types were altered
         if (field.isOptional()) {
           selectedFields.add(
-              Types.NestedField.optional(field.fieldId(), field.name(), projectedType));
+              Types.NestedField.optional(field.fieldId(), field.name(), projectedType, field.doc()));
         } else {
           selectedFields.add(
-              Types.NestedField.required(field.fieldId(), field.name(), projectedType));
+              Types.NestedField.required(field.fieldId(), field.name(), projectedType, field.doc()));
         }
       }
     }
index a81c2e1..4dccb74 100644 (file)
@@ -58,9 +58,9 @@ class ReassignIds extends TypeUtil.CustomOrderSchemaVisitor<Type> {
       Types.NestedField field = fields.get(i);
       int sourceFieldId = sourceStruct.field(field.name()).fieldId();
       if (field.isRequired()) {
-        newFields.add(Types.NestedField.required(sourceFieldId, field.name(), types.get(i)));
+        newFields.add(Types.NestedField.required(sourceFieldId, field.name(), types.get(i), field.doc()));
       } else {
-        newFields.add(Types.NestedField.optional(sourceFieldId, field.name(), types.get(i)));
+        newFields.add(Types.NestedField.optional(sourceFieldId, field.name(), types.get(i), field.doc()));
       }
     }
 
index f917a19..22111cf 100644 (file)
@@ -411,25 +411,35 @@ public class Types {
 
   public static class NestedField implements Serializable {
     public static NestedField optional(int id, String name, Type type) {
-      return new NestedField(true, id, name, type);
+      return new NestedField(true, id, name, type, null);
+    }
+
+    public static NestedField optional(int id, String name, Type type, String doc) {
+      return new NestedField(true, id, name, type, doc);
     }
 
     public static NestedField required(int id, String name, Type type) {
-      return new NestedField(false, id, name, type);
+      return new NestedField(false, id, name, type, null);
+    }
+
+    public static NestedField required(int id, String name, Type type, String doc) {
+      return new NestedField(false, id, name, type, doc);
     }
 
     private final boolean isOptional;
     private final int id;
     private final String name;
     private final Type type;
+    private final String doc;
 
-    private NestedField(boolean isOptional, int id, String name, Type type) {
+    private NestedField(boolean isOptional, int id, String name, Type type, String doc) {
       Preconditions.checkNotNull(name, "Name cannot be null");
       Preconditions.checkNotNull(type, "Type cannot be null");
       this.isOptional = isOptional;
       this.id = id;
       this.name = name;
       this.type = type;
+      this.doc = doc;
     }
 
     public boolean isOptional() {
@@ -452,10 +462,15 @@ public class Types {
       return type;
     }
 
+    public String doc() {
+      return doc;
+    }
+
     @Override
     public String toString() {
       return String.format("%d: %s: %s %s",
-          id, name, isOptional ? "optional" : "required", type);
+          id, name, isOptional ? "optional" : "required", type) +
+          (doc != null ? " (" + doc + ")" : "");
     }
 
     @Override
@@ -473,6 +488,8 @@ public class Types {
         return false;
       } else if (!name.equals(that.name)) {
         return false;
+      } else if (!Objects.equals(doc, that.doc)) {
+        return false;
       }
       return type.equals(that.type);
     }
index 2c89009..476459b 100644 (file)
@@ -44,6 +44,7 @@ public class SchemaParser {
   private static final String ELEMENT = "element";
   private static final String KEY = "key";
   private static final String VALUE = "value";
+  private static final String DOC = "doc";
   private static final String NAME = "name";
   private static final String ID = "id";
   private static final String ELEMENT_ID = "element-id";
@@ -65,6 +66,9 @@ public class SchemaParser {
       generator.writeBooleanField(REQUIRED, field.isRequired());
       generator.writeFieldName(TYPE);
       toJson(field.type(), generator);
+      if (field.doc() != null) {
+        generator.writeStringField(DOC, field.doc());
+      }
       generator.writeEndObject();
     }
     generator.writeEndArray();
@@ -185,11 +189,12 @@ public class SchemaParser {
       String name = JsonUtil.getString(NAME, field);
       Type type = typeFromJson(field.get(TYPE));
 
+      String doc = JsonUtil.getStringOrNull(DOC, field);
       boolean isRequired = JsonUtil.getBool(REQUIRED, field);
       if (isRequired) {
-        fields.add(Types.NestedField.required(id, name, type));
+        fields.add(Types.NestedField.required(id, name, type, doc));
       } else {
-        fields.add(Types.NestedField.optional(id, name, type));
+        fields.add(Types.NestedField.optional(id, name, type, doc));
       }
     }
 
index 45bdea1..65e7839 100644 (file)
@@ -31,6 +31,9 @@ import java.util.Collection;
 import java.util.List;
 import java.util.Map;
 
+import static com.netflix.iceberg.types.Types.NestedField.optional;
+import static com.netflix.iceberg.types.Types.NestedField.required;
+
 /**
  * Schema evolution API implementation.
  */
@@ -64,14 +67,14 @@ class SchemaUpdate implements UpdateSchema {
   }
 
   @Override
-  public UpdateSchema addColumn(String name, Type type) {
+  public UpdateSchema addColumn(String name, Type type, String doc) {
     Preconditions.checkArgument(!name.contains("."),
         "Cannot add column with ambiguous name: %s, use addColumn(parent, name, type)", name);
-    return addColumn(null, name, type);
+    return addColumn(null, name, type, doc);
   }
 
   @Override
-  public UpdateSchema addColumn(String parent, String name, Type type) {
+  public UpdateSchema addColumn(String parent, String name, Type type, String doc) {
     int parentId = TABLE_ROOT_ID;
     if (parent != null) {
       Types.NestedField parentField = schema.findField(parent);
@@ -102,8 +105,8 @@ class SchemaUpdate implements UpdateSchema {
 
     // assign new IDs in order
     int newId = assignNewColumnId();
-    adds.put(parentId, Types.NestedField.optional(newId, name,
-        TypeUtil.assignFreshIds(type, this::assignNewColumnId)));
+    adds.put(parentId, optional(newId, name,
+        TypeUtil.assignFreshIds(type, this::assignNewColumnId), doc));
 
     return this;
   }
@@ -126,6 +129,7 @@ class SchemaUpdate implements UpdateSchema {
   public UpdateSchema renameColumn(String name, String newName) {
     Types.NestedField field = schema.findField(name);
     Preconditions.checkArgument(field != null, "Cannot rename missing column: %s", name);
+    Preconditions.checkArgument(newName != null, "Cannot rename a column to null");
     Preconditions.checkArgument(!deletes.contains(field.fieldId()),
         "Cannot rename a column that will be deleted: %s", field.name());
 
@@ -133,9 +137,9 @@ class SchemaUpdate implements UpdateSchema {
     int fieldId = field.fieldId();
     Types.NestedField update = updates.get(fieldId);
     if (update != null) {
-      updates.put(fieldId, Types.NestedField.required(fieldId, newName, update.type()));
+      updates.put(fieldId, required(fieldId, newName, update.type(), update.doc()));
     } else {
-      updates.put(fieldId, Types.NestedField.required(fieldId, newName, field.type()));
+      updates.put(fieldId, required(fieldId, newName, field.type(), field.doc()));
     }
 
     return this;
@@ -154,9 +158,28 @@ class SchemaUpdate implements UpdateSchema {
     int fieldId = field.fieldId();
     Types.NestedField rename = updates.get(fieldId);
     if (rename != null) {
-      updates.put(fieldId, Types.NestedField.required(fieldId, rename.name(), newType));
+      updates.put(fieldId, required(fieldId, rename.name(), newType, rename.doc()));
+    } else {
+      updates.put(fieldId, required(fieldId, field.name(), newType, field.doc()));
+    }
+
+    return this;
+  }
+
+  @Override
+  public UpdateSchema updateColumnDoc(String name, String doc) {
+    Types.NestedField field = schema.findField(name);
+    Preconditions.checkArgument(field != null, "Cannot update missing column: %s", name);
+    Preconditions.checkArgument(!deletes.contains(field.fieldId()),
+        "Cannot update a column that will be deleted: %s", field.name());
+
+    // merge with a rename or update, if present
+    int fieldId = field.fieldId();
+    Types.NestedField update = updates.get(fieldId);
+    if (update != null) {
+      updates.put(fieldId, required(fieldId, update.name(), update.type(), doc));
     } else {
-      updates.put(fieldId, Types.NestedField.required(fieldId, field.name(), newType));
+      updates.put(fieldId, required(fieldId, field.name(), field.type(), doc));
     }
 
     return this;
@@ -231,17 +254,19 @@ class SchemaUpdate implements UpdateSchema {
 
         Types.NestedField field = struct.fields().get(i);
         String name = field.name();
+        String doc = field.doc();
         Types.NestedField update = updates.get(field.fieldId());
-        if (update != null && update.name() != null) {
+        if (update != null) {
           name = update.name();
+          doc = update.doc();
         }
 
         if (!name.equals(field.name()) || field.type() != resultType) {
           hasChange = true;
           if (field.isOptional()) {
-            newFields.add(Types.NestedField.optional(field.fieldId(), name, resultType));
+            newFields.add(optional(field.fieldId(), name, resultType, doc));
           } else {
-            newFields.add(Types.NestedField.required(field.fieldId(), name, resultType));
+            newFields.add(required(field.fieldId(), name, resultType, doc));
           }
         } else {
           newFields.add(field);
@@ -259,17 +284,20 @@ class SchemaUpdate implements UpdateSchema {
     @Override
     public Type field(Types.NestedField field, Type fieldResult) {
       // the API validates deletes, updates, and additions don't conflict
+      // handle deletes
       int fieldId = field.fieldId();
       if (deletes.contains(fieldId)) {
         return null;
       }
 
+      // handle updates
       Types.NestedField update = updates.get(field.fieldId());
       if (update != null && update.type() != field.type()) {
-        // rename is handled in struct
+        // rename is handled in struct, but struct needs the correct type from the field result
         return update.type();
       }
 
+      // handle adds
       Collection<Types.NestedField> newFields = adds.get(fieldId);
       if (newFields != null && !newFields.isEmpty()) {
         return addFields(fieldResult.asNestedType().asStructType(), newFields);
index cfb7fb0..68115f9 100644 (file)
@@ -73,6 +73,16 @@ public class JsonUtil {
     return pNode.asText();
   }
 
+  public static String getStringOrNull(String property, JsonNode node) {
+    if (!node.has(property)) {
+      return null;
+    }
+    JsonNode pNode = node.get(property);
+    Preconditions.checkArgument(pNode != null && !pNode.isNull() && pNode.isTextual(),
+        "Cannot parse %s from non-string value: %s", property, pNode);
+    return pNode.asText();
+  }
+
   public static Map<String, String> getStringMap(String property, JsonNode node) {
     Preconditions.checkArgument(node.has(property), "Cannot parse missing map %s", property);
     JsonNode pNode = node.get(property);
index 9e13d18..d548bd5 100644 (file)
@@ -41,7 +41,7 @@ public class TestSchemaUpdate {
       optional(3, "preferences", Types.StructType.of(
           required(8, "feature1", Types.BooleanType.get()),
           optional(9, "feature2", Types.BooleanType.get())
-      )),
+      ), "struct of named boolean options"),
       required(4, "locations", Types.MapType.ofRequired(10, 11,
           Types.StructType.of(
               required(20, "address", Types.StringType.get()),
@@ -52,19 +52,19 @@ public class TestSchemaUpdate {
           Types.StructType.of(
               required(12, "lat", Types.FloatType.get()),
               required(13, "long", Types.FloatType.get())
-          ))),
+          )), "map of address to coordinate"),
       optional(5, "points", Types.ListType.ofOptional(14,
           Types.StructType.of(
               required(15, "x", Types.LongType.get()),
               required(16, "y", Types.LongType.get())
-          ))),
+          )), "2-D cartesian points"),
       required(6, "doubles", Types.ListType.ofRequired(17,
           Types.DoubleType.get()
       )),
       optional(7, "properties", Types.MapType.ofOptional(18, 19,
           Types.StringType.get(),
           Types.StringType.get()
-      ))
+      ), "string map of properties")
   );
 
   private static final int SCHEMA_LAST_COLUMN_ID = 23;
@@ -104,7 +104,7 @@ public class TestSchemaUpdate {
         optional(3, "preferences", Types.StructType.of(
             required(8, "feature1", Types.BooleanType.get()),
             optional(9, "feature2", Types.BooleanType.get())
-        )),
+        ), "struct of named boolean options"),
         required(4, "locations", Types.MapType.ofRequired(10, 11,
             Types.StructType.of(
                 required(20, "address", Types.StringType.get()),
@@ -115,19 +115,19 @@ public class TestSchemaUpdate {
             Types.StructType.of(
                 required(12, "lat", Types.DoubleType.get()),
                 required(13, "long", Types.DoubleType.get())
-            ))),
+            )), "map of address to coordinate"),
         optional(5, "points", Types.ListType.ofOptional(14,
             Types.StructType.of(
                 required(15, "x", Types.LongType.get()),
                 required(16, "y", Types.LongType.get())
-            ))),
+            )), "2-D cartesian points"),
         required(6, "doubles", Types.ListType.ofRequired(17,
             Types.DoubleType.get()
         )),
         optional(7, "properties", Types.MapType.ofOptional(18, 19,
             Types.StringType.get(),
             Types.StringType.get()
-        ))
+        ), "string map of properties")
     );
 
     Schema updated = new SchemaUpdate(SCHEMA, SCHEMA_LAST_COLUMN_ID)
@@ -185,7 +185,7 @@ public class TestSchemaUpdate {
         optional(3, "options", Types.StructType.of(
             required(8, "feature1", Types.BooleanType.get()),
             optional(9, "newfeature", Types.BooleanType.get())
-        )),
+        ), "struct of named boolean options"),
         required(4, "locations", Types.MapType.ofRequired(10, 11,
             Types.StructType.of(
                 required(20, "address", Types.StringType.get()),
@@ -196,19 +196,19 @@ public class TestSchemaUpdate {
             Types.StructType.of(
                 required(12, "latitude", Types.FloatType.get()),
                 required(13, "long", Types.FloatType.get())
-            ))),
+            )), "map of address to coordinate"),
         optional(5, "points", Types.ListType.ofOptional(14,
             Types.StructType.of(
                 required(15, "X", Types.LongType.get()),
                 required(16, "y.y", Types.LongType.get())
-            ))),
+            )), "2-D cartesian points"),
         required(6, "doubles", Types.ListType.ofRequired(17,
             Types.DoubleType.get()
         )),
         optional(7, "properties", Types.MapType.ofOptional(18, 19,
             Types.StringType.get(),
             Types.StringType.get()
-        ))
+        ), "string map of properties")
     );
 
     Schema renamed = new SchemaUpdate(SCHEMA, SCHEMA_LAST_COLUMN_ID)
@@ -231,7 +231,7 @@ public class TestSchemaUpdate {
         optional(3, "preferences", Types.StructType.of(
             required(8, "feature1", Types.BooleanType.get()),
             optional(9, "feature2", Types.BooleanType.get())
-        )),
+        ), "struct of named boolean options"),
         required(4, "locations", Types.MapType.ofRequired(10, 11,
             Types.StructType.of(
                 required(20, "address", Types.StringType.get()),
@@ -243,21 +243,21 @@ public class TestSchemaUpdate {
                 required(12, "lat", Types.FloatType.get()),
                 required(13, "long", Types.FloatType.get()),
                 optional(25, "alt", Types.FloatType.get())
-            ))),
+            )), "map of address to coordinate"),
         optional(5, "points", Types.ListType.ofOptional(14,
             Types.StructType.of(
                 required(15, "x", Types.LongType.get()),
                 required(16, "y", Types.LongType.get()),
                 optional(26, "z", Types.LongType.get()),
                 optional(27, "t.t", Types.LongType.get())
-            ))),
+            )), "2-D cartesian points"),
         required(6, "doubles", Types.ListType.ofRequired(17,
             Types.DoubleType.get()
         )),
         optional(7, "properties", Types.MapType.ofOptional(18, 19,
             Types.StringType.get(),
             Types.StringType.get()
-        )),
+        ), "string map of properties"),
         optional(24, "toplevel", Types.DecimalType.of(9, 2))
     );
 
@@ -366,12 +366,12 @@ public class TestSchemaUpdate {
   @Test
   public void testMixedChanges() {
     Schema expected = new Schema(
-        required(1, "id", Types.LongType.get()),
+        required(1, "id", Types.LongType.get(), "unique id"),
         optional(2, "json", Types.StringType.get()),
         optional(3, "options", Types.StructType.of(
             required(8, "feature1", Types.BooleanType.get()),
             optional(9, "newfeature", Types.BooleanType.get())
-        )),
+        ), "struct of named boolean options"),
         required(4, "locations", Types.MapType.ofRequired(10, 11,
             Types.StructType.of(
                 required(20, "address", Types.StringType.get()),
@@ -380,16 +380,16 @@ public class TestSchemaUpdate {
                 required(23, "zip", Types.IntegerType.get())
             ),
             Types.StructType.of(
-                required(12, "latitude", Types.DoubleType.get()),
+                required(12, "latitude", Types.DoubleType.get(), "latitude"),
                 optional(25, "alt", Types.FloatType.get())
-            ))),
+            )), "map of address to coordinate"),
         optional(5, "points", Types.ListType.ofOptional(14,
             Types.StructType.of(
                 required(15, "X", Types.LongType.get()),
                 required(16, "y.y", Types.LongType.get()),
                 optional(26, "z", Types.LongType.get()),
-                optional(27, "t.t", Types.LongType.get())
-            ))),
+                optional(27, "t.t", Types.LongType.get(), "name with '.'")
+            )), "2-D cartesian points"),
         required(6, "doubles", Types.ListType.ofRequired(17,
             Types.DoubleType.get()
         )),
@@ -400,15 +400,16 @@ public class TestSchemaUpdate {
         .addColumn("toplevel", Types.DecimalType.of(9, 2))
         .addColumn("locations", "alt", Types.FloatType.get()) // map of structs
         .addColumn("points", "z", Types.LongType.get()) // list of structs
-        .addColumn("points", "t.t", Types.LongType.get()) // name with '.'
+        .addColumn("points", "t.t", Types.LongType.get(), "name with '.'")
         .renameColumn("data", "json")
         .renameColumn("preferences", "options")
         .renameColumn("preferences.feature2", "newfeature") // inside a renamed column
         .renameColumn("locations.lat", "latitude")
         .renameColumn("points.x", "X")
         .renameColumn("points.y", "y.y") // has a '.' in the field name
-        .updateColumn("id", Types.LongType.get())
+        .updateColumn("id", Types.LongType.get(), "unique id")
         .updateColumn("locations.lat", Types.DoubleType.get()) // use the original name
+        .updateColumnDoc("locations.lat", "latitude")
         .deleteColumn("locations.long")
         .deleteColumn("properties")
         .apply();
@@ -575,4 +576,30 @@ public class TestSchemaUpdate {
         }
     );
   }
+
+  @Test
+  public void testUpdateAddedColumnDoc() {
+    Schema schema = new Schema(required(1, "i", Types.IntegerType.get()));
+    AssertHelpers.assertThrows("Should reject add and update doc",
+        IllegalArgumentException.class, "Cannot update missing column", () -> {
+          new SchemaUpdate(schema, 3)
+              .addColumn("value", Types.LongType.get())
+              .updateColumnDoc("value", "a value")
+              .apply();
+        }
+    );
+  }
+
+  @Test
+  public void testUpdateDeletedColumnDoc() {
+    Schema schema = new Schema(required(1, "i", Types.IntegerType.get()));
+    AssertHelpers.assertThrows("Should reject add and update doc",
+        IllegalArgumentException.class, "Cannot update a column that will be deleted", () -> {
+          new SchemaUpdate(schema, 3)
+              .deleteColumn("i")
+              .updateColumnDoc("i", "a value")
+              .apply();
+        }
+    );
+  }
 }
index 0b04fac..e21b4b6 100644 (file)
@@ -60,7 +60,7 @@ public class TestTableMetadataJson {
   public void testJsonConversion() throws Exception {
     Schema schema = new Schema(
         Types.NestedField.required(1, "x", Types.LongType.get()),
-        Types.NestedField.required(2, "y", Types.LongType.get()),
+        Types.NestedField.required(2, "y", Types.LongType.get(), "comment"),
         Types.NestedField.required(3, "z", Types.LongType.get())
     );
 
index 683a0b2..4d854d7 100644 (file)
@@ -47,18 +47,18 @@ import static com.netflix.iceberg.types.Types.NestedField.required;
 public class HadoopTableTestBase {
   // Schema passed to create tables
   static final Schema SCHEMA = new Schema(
-      required(3, "id", Types.IntegerType.get()),
+      required(3, "id", Types.IntegerType.get(), "unique ID"),
       required(4, "data", Types.StringType.get())
   );
 
   // This is the actual schema for the table, with column IDs reassigned
   static final Schema TABLE_SCHEMA = new Schema(
-      required(1, "id", Types.IntegerType.get()),
+      required(1, "id", Types.IntegerType.get(), "unique ID"),
       required(2, "data", Types.StringType.get())
   );
 
   static final Schema UPDATED_SCHEMA = new Schema(
-      required(1, "id", Types.IntegerType.get()),
+      required(1, "id", Types.IntegerType.get(), "unique ID"),
       required(2, "data", Types.StringType.get()),
       optional(3, "n", Types.IntegerType.get())
   );
index 99219be..2642f40 100644 (file)
@@ -80,10 +80,12 @@ class SparkTypeToType extends SparkTypeVisitor<Type> {
         id = getNextId();
       }
 
+      String doc = field.getComment().isDefined() ? field.getComment().get() : null;
+
       if (field.nullable()) {
-        newFields.add(Types.NestedField.optional(id, field.name(), type));
+        newFields.add(Types.NestedField.optional(id, field.name(), type, doc));
       } else {
-        newFields.add(Types.NestedField.required(id, field.name(), type));
+        newFields.add(Types.NestedField.required(id, field.name(), type, doc));
       }
     }
 
index f2768f1..7f467c1 100644 (file)
@@ -59,7 +59,12 @@ class TypeToSparkType extends TypeUtil.SchemaVisitor<DataType> {
     for (int i = 0; i < fields.size(); i += 1) {
       Types.NestedField field = fields.get(i);
       DataType type = fieldResults.get(i);
-      sparkFields.add(StructField.apply(field.name(), type, field.isOptional(), Metadata.empty()));
+      StructField sparkField = StructField.apply(
+          field.name(), type, field.isOptional(), Metadata.empty());
+      if (field.doc() != null) {
+        sparkField = sparkField.withComment(field.doc());
+      }
+      sparkFields.add(sparkField);
     }
 
     return StructType$.MODULE$.apply(sparkFields);