Huaisi Xu has posted comments on this change. Change subject: IMPALA-1702: Enforce table level consistency accross service ......................................................................
Patch Set 4: (11 comments) http://gerrit.cloudera.org:8080/#/c/4349/4/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: Line 294: // Catalog cache has the latest tables from the statestore, in order to use the > The Impalad Catalog has ... Done Line 295: // same table version in a single query, we cache all referenced tables and reuse them > ... query , we cache all referenced tables here. Done Line 2299: if (table == null) { > remove Done http://gerrit.cloudera.org:8080/#/c/4349/4/fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java File fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java: Line 187: // SelectStmt (or the BE will be very confused). Assign a unique table ID to it. > The previous longer comment was clearer. How about: Done http://gerrit.cloudera.org:8080/#/c/4349/4/fe/src/main/java/org/apache/impala/analysis/DescriptorTable.java File fe/src/main/java/org/apache/impala/analysis/DescriptorTable.java: Line 44: // List of tables to exclude from partition pruning, and thus all partitions are sent to > Afaik, these tables have nothing to do with partition pruning, so the name The only place that this actually becomes useful is for partition pruning? so for "INSERT INTO a PARTITION(b='d') select c from a; if c is in another partition other than partition 'b', then without this partition b's metadata wont be sent. it has no other meaningful usage than this I think. maybe I just change this to a HashSet? Line 181: referencedPartitions = getReferencedPartitions(tbl); > will remove this Done http://gerrit.cloudera.org:8080/#/c/4349/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: Line 141: * Ever increasing counter for table id {@link Table#id_}. New id is given when > TableId doesn't exist anymore. Done http://gerrit.cloudera.org:8080/#/c/4349/4/fe/src/main/java/org/apache/impala/catalog/Table.java File fe/src/main/java/org/apache/impala/catalog/Table.java: Line 64: private static final Object metastoreAccessLock_ = new Object(); > Please remove while you are here. remove which? ?removed INVALID_TABLE_ID. not sure if this is what you wanted. Line 72: public static final long LOCAL_TABLE_ID = -4; > LOCAL_VIEW_ID is more explicit. Sorry to make you change it again. Done http://gerrit.cloudera.org:8080/#/c/4349/4/fe/src/main/java/org/apache/impala/planner/JoinTableId.java File fe/src/main/java/org/apache/impala/planner/JoinTableId.java: Line 25: // Construction only allowed via an IdGenerator. > newline before comment Done http://gerrit.cloudera.org:8080/#/c/4349/4/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: Line 1126: * Check that we don't have any duplicate table IDs (see IMPALA-1702). > We should declare IMPALA-1702 as fixed and not mention it here. Instead, we I added something in ImpaladCatalog.java::addTable(). not sure if that is what you suggested.k -- 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: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Huaisi Xu <h...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Huaisi Xu <h...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
