Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/23905 )
Change subject: IMPALA-12844 Support setting DBPROPERTIES ...................................................................... Patch Set 7: (7 comments) Looks good in general - one important question is the correct privilege to use in alter db dbproperties. http://gerrit.cloudera.org:8080/#/c/23905/7/fe/src/main/java/org/apache/impala/analysis/AlterDbSetDbPropertiesStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterDbSetDbPropertiesStmt.java: http://gerrit.cloudera.org:8080/#/c/23905/7/fe/src/main/java/org/apache/impala/analysis/AlterDbSetDbPropertiesStmt.java@51 PS7, Line 51: ALL ALTER privilege looks enough to me. http://gerrit.cloudera.org:8080/#/c/23905/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/23905/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@8408 PS7, Line 8408: alterDatabaseSetDbProperties This looks very similar to alterDatabaseSetOwner and dublicates many non-trivial steps. I think that it would be better to move most logic to alterDatabase and only do the part that is different in the switch. Didn't think it through how exactly the alterDatabaseSetDbProperties/alterDatabaseSetOwner would look after this refactor. http://gerrit.cloudera.org:8080/#/c/23905/7/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java: http://gerrit.cloudera.org:8080/#/c/23905/7/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@4811 PS7, Line 4811: TestAlterDatabaseSetDbProperties TestCreateDb() could be also extended. http://gerrit.cloudera.org:8080/#/c/23905/7/fe/src/test/java/org/apache/impala/analysis/ParserTest.java File fe/src/test/java/org/apache/impala/analysis/ParserTest.java: http://gerrit.cloudera.org:8080/#/c/23905/7/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@2280 PS7, Line 2280: ParserError(String.format("CREATE %s Foo WITH DBPROPERTIES('a'=b)", kw)); Can you add a few tests that have multiple optional arguments for create databases? E.g. location and comment in the correct or incorrect order. http://gerrit.cloudera.org:8080/#/c/23905/7/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/23905/7/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java@2685 PS7, Line 2685: authorize("alter database functional set dbproperties('a'='b')") : .ok(onServer(true, TPrivilegeLevel.ALL)) : .ok(onServer(true, TPrivilegeLevel.OWNER)) : .ok(onDatabase(true, "functional", TPrivilegeLevel.ALL)) : .ok(onDatabase(true, "functional", TPrivilegeLevel.OWNER)) : .error(accessError(true, "functional")) : .error(accessError(true, "functional"), onServer(true, allExcept( : TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER))) : .error(accessError(true, "functional"), onDatabase(true, "functional", : allExcept(TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER))); : : authorize("alter database functional set dbproperties('a'='b')") : .ok(onServer(TPrivilegeLevel.values())) : .ok(onDatabase("functional", TPrivilegeLevel.values())); : : // Database does not exist. : authorize("alter database nodb set dbproperties('a'='b')") : .error(accessError(true, "nodb")) : .error(accessError(true, "nodb"), onServer(true, allExcept( : TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER))); This looks the same as the "core" of testAlterDatabase - helper function like checkAlterDbPrivileges could be created that is called in both function. On the other side I am actually not sure if the two DDLs should need the same privileges - setDbProperties could be be probably allowd in more cases (TPrivilegeLevel::ALTER) http://gerrit.cloudera.org:8080/#/c/23905/7/tests/metadata/test_ddl.py File tests/metadata/test_ddl.py: http://gerrit.cloudera.org:8080/#/c/23905/7/tests/metadata/test_ddl.py@270 PS7, Line 270: test_alter_database_set_db_properties A create database with dbproperties test could be also added, e.g here: https://github.com/apache/impala/blob/master/testdata/workloads/functional-query/queries/QueryTest/create-database.test Another test that could be updated is https://github.com/apache/impala/blob/a398f7992e8a94e20861ffe81d3a84f186c406da/tests/metadata/test_metadata_query_statements.py#L217 It create the db with with dbproperties using Hive currently. http://gerrit.cloudera.org:8080/#/c/23905/7/tests/metadata/test_ddl_base.py File tests/metadata/test_ddl_base.py: http://gerrit.cloudera.org:8080/#/c/23905/7/tests/metadata/test_ddl_base.py@51 PS7, Line 51: def _create_db(self, db_name, sync=False, comment=None): db properties support could be added here -- To view, visit http://gerrit.cloudera.org:8080/23905 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I628c7bed4f0c39aed7f5f4ffea52421caf501933 Gerrit-Change-Number: 23905 Gerrit-PatchSet: 7 Gerrit-Owner: Balazs Hevele <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Mon, 02 Feb 2026 10:31:48 +0000 Gerrit-HasComments: Yes
