github-actions[bot] commented on code in PR #60323: URL: https://github.com/apache/doris/pull/60323#discussion_r3526269317
########## fe/fe-core/src/main/java/org/apache/doris/tablefunction/BrokersTableValuedFunction.java: ########## @@ -0,0 +1,127 @@ +// 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.tablefunction; + +import org.apache.doris.catalog.Column; +import org.apache.doris.catalog.Env; +import org.apache.doris.catalog.InfoSchemaDb; +import org.apache.doris.catalog.ScalarType; +import org.apache.doris.common.AnalysisException; +import org.apache.doris.common.ErrorCode; +import org.apache.doris.datasource.InternalCatalog; +import org.apache.doris.mysql.privilege.PrivPredicate; +import org.apache.doris.qe.ConnectContext; +import org.apache.doris.thrift.TBrokersMetadataParams; +import org.apache.doris.thrift.TMetaScanRange; +import org.apache.doris.thrift.TMetadataType; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import org.apache.commons.lang3.StringUtils; + +import java.util.List; +import java.util.Map; + +/** + * The Implement of table valued function + * brokers(). + */ +public class BrokersTableValuedFunction extends MetadataTableValuedFunction { + + public static final String NAME = "brokers"; + private static final String CLUSTER_NAME = "cluster_name"; + private static final ImmutableList<Column> SCHEMA = ImmutableList.of( + new Column("Name", ScalarType.createStringType()), + new Column("Host", ScalarType.createStringType()), + new Column("Port", ScalarType.createStringType()), + new Column("Alive", ScalarType.createStringType()), + new Column("LastStartTime", ScalarType.createStringType()), + new Column("LastUpdateTime", ScalarType.createStringType()), + new Column("ErrMsg", ScalarType.createStringType()) + ); + private static final ImmutableMap<String, Integer> COLUMN_TO_INDEX; + + static { + ImmutableMap.Builder<String, Integer> builder = new ImmutableMap.Builder(); + for (int i = 0; i < SCHEMA.size(); i++) { + builder.put(SCHEMA.get(i).getName().toLowerCase(), i); + } + COLUMN_TO_INDEX = builder.build(); + } + + private final String clusterName; + + /** + * constructor for BrokersTableValuedFunction + */ + public BrokersTableValuedFunction(Map<String, String> params) throws AnalysisException { + if (params == null) { + throw new AnalysisException("params cannot be null"); + } + String originalClusterName = params.get(CLUSTER_NAME); + if (originalClusterName != null) { + if (StringUtils.isBlank(originalClusterName)) { + throw new AnalysisException("Invalid brokers param value: " + originalClusterName); + } + clusterName = originalClusterName.toLowerCase(); + } else { + clusterName = null; + } + for (String key : params.keySet()) { + if (key == null || !CLUSTER_NAME.equals(key.toLowerCase())) { + throw new AnalysisException("'" + key + "' is invalid property," + + " only support property [" + CLUSTER_NAME + "]"); + } + } + if (!Env.getCurrentEnv().getAccessManager().checkDbPriv(ConnectContext.get(), Review Comment: This privilege check is weaker than the existing broker metadata surfaces. `SHOW BROKER` gates the same `BrokerMgr.getBrokersInfo()` data behind global ADMIN or OPERATOR/NODE, and `SHOW PROC '/brokers'` requires ADMIN_OR_NODE, but this TVF only requires SELECT on `internal.information_schema`. New users get that information_schema SELECT in their default role, so `select * from brokers()` would expose broker names, hosts, ports, liveness, and errors to users who cannot run `SHOW BROKER`. Please align the TVF auth with the existing broker command/proc privilege before returning these rows. ########## fe/fe-core/src/main/java/org/apache/doris/tablefunction/BrokersTableValuedFunction.java: ########## @@ -0,0 +1,127 @@ +// 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.tablefunction; + +import org.apache.doris.catalog.Column; +import org.apache.doris.catalog.Env; +import org.apache.doris.catalog.InfoSchemaDb; +import org.apache.doris.catalog.ScalarType; +import org.apache.doris.common.AnalysisException; +import org.apache.doris.common.ErrorCode; +import org.apache.doris.datasource.InternalCatalog; +import org.apache.doris.mysql.privilege.PrivPredicate; +import org.apache.doris.qe.ConnectContext; +import org.apache.doris.thrift.TBrokersMetadataParams; +import org.apache.doris.thrift.TMetaScanRange; +import org.apache.doris.thrift.TMetadataType; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import org.apache.commons.lang3.StringUtils; + +import java.util.List; +import java.util.Map; + +/** + * The Implement of table valued function + * brokers(). + */ +public class BrokersTableValuedFunction extends MetadataTableValuedFunction { + + public static final String NAME = "brokers"; + private static final String CLUSTER_NAME = "cluster_name"; + private static final ImmutableList<Column> SCHEMA = ImmutableList.of( + new Column("Name", ScalarType.createStringType()), + new Column("Host", ScalarType.createStringType()), + new Column("Port", ScalarType.createStringType()), + new Column("Alive", ScalarType.createStringType()), + new Column("LastStartTime", ScalarType.createStringType()), + new Column("LastUpdateTime", ScalarType.createStringType()), + new Column("ErrMsg", ScalarType.createStringType()) + ); + private static final ImmutableMap<String, Integer> COLUMN_TO_INDEX; + + static { + ImmutableMap.Builder<String, Integer> builder = new ImmutableMap.Builder(); + for (int i = 0; i < SCHEMA.size(); i++) { + builder.put(SCHEMA.get(i).getName().toLowerCase(), i); + } + COLUMN_TO_INDEX = builder.build(); + } + + private final String clusterName; + + /** + * constructor for BrokersTableValuedFunction + */ + public BrokersTableValuedFunction(Map<String, String> params) throws AnalysisException { + if (params == null) { + throw new AnalysisException("params cannot be null"); + } + String originalClusterName = params.get(CLUSTER_NAME); Review Comment: `CLUSTER_NAME` is accepted by the validation loop below, but this lookup is case-sensitive and runs before any normalization. For `brokers("CLUSTER_NAME" = "missing")`, `originalClusterName` stays null, `getMetaScanRange()` sends a null filter, and the scan returns all brokers instead of zero rows. The new regression case with uppercase `CLUSTER_NAME` does not catch this because it uses the existing test cluster and expects all three rows. Please normalize the property map before lookup, as the other metadata TVFs do, and add a case where uppercase `CLUSTER_NAME` must filter something out. ########## gensrc/thrift/Types.thrift: ########## @@ -764,6 +764,7 @@ enum TMetadataType { PAIMON = 12, PARQUET = 13, STREAMS = 14, + BROKERS = 15, Review Comment: This new metadata type needs a compatibility gate or a failure path for mixed FE/BE versions. A new FE will build a `TMetaScanRange` with `BROKERS` and `MetadataScanNode` sends it to whatever query backend `FederationBackendPolicy` selects. If that BE is still old during rolling upgrade, it does not have the `BROKERS` case and `MetaScanner::_fetch_metadata()` takes the default path that sets EOF and returns OK, so `brokers()` can silently return no rows. Please either route/gate this TVF to BEs that support the new type or make unsupported metadata types fail loudly instead of succeeding with an empty result. ########## fe/fe-core/src/main/java/org/apache/doris/tablefunction/BrokersTableValuedFunction.java: ########## @@ -0,0 +1,127 @@ +// 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.tablefunction; + +import org.apache.doris.catalog.Column; +import org.apache.doris.catalog.Env; +import org.apache.doris.catalog.InfoSchemaDb; +import org.apache.doris.catalog.ScalarType; +import org.apache.doris.common.AnalysisException; +import org.apache.doris.common.ErrorCode; +import org.apache.doris.datasource.InternalCatalog; +import org.apache.doris.mysql.privilege.PrivPredicate; +import org.apache.doris.qe.ConnectContext; +import org.apache.doris.thrift.TBrokersMetadataParams; +import org.apache.doris.thrift.TMetaScanRange; +import org.apache.doris.thrift.TMetadataType; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import org.apache.commons.lang3.StringUtils; + +import java.util.List; +import java.util.Map; + +/** + * The Implement of table valued function + * brokers(). + */ +public class BrokersTableValuedFunction extends MetadataTableValuedFunction { + + public static final String NAME = "brokers"; + private static final String CLUSTER_NAME = "cluster_name"; + private static final ImmutableList<Column> SCHEMA = ImmutableList.of( + new Column("Name", ScalarType.createStringType()), + new Column("Host", ScalarType.createStringType()), + new Column("Port", ScalarType.createStringType()), + new Column("Alive", ScalarType.createStringType()), + new Column("LastStartTime", ScalarType.createStringType()), + new Column("LastUpdateTime", ScalarType.createStringType()), + new Column("ErrMsg", ScalarType.createStringType()) + ); + private static final ImmutableMap<String, Integer> COLUMN_TO_INDEX; + + static { + ImmutableMap.Builder<String, Integer> builder = new ImmutableMap.Builder(); + for (int i = 0; i < SCHEMA.size(); i++) { + builder.put(SCHEMA.get(i).getName().toLowerCase(), i); + } + COLUMN_TO_INDEX = builder.build(); + } + + private final String clusterName; + + /** + * constructor for BrokersTableValuedFunction + */ + public BrokersTableValuedFunction(Map<String, String> params) throws AnalysisException { + if (params == null) { + throw new AnalysisException("params cannot be null"); + } + String originalClusterName = params.get(CLUSTER_NAME); + if (originalClusterName != null) { + if (StringUtils.isBlank(originalClusterName)) { + throw new AnalysisException("Invalid brokers param value: " + originalClusterName); + } + clusterName = originalClusterName.toLowerCase(); Review Comment: This changes the semantics of broker names from exact to case-insensitive only for the TVF. Add/drop and lookup paths preserve the broker name text and use exact map keys in `BrokerMgr`, so `BrokerA` and `brokera` can be distinct broker names. After this lowercasing, `MetadataGenerator.brokersMetadataResult()` compares with `equalsIgnoreCase`, so `brokers("cluster_name" = "BrokerA")` would return both clusters. Please preserve the original filter value and compare broker names consistently with the existing broker manager APIs. -- 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]
