github-actions[bot] commented on code in PR #60902: URL: https://github.com/apache/doris/pull/60902#discussion_r2881403720
########## fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/DropAuthenticationIntegrationCommand.java: ########## @@ -0,0 +1,65 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.nereids.trees.plans.commands; + +import org.apache.doris.catalog.Env; +import org.apache.doris.common.ErrorCode; +import org.apache.doris.common.ErrorReport; +import org.apache.doris.mysql.privilege.PrivPredicate; +import org.apache.doris.nereids.trees.plans.PlanType; +import org.apache.doris.nereids.trees.plans.visitor.PlanVisitor; +import org.apache.doris.qe.ConnectContext; +import org.apache.doris.qe.StmtExecutor; + +import java.util.Objects; + +/** + * DROP AUTHENTICATION INTEGRATION command entry. + */ +public class DropAuthenticationIntegrationCommand extends DropCommand { + private final boolean ifExists; + private final String integrationName; + + public DropAuthenticationIntegrationCommand(boolean ifExists, String integrationName) { + super(PlanType.DROP_AUTHENTICATION_INTEGRATION_COMMAND); + this.ifExists = ifExists; + this.integrationName = Objects.requireNonNull(integrationName, "integrationName can not be null"); + } + + @Override + public <R, C> R accept(PlanVisitor<R, C> visitor, C context) { + return visitor.visitCommand(this, context); + } Review Comment: [Low-Medium] Same as above - should use a dedicated `visitDropAuthenticationIntegrationCommand` method instead of generic `visitor.visitCommand`. ########## fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/CreateAuthenticationIntegrationCommand.java: ########## @@ -0,0 +1,87 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.nereids.trees.plans.commands; + +import org.apache.doris.analysis.StmtType; +import org.apache.doris.catalog.Env; +import org.apache.doris.common.ErrorCode; +import org.apache.doris.common.ErrorReport; +import org.apache.doris.mysql.privilege.PrivPredicate; +import org.apache.doris.nereids.trees.plans.PlanType; +import org.apache.doris.nereids.trees.plans.visitor.PlanVisitor; +import org.apache.doris.qe.ConnectContext; +import org.apache.doris.qe.StmtExecutor; + +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.Objects; + +/** + * CREATE AUTHENTICATION INTEGRATION command entry. + */ +public class CreateAuthenticationIntegrationCommand extends Command implements ForwardWithSync, NeedAuditEncryption { + private final String integrationName; + private final Map<String, String> properties; + private final String comment; + + public CreateAuthenticationIntegrationCommand(String integrationName, + Map<String, String> properties, String comment) { + super(PlanType.CREATE_AUTHENTICATION_INTEGRATION_COMMAND); + this.integrationName = Objects.requireNonNull(integrationName, "integrationName can not be null"); + this.properties = Collections.unmodifiableMap( + new LinkedHashMap<>(Objects.requireNonNull(properties, "properties can not be null"))); + this.comment = comment; + } + + @Override + public <R, C> R accept(PlanVisitor<R, C> visitor, C context) { + return visitor.visitCommand(this, context); + } Review Comment: [Low-Medium] **Pattern deviation**: This uses the generic `visitor.visitCommand(this, context)`. The established codebase pattern is to add a dedicated `visitCreateAuthenticationIntegrationCommand` method to `CommandVisitor.java` (defaulting to `visitCommand`) and dispatch to it here. See `CreateCatalogCommand.accept()` for reference. Same applies to Alter and Drop commands. ########## fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/AlterAuthenticationIntegrationCommand.java: ########## @@ -0,0 +1,115 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.nereids.trees.plans.commands; + +import org.apache.doris.catalog.Env; +import org.apache.doris.common.AnalysisException; +import org.apache.doris.common.ErrorCode; +import org.apache.doris.common.ErrorReport; +import org.apache.doris.mysql.privilege.PrivPredicate; +import org.apache.doris.nereids.trees.plans.PlanType; +import org.apache.doris.nereids.trees.plans.visitor.PlanVisitor; +import org.apache.doris.qe.ConnectContext; +import org.apache.doris.qe.StmtExecutor; + +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.Objects; + +/** + * ALTER AUTHENTICATION INTEGRATION command entry. + */ +public class AlterAuthenticationIntegrationCommand extends AlterCommand implements NeedAuditEncryption { + /** alter action. */ + public enum AlterType { + SET_PROPERTIES, + SET_COMMENT + } + + private final String integrationName; + private final AlterType alterType; + private final Map<String, String> properties; + private final String comment; + + private AlterAuthenticationIntegrationCommand(String integrationName, AlterType alterType, + Map<String, String> properties, String comment) { + super(PlanType.ALTER_AUTHENTICATION_INTEGRATION_COMMAND); + this.integrationName = Objects.requireNonNull(integrationName, "integrationName can not be null"); + this.alterType = Objects.requireNonNull(alterType, "alterType can not be null"); + this.properties = Collections.unmodifiableMap( + new LinkedHashMap<>(Objects.requireNonNull(properties, "properties can not be null"))); + this.comment = comment; + } + + public static AlterAuthenticationIntegrationCommand forSetProperties(String integrationName, + Map<String, String> properties) { + return new AlterAuthenticationIntegrationCommand( + integrationName, AlterType.SET_PROPERTIES, properties, null); + } + + public static AlterAuthenticationIntegrationCommand forSetComment(String integrationName, String comment) { + return new AlterAuthenticationIntegrationCommand( + integrationName, AlterType.SET_COMMENT, Collections.emptyMap(), comment); + } + + @Override + public <R, C> R accept(PlanVisitor<R, C> visitor, C context) { + return visitor.visitCommand(this, context); + } Review Comment: [Low-Medium] Same as above - should use a dedicated `visitAlterAuthenticationIntegrationCommand` method instead of generic `visitor.visitCommand`. ########## fe/fe-core/src/main/antlr4/org/apache/doris/nereids/DorisParser.g4: ########## @@ -208,6 +208,8 @@ supportedCreateStatement LIKE existedTable=multipartIdentifier (WITH ROLLUP (rollupNames=identifierList)?)? #createTableLike | CREATE ROLE (IF NOT EXISTS)? name=identifierOrText (COMMENT STRING_LITERAL)? #createRole + | CREATE AUTHENTICATION INTEGRATION integrationName=identifier + WITH PROPERTIES LEFT_PAREN propertyItemList RIGHT_PAREN commentSpec? #createAuthenticationIntegration Review Comment: [Medium] **Missing `IF NOT EXISTS` support**: This `CREATE AUTHENTICATION INTEGRATION` rule does not support `IF NOT EXISTS`, unlike almost all other CREATE DDL commands in Doris (`CREATE CATALOG (IF NOT EXISTS)?`, `CREATE ROLE (IF NOT EXISTS)?`, `CREATE WORKLOAD GROUP (IF NOT EXISTS)?`). Consider adding it for consistency and to support idempotent DDL scripts: ``` | CREATE AUTHENTICATION INTEGRATION (IF NOT EXISTS)? integrationName=identifier WITH PROPERTIES LEFT_PAREN propertyItemList RIGHT_PAREN commentSpec? ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
