kyluka/hive — pull request #4 by rachelgollub (2012-03-28 16:43:05+00:00)
Title: Hiverevision

Fixed negative tests that were failing because of changed error messages. Also responded to the following requests for changes:

> Since All the new introduced files deals with formatting output. I think they should be move to org.apache.hadoop.hive.ql.metadata.formatting package. Perhaps MetaDataFormatUtils.java should also be moved into this new package.

Done.

> Also, it would be good to add negative test cases in clientnegative for failure scenarios, like adding/dropping a table which doesn't exist.

These seemed to be covered by some of the existing negative tests, but we can always add more later.

> ql/src/java/org/apache/hadoop/hive/ql/metadata/MetaDataFormatter.java:23 Unused import.

Fixed.

> ql/src/java/org/apache/hadoop/hive/ql/metadata/MetaDataFormatter.java:40 Since there are only handful of them, will enum be a better choice here than ints?

I think ints are clearer here, since the names are just human readable cues for the actual int error messages.

> ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java:397 Are you sure you want stack-trace here? In case, table already exists you just want to say that to user, not the full stack trace.

Fixed.

> ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java:402 Are you sure you want stack-trace here? In case, db/table/partition doesnt exist you just want to say that to user, not the full stack trace.

Fixed.

> ql/src/java/org/apache/hadoop/hive/ql/metadata/MetaDataFormatter.java:38 Since all of the methods in the interface takes an output stream as an arg and writes to it, so interface is both formatter as well as writer. So, I wonder if better name for the interface is FormattedMetaDataWriter or something like that?

Wasn't really sure about this -- it's possible, but either one seemed to make sense, so I left it alone for now.

> ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java:1855 For showDatabases/showTables you have made changes even for these exceptions. To keep it symmetrical, it makes sense to have these messages also go through formatter.

Fixed.

> ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java:1858 For showDatabases/showTables you have made changes even for these exceptions. To keep it symmetrical, it makes sense to have these messages also go through formatter.

Fixed.

> ql/src/java/org/apache/hadoop/hive/ql/metadata/TextMetaDataFormatter.java:127 Whats the advantage of splitting it into two methods. You might as well inline content of this method here.

Fixed.

> ql/src/java/org/apache/hadoop/hive/ql/metadata/TextMetaDataFormatter.java:199 Whats the advantage of splitting it into two methods. You might as well inline content of this method here.

Fixed.

> ql/src/java/org/apache/hadoop/hive/ql/metadata/MapBuilder.java:28 This map can be parameterized to

Fixed.

> ql/src/java/org/apache/hadoop/hive/ql/metadata/JsonMetaDataFormatter.java:163 Return type should be List

Fixed.

> ql/src/java/org/apache/hadoop/hive/ql/metadata/JsonMetaDataFormatter.java:193 Genricize the return type.
> ql/src/java/org/apache/hadoop/hive/ql/metadata/JsonMetaDataFormatter.java:210 Genricize the return type.
> ql/src/java/org/apache/hadoop/hive/ql/metadata/JsonMetaDataFormatter.java:375 Genricize the return type.
> ql/src/java/org/apache/hadoop/hive/ql/metadata/JsonMetaDataFormatter.java:389 Genricize the return type.

Fixed.

> ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java:191 This config key should be defined into HiveConf.java and should be added in hive-site.xml.template. Also, instead of "hive.format" better name of this key would be "hive.ddl.output.format"

Both done.