kevinwilfong has commented on the revision "
HIVE-2530 [jira] Implement SHOW TBLPROPERTIES".
Looks good, just a few comments.
ql/src/java/org/apache/hadoop/hive/ql/metadata/Table.java:370 The Table class already has a getParameters method which does exactly this. While I agree that the inconsistencies between the uses of the words Parameters and Properties throughout the code is confusing, could you either call that method instead or at least have this method return the result of getParameters(), which would at least help to keep them consistent going forward.
ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java:2573 Given that when this feature was requested, very specific guidelines were provided for how to format the data (one per row, tabs between property and value) it looks like this is wanted to make it easier to parse the properties from a query. Adding this line will make that a little more difficult.
In addition, there is a precedent for not adding a line like this in commands like SHOW PARTITIONS and DESCRIBE which list the partitions and columns respectively without a descriptive line like this.
ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java:2526-2527 I'm guessing you got this from the implementation of DESCRIBE TABLE?
The reasoning for having that line there was so that you could describe "the types within the column in the case when the column is an object"
That shouldn't apply to this method, so you should be able to assume that the table name is the table name, without this parsing.
ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java:2533-2542 Could you put this code in the try catch block below. The stringifyException method prints a stack trace so developers will be able to figure out if the table was null or not. The catch code looks identical, so this will help remove some duplicate code.