Marcel Kornacker has posted comments on this change. Change subject: IMPALA-2840: Don't store table location in partition location ......................................................................
Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/2355/2/fe/src/main/java/com/cloudera/impala/catalog/HdfsPartition.java File fe/src/main/java/com/cloudera/impala/catalog/HdfsPartition.java: Line 77: is_relative_ = (table != null) && (location != null) is there a case for allowing absolute locations? i think both table and location should always be != null. (why would HdfsTable need partitions w/o a parent table or location?) (aside from that: if you used both relative and absolute locations in the same map as keys, you might end up with duplicate entries without realizing it, at least with the way you hash in this class.) http://gerrit.cloudera.org:8080/#/c/2355/2/fe/src/main/java/com/cloudera/impala/catalog/HdfsTable.java File fe/src/main/java/com/cloudera/impala/catalog/HdfsTable.java: Line 824: new HdfsPartition.Location(this, partDirPath.toString())).values(), indent this 2 spaces (doesn't your formatter do that automatically?) Line 1379: new HdfsPartition.Location(this, partDirPath.toString())).values()); same here hm, a lot of object creation for what amounts to getting a hash value for a suffix string. -- 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: 2 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-HasComments: Yes
