[GitHub] [iceberg] pvary commented on a change in pull request #1842: Hive: Refactor Hive Iceberg schema conversions to a single file

[GitBox <gi...@apache.org>]

pvary commented on a change in pull request #1842:
URL: https://github.com/apache/iceberg/pull/1842#discussion_r537041640



##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTypeConverter.java
##########
@@ -30,7 +37,52 @@ private HiveTypeConverter() {
 
   }
 
-  public static String convert(Type type) {
+  /**
+   * Converts the Iceberg schema to a Hive schema.
+   * @param schema The original Iceberg schema to convert
+   * @return The Hive column list generated from the Iceberg schema
+   */
+  public static List<FieldSchema> hiveSchema(Schema schema) {

Review comment:
       Thanks @rdblue for the review!
   
   Renamed the class to HiveSchemaUtil and the methods to `convert`. Also added `convert` methods for Type conversion.
   Kept the `HiveSchemaConverter` class for now for 2 reasons:
   - We have to extend the actual converter for Hive3 (Timestamp with local timezone is a separate type in Hive 3)
   - You suggested to implement a Hive type visitor on another review - we might want to use that for conversions when it is ready
   
   Until we have a final solution I added a javadoc where specifically stated that this package private class should not be used anywhere for conversion and pointed to the correct class to use.
   
   What do you think? Would this be good enough for start?
   
   Thanks,
   Peter




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org