Jim Apple has posted comments on this change. Change subject: IMPALA-2840: Don't store table location in partition location ......................................................................
Patch Set 2: (12 comments) http://gerrit.cloudera.org:8080/#/c/2355/2/common/thrift/CatalogObjects.thrift File common/thrift/CatalogObjects.thrift: Line 225: protion > portion Done 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 73: private boolean is_relative_; : private String suffix_; > Make them both final Done Line 76: HdfsTable table > That sounds like a good enough solution to me, especially if we can avoid a Done Line 86: Location(THdfsPartitionLocation thrift) > nit: we typically have static fromThrift() functions. In this case, doing so would require creating a (private) constructor that directly takes a (prefix_,suffix_) pair, which I think is easier to abuse than a thrift constructor. Line 119: public int compareTo(Location o) { > Can you add a comment on the imposed order (relative last, etc)? Done Line 445: Location getRawLocation() > There is a minor issue with this. I can call getRawLocation() on a partitio Done, but to do so, it made the arch much cleaner to move the class to HdfsTable. Line 635: > nit: why the empty lines? Done Line 764: ? new Location(thriftPartition.getLocation()) : : null; > nit: can you merge into a single line? Not without going beyond 90 characters Line 811: if (location_ != null) { : thriftHdfsPart.setLocation(location_.toThrift()); : } > nit: if possible, single line Done 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?) clang-format things, based on ContinuationIndentWidth, that if the first line is indented four spaces, the remaining ones should be as well. The code has changed now. It looks to me like it now matches the "Java Style Guide" on our wiki. Line 1379: new HdfsPartition.Location(this, partDirPath.toString())).values()); > same here The object creation is unfortunate. I think it will be fixed when we move perPartitionFileDescMap_ to the partitions themselves. http://gerrit.cloudera.org:8080/#/c/2355/2/fe/src/test/java/com/cloudera/impala/planner/PlannerTestBase.java File fe/src/test/java/com/cloudera/impala/planner/PlannerTestBase.java: Line 245: THdfsTable table = findTable(entry.getKey()); : THdfsPartition partition = table.getPartitions().get(split.partition_id); > Isn't this equivalent to findPartition()? We need the table in order to construct file_location. -- 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-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
