Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/23905 )
Change subject: IMPALA-12844: Support setting DBPROPERTIES ...................................................................... Patch Set 9: (4 comments) Thanks for addressing my comment on patch set 8. I left some additional minor comments on CatalogOpExecutor.java. http://gerrit.cloudera.org:8080/#/c/23905/9/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/9/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@8394 PS9, Line 8394: TAlterDbSetDbPropertiesParams setDbPropertiesParams = : params.getSet_dbproperties_params(); : TAlterDbSetOwnerParams setOwnerParams = params.getSet_owner_params(); nit: Could we move params.getSet_dbproperties_params() to the case of SET_DBPROPERTIES in the following? The same applies to params.getSet_owner_params() (move this to the case of SET_OWNER). http://gerrit.cloudera.org:8080/#/c/23905/9/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@8424 PS9, Line 8424: if (setOwnerParams != null) { : msDb.setOwnerName(setOwnerParams.owner_name); : msDb.setOwnerType(PrincipalType.valueOf(setOwnerParams.owner_type.name())); : } : : if (setDbPropertiesParams != null) { : for (Map.Entry<String, String> property : : setDbPropertiesParams.properties.entrySet()) { : msDb.putToParameters(property.getKey(), property.getValue()); : } : } nit: Would it be better to use a switch statement here to deal with various cases? Is possible that both setOwnerParams and setDbPropertiesParams are non-null values? http://gerrit.cloudera.org:8080/#/c/23905/9/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@8441 PS9, Line 8441: updateDatabaseOwnerPrivilege Just provide my observation here. When Apache Ranger is the authorization provider, https://github.com/apache/impala/blob/322515c/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java#L376-L380 would be called. This is essentially a no-op. We did something when Apache Sentry was the authorization provider, e.g., https://github.com/apache/impala/blame/3.x/fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java#L340-L352/. I guess it's okay we keep this here for now. Or maybe we could add some code comment here? // In RangerCatalogdAuthorizationManager.java. @Override public void updateDatabaseOwnerPrivilege(String serverName, String databaseName, String oldOwner, PrincipalType oldOwnerType, String newOwner, PrincipalType newOwnerType, TDdlExecResponse response) throws ImpalaException { } http://gerrit.cloudera.org:8080/#/c/23905/8/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/8/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java@335 PS8, Line 335: > Done Done -- 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: 9 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: Wed, 04 Feb 2026 20:12:36 +0000 Gerrit-HasComments: Yes
