Github user maugly24 commented on a diff in the pull request:

    https://github.com/apache/sqoop/pull/60#discussion_r237701227
  
    --- Diff: src/java/org/apache/sqoop/hive/HiveTypes.java ---
    @@ -37,16 +42,28 @@
       private static final String HIVE_TYPE_STRING = "STRING";
       private static final String HIVE_TYPE_BOOLEAN = "BOOLEAN";
       private static final String HIVE_TYPE_BINARY = "BINARY";
    +  private static final String HIVE_TYPE_DECIMAL = "DECIMAL";
     
       public static final Log LOG = 
LogFactory.getLog(HiveTypes.class.getName());
     
       private HiveTypes() { }
     
    +
    +  public static String toHiveType(int sqlType, SqoopOptions options) {
    +
    +    if 
(options.getConf().getBoolean(ConfigurationConstants.PROP_ENABLE_PARQUET_LOGICAL_TYPE_DECIMAL,
 false)
    +        && (sqlType == Types.NUMERIC || sqlType == Types.DECIMAL)){
    +      return HIVE_TYPE_DECIMAL;
    +    }
    +    return toHiveType(sqlType);
    +  }
    +
    +
       /**
        * Given JDBC SQL types coming from another database, what is the best
        * mapping to a Hive-specific type?
        */
    -  public static String toHiveType(int sqlType) {
    +  private static String toHiveType(int sqlType) {
    --- End diff --
    
    If you touch this class+method anyway would you mind modifying the 
underlying design in a way not to return null value?
    
    It would be quite error prone e.g. in case of TableDefWriter if the 
FileLayout is Parquet and by any mistake the type of a given column is not 
mapped then the execution path would not end up in "not supported" exception, 
but TableDefWriter would try to create the table with "NULL" datatype.
    
    IMHO this would be a great improvement here.
    
    I would also standardize it in a way not to throw IOException (e.g. 
getHiveColumnTypeForTextTable) and RuntimeException for the same problem, but 
go only with RuntimeException.


---

Reply via email to