Bikramjeet Vig has posted comments on this change. Change subject: IMPALA-1683: Allow REFRESH on a single partition ......................................................................
Patch Set 10: Code-Review+1 (18 comments) Carrying forward David's +1 http://gerrit.cloudera.org:8080/#/c/3385/10/fe/src/main/java/com/cloudera/impala/analysis/AnalysisContext.java File fe/src/main/java/com/cloudera/impala/analysis/AnalysisContext.java: Line 393: // except for DESCRIBE TABLE or REFRESH/INVALIDATE statements > Do we really register column-level auth requests for refresh/invalidate sta Since we require "ANY" privilege for refresh, the partition analyzer adds an "ANY" column privilege request. This 'if' block enforces the condition that if an AuthorizeableColumn request exists, it should be preceded by a table level "SELECT" request (See Line 492), but since we only have another table level "ANY" request, this path will throw an error. Line 439: analysisResult_.isResetMetadataStmt()); > Why do we need this? My understanding is that we'll never register column-l As explained above, we do require a column level privilege and hence this condition is required http://gerrit.cloudera.org:8080/#/c/3385/10/fe/src/main/java/com/cloudera/impala/analysis/ResetMetadataStmt.java File fe/src/main/java/com/cloudera/impala/analysis/ResetMetadataStmt.java: Line 36: // not null when refreshing a single partition specified by this partition > not null when refreshing a single partition. Done Line 53: public void analyze(Analyzer analyzer) throws AnalysisException { > This seems to allow "invalidate metadata <part_spec>" as well. I think its This case will be caught by the parser but I'll still put a precondition for that and I have added some test cases in parser test as well. http://gerrit.cloudera.org:8080/#/c/3385/10/fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java: Line 1175: */ > Can you explain a little on the return 'tbl' of this method? Done Line 1182: HdfsTable hdfsTable = (HdfsTable) tbl; > can you use hdfsTable consistently instead of tbl below for clarity? for ex Done Line 1183: HdfsPartition hdfspartition = hdfsTable > hdfsPartition Done Line 1195: TTableName tblName = new TTableName(tbl.getDb().getName().toLowerCase(), > I don't think you really need this if you take care of my other comments he Done Line 1200: db.getName(), tblName.getTable_name(), partitionName); > use tbl.getDb().getName(), tbl.getName() Done Line 1211: "Error loading metadata for partition: " + db.getName() + "." > usu tbl.getFullName() Done http://gerrit.cloudera.org:8080/#/c/3385/10/fe/src/main/java/com/cloudera/impala/catalog/HdfsTable.java File fe/src/main/java/com/cloudera/impala/catalog/HdfsTable.java: Line 1936: > no need for newline Done http://gerrit.cloudera.org:8080/#/c/3385/10/fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java File fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java: Line 2752: if (req.isSetPartition_spec()) { > Same Preconditions check as my comment in Analyzer (to prevent "invalidate The 'if' statements preceding this make sure that execution will only reach this point if its a refresh statement and we have a valid table name and therefore a non null table object. Also, in case we have a malformed partition_spec then we wont get a valid partition object and the hive rpc will throw an exception. this will make sure nothing breaks http://gerrit.cloudera.org:8080/#/c/3385/10/fe/src/test/java/com/cloudera/impala/analysis/AnalyzerTest.java File fe/src/test/java/com/cloudera/impala/analysis/AnalyzerTest.java: Line 667: AnalysisError( > add test where you try to refresh a partition of an unpartitioned table Done http://gerrit.cloudera.org:8080/#/c/3385/10/fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java File fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java: Line 785: //Positive cases for checking refresh partition > nit: space before "Positive" Done Line 786: AuthzOk("refresh functional.alltypesagg partition (year=2010, month =1, day = 1)"); > nit: use consistent style with partition key values, e.g. year=2010 (no spa Done Line 813: AuthzError( > also add a test where you try to refresh a non-existent partition of a tabl Done http://gerrit.cloudera.org:8080/#/c/3385/10/tests/metadata/test_refresh_partition.py File tests/metadata/test_refresh_partition.py: Line 79: 'create table if not exists %s (x int) partitioned by (y int, z int)' % > Remove the "IF NOT EXISTS" part (and elsewhere in this file). Done Line 230: CREATE TABLE %s LIKE functional.alltypes STORED AS PARQUET > nit: why keyword in caps here and everywhere else lowercase? Done -- To view, visit http://gerrit.cloudera.org:8080/3385 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4bf5dd630f2e175be5800187bbf9337c91f82bd8 Gerrit-PatchSet: 10 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-HasComments: Yes
