Jim Apple has posted comments on this change. Change subject: IMPALA-2840: Don't store table location in partition location ......................................................................
Patch Set 5: (7 comments) http://gerrit.cloudera.org:8080/#/c/2355/5/be/src/runtime/descriptors.cc File be/src/runtime/descriptors.cc: Line 38: namespace > Out of curiosity, why don't you put this function under impala namespace? To keep from polluting the namespace (when linking, since during compiling it would be invisible whether in impala:: or an unnamed namespace. Line 41: // format that references the 'partition_prefixes' of 'thrift_table'. > Can you also comment on the return value when location isn't set? Done http://gerrit.cloudera.org:8080/#/c/2355/5/common/thrift/CatalogObjects.thrift File common/thrift/CatalogObjects.thrift: Line 223: HDFS partition's location > There is no reason why we can't use this to compress other locations as wel Done in HdfsPartitionLocationCompressor.java class comment. http://gerrit.cloudera.org:8080/#/c/2355/5/fe/src/main/java/com/cloudera/impala/catalog/HdfsPartitionLocationCompressor.java File fe/src/main/java/com/cloudera/impala/catalog/HdfsPartitionLocationCompressor.java: Line 28: This class is not thread-safe. Clients of this class need to protect against : * concurrent updates using external locking. : * : * Owned by HdfsTable instance. > Since this is owned by HdfsTable and the catalog uses table-level locking, Done Line 35: numClusteringColumns_ > This doesn't seem to be used anywhere besides getter and setter. Why storin It is also used in decomopose(). Line 39: indexToPrefix > This is a bit misleading as it sort of indicates that this is a map. Maybe Changed to use ListMap, an Impala-specific bidi map from T to int/Integer and back. Line 88: public class Location > How about the following idea for organizing this code: What I don't like about this is that it spreads the compression code out into three different places. One thing we could do, once we start using prefix compression in more than one place, is make this class more generic, parameterizing by an interface that has a decompose method. I'd prefer to roll that up into the TODO at the top of this class about using this compression method for other sets of Strings that are likely to share prefixes. What do you think? -- To view, visit http://gerrit.cloudera.org:8080/2355 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8c67b6ce0f83de2f5277a528a9ce67e47d638adb Gerrit-PatchSet: 5 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Jim Apple <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
