rdsr commented on a change in pull request #2086:
URL: https://github.com/apache/iceberg/pull/2086#discussion_r556727997



##########
File path: 
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveSchemaConverter.java
##########
@@ -110,7 +111,7 @@ Type convertType(TypeInfo typeInfo) {
       case STRUCT:
         StructTypeInfo structTypeInfo = (StructTypeInfo) typeInfo;
         List<Types.NestedField> fields =
-            convertInternal(structTypeInfo.getAllStructFieldNames(), 
structTypeInfo.getAllStructFieldTypeInfos());
+            convertInternal(structTypeInfo.getAllStructFieldNames(), 
structTypeInfo.getAllStructFieldTypeInfos(), null);

Review comment:
       Should the comment parameter be an empty list instead of null? This may 
remove various null checks in the code e.g in `convertInternal` above
   Also, for an empty struct I believe `fieldnames` and `typeinfos` will be 
empty instead of null so it maybe more consistent for it to be empty.

##########
File path: mr/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergSerDe.java
##########
@@ -137,15 +137,21 @@ private static Schema hiveSchemaOrThrow(Properties 
serDeProperties, Exception pr
     // Read the configuration parameters
     String columnNames = 
serDeProperties.getProperty(serdeConstants.LIST_COLUMNS);
     String columnTypes = 
serDeProperties.getProperty(serdeConstants.LIST_COLUMN_TYPES);
+    // No constant for column comments and column comments delimiter.

Review comment:
       not sure I follow this comment..




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[email protected]



---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to