Jim Apple has posted comments on this change. Change subject: IMPALA-2840: Don't store table location in partition location ......................................................................
Patch Set 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/2355/1//COMMIT_MSG Commit Message: Line 7: IMPALA-2840: Don't store table location in partition location > leave a todo Done 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 Put in the header. 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 Done. 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 t Do you mean that the value type of perPartitionFileDescMap_, a Map<String, FileDescriptor> mapping file names to file descriptors, could just be put in HdfsPartition? If so, I see your point, but I think that change would be more lines of code changed than the part of this patch to do the prefix-factoring. I'd like to add a TODO and do it in a follow-up patch. Thoughts? I'm not sure if mode mixing is possible here: HdfsPartition locations are constructed from absolute paths combined with HdfsTable objects, so the determination of whether to use absolute or relative paths is delegated there. Line 823: perPartitionFileDescMap_.get(new HdfsPartition.Location( > move everything after '.get(' to new line Done Line 1378: perPartitionFileDescMap_.get(new HdfsPartition.Location(this, > don't break up new call Done 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...') Done 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") > as you said, this will be flaky, so let's not test it this way. we should s Done -- 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
