Alex Behm has posted comments on this change. Change subject: IMPALA-1683: Allow REFRESH on a single partition ......................................................................
Patch Set 10: (15 comments) 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 statements? Line 439: analysisResult_.isResetMetadataStmt()); Why do we need this? My understanding is that we'll never register column-level auth requests for refresh/invalidate statements, so the first condition in this Preconditions check should be sufficient. 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. (shorter == better) 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 1182: HdfsTable hdfsTable = (HdfsTable) tbl; can you use hdfsTable consistently instead of tbl below for clarity? for example, see L1205-1206 which might seem confusing otherwise Line 1183: HdfsPartition hdfspartition = hdfsTable hdfsPartition 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 here. Line 1200: db.getName(), tblName.getTable_name(), partitionName); use tbl.getDb().getName(), tbl.getName() Line 1211: "Error loading metadata for partition: " + db.getName() + "." usu tbl.getFullName() 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 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 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" 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 spaces between <col>=<value> fix elsewhere also Line 813: AuthzError( also add a test where you try to refresh a non-existent partition of a table that the user does not have access to the test is to make sure that an auth error is reported, and not an analysis error because that would reveal its non-existence to an unprivileged user 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). We expect the table not to exist, and if something goes wrong it may be more difficult to debug because we silently ignore that failure (table already exists) Line 230: CREATE TABLE %s LIKE functional.alltypes STORED AS PARQUET nit: why keyword in caps here and everywhere else lowercase? -- 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: 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
