Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10611 )
Change subject: IMPALA-7128 (part 1) Refactor interfaces for Db, View, Table, Partition ...................................................................... Patch Set 2: (4 comments) Responded to the comments that might merit discussion. I'll work on addressing the nits, etc. http://gerrit.cloudera.org:8080/#/c/10611/2/fe/src/main/java/org/apache/impala/catalog/FeDb.java File fe/src/main/java/org/apache/impala/catalog/FeDb.java: http://gerrit.cloudera.org:8080/#/c/10611/2/fe/src/main/java/org/apache/impala/catalog/FeDb.java@31 PS2, Line 31: @return > Both styles exist (javadoc or not) in the code base, but would be preferabl k, will switch to javadoc. I think the lack of consistency here is because some comments were copy-pasted whereas others were written anew :) http://gerrit.cloudera.org:8080/#/c/10611/2/fe/src/main/java/org/apache/impala/catalog/FeHdfsTable.java File fe/src/main/java/org/apache/impala/catalog/FeHdfsTable.java: http://gerrit.cloudera.org:8080/#/c/10611/2/fe/src/main/java/org/apache/impala/catalog/FeHdfsTable.java@36 PS2, Line 36: Note: despite the "HDFS" nomenclature, this table type is also used for : * interacting with S3 or other Hadoop-compatible filesystems. > yup! shall we adopt the more general, preferred naming now? I had tried to just be consistent that 'Foo' class turned into 'FeFoo' interface, but if you think it's a better idea to bite the bullet and switch now I'm happy to do so. http://gerrit.cloudera.org:8080/#/c/10611/2/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java File fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java: http://gerrit.cloudera.org:8080/#/c/10611/2/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@964 PS2, Line 964: KeyValueComparator > this change can introduce differences depending on how partitions are dealt Partitions are used in Collections.sort, which requires the class to be comparable to itself. Previously HdfsPartition was Comparable<HdfsPartition>, but with an interface, it becomes a bit trickier. One approach is to make FeHdfsPartition extend Comparable<FeHdfsPartition>, but then all implementation classes need to implement compareTo(). And in fact all implementation classes will implement it exactly the same (ie using comparePartitionKeyValues, which only relies on the method that's part of the interface). If we were on Java 8, we could use a default method in the FeHdfsPartition interface to do this, but since we have to be Java 7-compatible, we can't do this. Given that, I felt like it was better to stop using the Comparable interface and instead use an explicit Comparator class. Regarding the concern about behavior differences in collections, I answered a similar one elsewhere but just to re-iterate here: I don't believe there are any collections in Java that would differ in behavior based on whether an object implements Comparable, unless they explicitly restrict themselves to be used with a Comparable object at compile-time. i.e any collections relying on Comparable would fail to compile (rather than change behavior) when I removed the Comparable interface from HdfsPartition. I did a quick check of the above by grepping the JDK source for 'instanceof Comparable' and didn't find any in any collection classes. http://gerrit.cloudera.org:8080/#/c/10611/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: http://gerrit.cloudera.org:8080/#/c/10611/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@2016 PS2, Line 2016: HdfsPartition.KV_COMPARATOR > how did you find all places to make this type of update, Collections.sort o The Collections.sort(List<T>) signature only matches if T implements Comparable, so when I removed the Comparable interface from the HdfsPartition class, I got compilation errors at all the call sites where it was relying on it. The same would be true of other things like TreeMap<HdfsPartition,X> -- it's not possible to instantiate one without either the class being Comparable or by providing a Comparator. -- To view, visit http://gerrit.cloudera.org:8080/10611 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id55f7d2e94d81e66ce720acb6315f15a89621b31 Gerrit-Change-Number: 10611 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Thu, 07 Jun 2018 21:50:28 +0000 Gerrit-HasComments: Yes
