Balazs Hevele has posted comments on this change. ( http://gerrit.cloudera.org:8080/23905 )
Change subject: IMPALA-12844 Support setting DBPROPERTIES ...................................................................... Patch Set 8: (7 comments) 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: tDb > I agree with Csaba on this. We could require the requesting user to just ha Thank you for the detailed information. I changed the privilege to ALTER and added the test cases to RangedAuditLogTest.java 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: > This looks very similar to alterDatabaseSetOwner and dublicates many non-tr Done 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: append("a"); > TestCreateDb() could be also extended. Done 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 da Done 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(TPrivilegeLevel.ALL)) : .ok(onServer(TPrivilegeLevel.OWNER)) : .ok(onServer(TPrivilegeLevel.ALTER)) : .ok(onDatabase("functional", TPrivilegeLevel.ALL)) : .ok(onDatabase("functional", TPrivilegeLevel.OWNER)) : .ok(onDatabase("functional", TPrivilegeLevel.ALTER)) : .error(alterError("functional")) : .error(alterError("functional"), onServer(allExcept( : TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER, : TPrivilegeLevel.ALTER))) : .error(alterError("functional"), onDatabase("functional", : allExcept(TPrivilegeLevel.ALL, TPrivilegeLevel.OWNER, : TPrivilegeLevel.ALTER))); : : authorize("alter database functional set dbproperties('a'='b')") : .ok(onServer(TPrivilegeLevel.values())) : .ok(onDatabase("functional", TPrivilegeLevel.values())); : : // Database does not exist. > This looks the same as the "core" of testAlterDatabase - helper function li Added ALTER privilege for dbproperties tests, but now I don't see a clear way to pull set owner/set dbproperties tests to a common function. 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: htt Done 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 It could be added, but no tests use the rest of the optional arguments either (sync, comment), so the question is if it's worth adding -- 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: 8 Gerrit-Owner: Balazs Hevele <[email protected]> Gerrit-Reviewer: Balazs Hevele <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Tue, 03 Feb 2026 10:10:33 +0000 Gerrit-HasComments: Yes
