Alex Behm has posted comments on this change. Change subject: IMPALA-1702: Enforce table level consistency accross service ......................................................................
Patch Set 6: (12 comments) So much better! http://gerrit.cloudera.org:8080/#/c/4349/6/common/thrift/Types.thrift File common/thrift/Types.thrift: Line 22: typedef i64 TTableId > now i32 is ok I think. We can also move this typedef into a more appropriate place like Descriptors.thrift because nobody else should care about it now. http://gerrit.cloudera.org:8080/#/c/4349/6/fe/src/main/java/org/apache/impala/analysis/DescriptorTable.java File fe/src/main/java/org/apache/impala/analysis/DescriptorTable.java: Line 50: static public final long TABLE_SINK_ID = 0; I know it doesn't matter but we typically do "public static final ..." Line 52: private long nextTableId = TABLE_SINK_ID + 1; nextTableId_ change to int? Line 54: private long getNextTableId() { return nextTableId++; } I think we can just remove this and inline at the single caller Line 158: // tableIdMap.get(table) == null iff (table == null || table instanceof View) Maps from base table to its table id. Line 165: if (tupleDesc.isMaterialized()) { Lots of nesting here, can we invert this condition, i.e. if (!tupleDesc.isMaterialized()) continue; Line 166: Table table = tupleDesc.getTable(); Remember the tableId here so avoid some hash lookups. Long tableId = null; if (table != null && !(table instanceof View)) { ... } Line 168: if (!tableIdMap.containsKey(table)) { Use this pattern to avoid multiple lookups: Entry e = map,.get(); if (e == null) { e = nextid; map.put(); } http://gerrit.cloudera.org:8080/#/c/4349/6/fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java File fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java: Line 198: public TTupleDescriptor toThrift(Long assignedTableId) { just tableId http://gerrit.cloudera.org:8080/#/c/4349/6/fe/src/main/java/org/apache/impala/catalog/HBaseTable.java File fe/src/main/java/org/apache/impala/catalog/HBaseTable.java: Line 665: public TTableDescriptor toThriftDescriptor(long id, Set<Long> referencedPartitions) { tableId http://gerrit.cloudera.org:8080/#/c/4349/6/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: Line 1567: public TTableDescriptor toThriftDescriptor(long id, Set<Long> referencedPartitions) { tableId http://gerrit.cloudera.org:8080/#/c/4349/6/fe/src/main/java/org/apache/impala/catalog/IncompleteTable.java File fe/src/main/java/org/apache/impala/catalog/IncompleteTable.java: Line 68: public TTableDescriptor toThriftDescriptor(long id, Set<Long> referencedPartitions) { tableId (and elsewhere) -- To view, visit http://gerrit.cloudera.org:8080/4349 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifad648b72684ae495ec387590ab1bc58ce5b39e2 Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Huaisi Xu <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Huaisi Xu <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
