Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/16529 )
Change subject: IMPALA-10172: Support Hive metastore managed locations for databases ...................................................................... Patch Set 4: (5 comments) > (1 comment) > > When a managed table is created I think the table should inherit > the managed table location, somewhere here: > https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java#L127 > What do you think? There is a hierarchy of places where the location can be defined: 1. service level default managed/external path + db_name 2. location/managedlocation overridden per database 3. location set in table The code your link points to deals with case 3. and is optional, see https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java#L2397 It is a bit tricky how the 1 and 2 are applied - normally it is done by the HMS, not by Impala, but there are cases where we ask "where would HMS put this table?", see https://github.com/apache/impala/blob/master/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java#L1047 http://gerrit.cloudera.org:8080/#/c/16529/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16529/4//COMMIT_MSG@14 PS4, Line 14: Impala's output for DESCRIBE DATABASE (EXTENDED) > nit: could you provide examples of the new output? Done http://gerrit.cloudera.org:8080/#/c/16529/4//COMMIT_MSG@16 PS4, Line 16: braking > nit: breaking Done http://gerrit.cloudera.org:8080/#/c/16529/4/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java File fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java: http://gerrit.cloudera.org:8080/#/c/16529/4/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java@1556 PS4, Line 1556: authorize(stmt) : .ok(onServer(TPrivilegeLevel.ALL)) : .ok(onServer(TPrivilegeLevel.OWNER)) : .ok(onServer(TPrivilegeLevel.CREATE), onUri(uri, TPrivilegeLevel.ALL)) : .ok(onServer(TPrivilegeLevel.CREATE), onUri(uri, TPrivilegeLevel.OWNER)) : .error(createError("newdb")) : .error(createError("newdb"), onServer(allExcept(TPrivilegeLevel.ALL, : TPrivilegeLevel.OWNER, TPrivilegeLevel.CREATE)), : onUri(uri, TPrivilegeLevel.ALL)) : .error(createError("newdb"), onServer(allExcept(TPrivilegeLevel.ALL, : TPrivilegeLevel.OWNER, TPrivilegeLevel.CREATE)), : onUri(uri, TPrivilegeLevel.OWNER)) : .error(accessError(uri), onServer(TPrivilegeLevel.CREATE)); > nit: instead of copy-pasting this could go to a helper function. I used the a loop to avoid the redundancy, which seems to be the pattern in this file. http://gerrit.cloudera.org:8080/#/c/16529/4/testdata/workloads/functional-query/queries/QueryTest/describe-db.test File testdata/workloads/functional-query/queries/QueryTest/describe-db.test: http://gerrit.cloudera.org:8080/#/c/16529/4/testdata/workloads/functional-query/queries/QueryTest/describe-db.test@47 PS4, Line 47: > nit: double space Done http://gerrit.cloudera.org:8080/#/c/16529/4/testdata/workloads/functional-query/queries/QueryTest/describe-hive-db.test File testdata/workloads/functional-query/queries/QueryTest/describe-hive-db.test: http://gerrit.cloudera.org:8080/#/c/16529/4/testdata/workloads/functional-query/queries/QueryTest/describe-hive-db.test@36 PS4, Line 36: 'managedlocation:','$NAMENODE/test2.db','' > Instead of putting it in a new row, how about adding it as the last column There is some discussion about this in the commit message and in IMPALA-6686. This 2 row output is weird in case of DESCRIBE DATABASE, but it is consistent with DESCRIBE DATABASE EXTENDED, and the output will not change for "normal" databases where this property is not set. I agree with switching to a single row version, which would be also consistent with Hive, but I would prefer that in a separate commit with a query option that can switch between formats. -- To view, visit http://gerrit.cloudera.org:8080/16529 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I925632a43ff224f762031e89981896722e453399 Gerrit-Change-Number: 16529 Gerrit-PatchSet: 4 Gerrit-Owner: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tamas Mate <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Mon, 05 Oct 2020 18:02:59 +0000 Gerrit-HasComments: Yes
