Huaisi Xu has posted comments on this change. Change subject: IMPALA-1702: Enforce table level consistency accross service ......................................................................
Patch Set 7: (16 comments) http://gerrit.cloudera.org:8080/#/c/4349/6/common/thrift/Types.thrift File common/thrift/Types.thrift: Line 22: typedef i32 TPlanNodeId > We can also move this typedef into a more appropriate place like Descriptor I think it is ok to push it here to stick with others. just make it an int. http://gerrit.cloudera.org:8080/#/c/4349/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: Line 70: import org.apache.impala.util.DisjointSet; > will remove Done 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: public static final int TABLE_SINK_ID = 0; > I know it doesn't matter but we typically do "public static final ..." Done Line 52: private int nextTableId_ = TABLE_SINK_ID + 1; > nextTableId_ Done Line 54: public TupleDescriptor createTupleDescriptor(String debugName) { > I think we can just remove this and inline at the single caller Done Line 158: if (targetTable_ != null) tableIdMap.put(targetTable_, TABLE_SINK_ID); > Maps from base table to its table id. Done Line 165: Integer tableId = tableIdMap.get(table); > Lots of nesting here, can we invert this condition, i.e. Done Line 166: if (table != null && !(table instanceof View) && tableId == null) { > Remember the tableId here so avoid some hash lookups. Done Line 168: tableIdMap.put(table, tableId); > Use this pattern to avoid multiple lookups: Done 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(Integer tableId) { > just tableId Done http://gerrit.cloudera.org:8080/#/c/4349/6/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: Line 34: import java.util.concurrent.locks.ReentrantReadWriteLock; > will remove Done 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(int tableId, Set<Long> referencedPartitions) { > tableId Done 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(int tableId, Set<Long> referencedPartitions) { > tableId Done 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(int tableId, Set<Long> referencedPartitions) { > tableId Done http://gerrit.cloudera.org:8080/#/c/4349/6/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: Line 38: import org.apache.impala.catalog.KuduTable; > not intentional.. will revert next patch! Done PS6, Line 1105: String db = insertStmt.getTargetTableName().getDb(); : finalizeParams.setTable_db(db == null ? queryCtx.session.database : db); : HdfsTable hdfsTable = (HdfsTable) insertStmt.getTargetTable(); : finalizeParams.setHdfs_base_dir(hdfsTable.getHdfsBaseDir()); : finalizeParams.setStaging_dir( : hdfsTable.getHdfsBaseDir() + "/_impala_insert_staging"); : > will update Done -- 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: 7 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
