Marcel Kornacker has posted comments on this change. Change subject: IMPALA-2840: Don't store table location in partition location ......................................................................
Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/2355/1/be/src/runtime/descriptors.cc File be/src/runtime/descriptors.cc: Line 152: location_((thrift_partition.location.is_relative ? thrift_table.hdfsBaseDir : "") + leave todo to use the same representation in the be http://gerrit.cloudera.org:8080/#/c/2355/1/fe/src/main/java/com/cloudera/impala/catalog/HdfsPartition.java File fe/src/main/java/com/cloudera/impala/catalog/HdfsPartition.java: Line 102: return suffix_.hashCode() ^ (is_relative_ ? 0xc6bfaf3a : 0x929b49e1); explain constants http://gerrit.cloudera.org:8080/#/c/2355/1/fe/src/main/java/com/cloudera/impala/catalog/HdfsTable.java File fe/src/main/java/com/cloudera/impala/catalog/HdfsTable.java: Line 178: private Map<HdfsPartition.Location, Map<String, FileDescriptor>> this seems artificial: if we already have a location, we could just stick that name-to-descriptor map in there; if we really just want to look it up by a particular string (either suffix or full) we should index by string. as it is, if you accidentally mix modes (some relative, some absolute), this won't work. Line 823: perPartitionFileDescMap_.get(new HdfsPartition.Location( move everything after '.get(' to new line Line 1378: perPartitionFileDescMap_.get(new HdfsPartition.Location(this, don't break up new call http://gerrit.cloudera.org:8080/#/c/2355/1/fe/src/test/java/com/cloudera/impala/planner/PlannerTestBase.java File fe/src/test/java/com/cloudera/impala/planner/PlannerTestBase.java: Line 228: private StringBuilder PrintScanRangeLocations(TQueryExecRequest execRequest) { while you're at it you might as well fix the name ('printScan...') http://gerrit.cloudera.org:8080/#/c/2355/1/tests/custom_cluster/test_catalog_mem_usage.py File tests/custom_cluster/test_catalog_mem_usage.py: Line 46: @CustomClusterTestSuite.with_args(jvm_args="-Xmx20m") > Tests like this are pretty likely to be flaky. I don't have a better soluti as you said, this will be flaky, so let's not test it this way. we should set up something along the lines of the perf regression tests, and include mem utilization. but that's beyond this patch. -- 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: 1 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
