CRZbulabula commented on code in PR #17705: URL: https://github.com/apache/iotdb/pull/17705#discussion_r3301099844
########## iotdb-core/datanode/src/test/java/org/apache/iotdb/db/queryengine/plan/execution/config/metadata/relational/CountDBTaskTest.java: ########## @@ -0,0 +1,107 @@ +/* + * 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.iotdb.db.queryengine.plan.execution.config.metadata.relational; + +import org.apache.iotdb.commons.conf.IoTDBConstant; +import org.apache.iotdb.commons.queryengine.plan.relational.sql.ast.NodeLocation; +import org.apache.iotdb.db.queryengine.plan.execution.config.ConfigTaskResult; +import org.apache.iotdb.db.queryengine.plan.execution.config.executor.IConfigTaskExecutor; +import org.apache.iotdb.db.queryengine.plan.relational.sql.ast.CountDB; +import org.apache.iotdb.rpc.TSStatusCode; + +import com.google.common.util.concurrent.SettableFuture; +import org.apache.tsfile.read.common.block.TsBlock; +import org.junit.Test; +import org.mockito.Mockito; + +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.function.Predicate; + +import static org.apache.iotdb.commons.schema.table.InformationSchema.INFORMATION_DATABASE; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertSame; +import static org.mockito.ArgumentMatchers.same; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +public class CountDBTaskTest { + + @Test + public void testBuildTSBlockCountsVisibleDatabases() throws Exception { + final Map<String, Object> databaseInfoMap = new HashMap<>(); + databaseInfoMap.put("db1", new Object()); + databaseInfoMap.put("db2", new Object()); + + final SettableFuture<ConfigTaskResult> future = SettableFuture.create(); + CountDBTask.buildTSBlock(databaseInfoMap, future, databaseName -> !"db2".equals(databaseName)); + + final ConfigTaskResult result = future.get(); + final TsBlock resultSet = result.getResultSet(); + + assertEquals(TSStatusCode.SUCCESS_STATUS, result.getStatusCode()); + assertEquals( + Collections.singletonList(IoTDBConstant.COLUMN_COUNT), + result.getResultSetHeader().getRespColumns()); + assertEquals(1, resultSet.getPositionCount()); + assertEquals(2, resultSet.getColumn(0).getInt(0)); + } + + @Test + public void testBuildTSBlockCanHideInformationSchema() throws Exception { + final Map<String, Object> databaseInfoMap = new HashMap<>(); + databaseInfoMap.put("db1", new Object()); + databaseInfoMap.put("db2", new Object()); + + final SettableFuture<ConfigTaskResult> future = SettableFuture.create(); + CountDBTask.buildTSBlock(databaseInfoMap, future, databaseName -> "db1".equals(databaseName)); + + final ConfigTaskResult result = future.get(); + assertEquals(1, result.getResultSet().getColumn(0).getInt(0)); + } + + @Test + public void testBuildTSBlockDoesNotDoubleCountInformationSchema() throws Exception { + final Map<String, Object> databaseInfoMap = new HashMap<>(); + databaseInfoMap.put("db1", new Object()); + databaseInfoMap.put(INFORMATION_DATABASE, new Object()); + + final SettableFuture<ConfigTaskResult> future = SettableFuture.create(); + CountDBTask.buildTSBlock(databaseInfoMap, future, databaseName -> true); + + final ConfigTaskResult result = future.get(); + assertEquals(2, result.getResultSet().getColumn(0).getInt(0)); + } + + @Test + public void testExecuteDelegatesToExecutor() throws Exception { + final CountDB node = new CountDB(new NodeLocation(1, 1)); + final Predicate<String> canSeenDB = databaseName -> true; Review Comment: The rest of this PR renames `canSeenDB` → `canSeeDB` across `ShowDBTask`, `IConfigTaskExecutor`, and `ClusterConfigTaskExecutor`. The local variable here (and its references on lines 98, 102, 104) still uses the old misspelling. Recommend renaming for internal consistency with the rest of the PR. ########## iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/execution/config/metadata/relational/CountDBTask.java: ########## @@ -0,0 +1,82 @@ +/* + * 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.iotdb.db.queryengine.plan.execution.config.metadata.relational; + +import org.apache.iotdb.commons.conf.IoTDBConstant; +import org.apache.iotdb.commons.schema.column.ColumnHeader; +import org.apache.iotdb.db.queryengine.common.header.DatasetHeader; +import org.apache.iotdb.db.queryengine.plan.execution.config.ConfigTaskResult; +import org.apache.iotdb.db.queryengine.plan.execution.config.IConfigTask; +import org.apache.iotdb.db.queryengine.plan.execution.config.executor.IConfigTaskExecutor; +import org.apache.iotdb.db.queryengine.plan.relational.sql.ast.CountDB; +import org.apache.iotdb.rpc.TSStatusCode; + +import com.google.common.util.concurrent.ListenableFuture; +import com.google.common.util.concurrent.SettableFuture; +import org.apache.tsfile.enums.TSDataType; +import org.apache.tsfile.read.common.block.TsBlockBuilder; + +import java.util.Collections; +import java.util.Map; +import java.util.function.Predicate; + +import static org.apache.iotdb.commons.schema.table.InformationSchema.INFORMATION_DATABASE; + +public class CountDBTask implements IConfigTask { + + private final CountDB node; + private final Predicate<String> canSeeDB; + + public CountDBTask(final CountDB node, final Predicate<String> canSeeDB) { + this.node = node; + this.canSeeDB = canSeeDB; + } + + @Override + public ListenableFuture<ConfigTaskResult> execute(final IConfigTaskExecutor configTaskExecutor) + throws InterruptedException { + return configTaskExecutor.countDatabases(node, canSeeDB); + } + + public static void buildTSBlock( + final Map<String, ?> databaseInfoMap, Review Comment: Minor API tightening: only `databaseInfoMap.keySet()` is ever used downstream, so a `Set<String>` (or `Collection<String>`) parameter would more clearly convey the intent and free the test from having to fabricate a `Map<String, Object>` with dummy values. Not blocking. ########## iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/execution/config/executor/ClusterConfigTaskExecutor.java: ########## @@ -4133,7 +4135,25 @@ public SettableFuture<ConfigTaskResult> showDatabases( .setIsTableModel(true); final TShowDatabaseResp resp = client.showDatabase(req); // build TSBlock - ShowDBTask.buildTSBlock(resp.getDatabaseInfoMap(), future, showDB.isDetails(), canSeenDB); + ShowDBTask.buildTSBlock(resp.getDatabaseInfoMap(), future, showDB.isDetails(), canSeeDB); + } catch (final IOException | ClientManagerException | TException e) { + future.setException(e); + } + return future; + } + + @Override + public SettableFuture<ConfigTaskResult> countDatabases( + final CountDB countDB, final Predicate<String> canSeeDB) { + final SettableFuture<ConfigTaskResult> future = SettableFuture.create(); + final List<String> databasePathPattern = Arrays.asList(ALL_RESULT_NODES); + try (final ConfigNodeClient client = + CONFIG_NODE_CLIENT_MANAGER.borrowClient(ConfigNodeInfo.CONFIG_REGION_ID)) { + final TGetDatabaseReq req = + new TGetDatabaseReq(databasePathPattern, ALL_MATCH_SCOPE.serialize()) + .setIsTableModel(true); + final TShowDatabaseResp resp = client.showDatabase(req); + CountDBTask.buildTSBlock(resp.getDatabaseInfoMap(), future, canSeeDB); Review Comment: The body of `countDatabases` is nearly identical to `showDatabases` above (lines 4124–4143): same `databasePathPattern`, same `TGetDatabaseReq` construction, same `client.showDatabase(req)` call, same exception handling. The only meaningful difference is which `buildTSBlock` overload runs. Consider extracting a small private helper that fetches the `databaseInfoMap` and lets the caller pass a `BiConsumer<Map<String, TDatabaseInfo>, SettableFuture<ConfigTaskResult>>` or similar. Not blocking — duplication is small and consistent with the existing style in this file. ########## iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/execution/config/metadata/relational/CountDBTask.java: ########## @@ -0,0 +1,82 @@ +/* + * 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.iotdb.db.queryengine.plan.execution.config.metadata.relational; + +import org.apache.iotdb.commons.conf.IoTDBConstant; +import org.apache.iotdb.commons.schema.column.ColumnHeader; +import org.apache.iotdb.db.queryengine.common.header.DatasetHeader; +import org.apache.iotdb.db.queryengine.plan.execution.config.ConfigTaskResult; +import org.apache.iotdb.db.queryengine.plan.execution.config.IConfigTask; +import org.apache.iotdb.db.queryengine.plan.execution.config.executor.IConfigTaskExecutor; +import org.apache.iotdb.db.queryengine.plan.relational.sql.ast.CountDB; +import org.apache.iotdb.rpc.TSStatusCode; + +import com.google.common.util.concurrent.ListenableFuture; +import com.google.common.util.concurrent.SettableFuture; +import org.apache.tsfile.enums.TSDataType; +import org.apache.tsfile.read.common.block.TsBlockBuilder; + +import java.util.Collections; +import java.util.Map; +import java.util.function.Predicate; + +import static org.apache.iotdb.commons.schema.table.InformationSchema.INFORMATION_DATABASE; + +public class CountDBTask implements IConfigTask { + + private final CountDB node; + private final Predicate<String> canSeeDB; + + public CountDBTask(final CountDB node, final Predicate<String> canSeeDB) { + this.node = node; + this.canSeeDB = canSeeDB; + } + + @Override + public ListenableFuture<ConfigTaskResult> execute(final IConfigTaskExecutor configTaskExecutor) + throws InterruptedException { + return configTaskExecutor.countDatabases(node, canSeeDB); + } + + public static void buildTSBlock( + final Map<String, ?> databaseInfoMap, + final SettableFuture<ConfigTaskResult> future, + final Predicate<String> canSeeDB) { + // information_schema is synthesized in table model rather than returned from ConfigNode. + final long databaseCount = + databaseInfoMap.keySet().stream() + .filter(databaseName -> !INFORMATION_DATABASE.equals(databaseName)) + .filter(canSeeDB::test) + .count() + + (canSeeDB.test(INFORMATION_DATABASE) ? 1 : 0); Review Comment: The `.count() + (canSeeDB.test(...) ? 1 : 0)` pattern reads a bit cryptically because the synthesis of `information_schema` is split between the stream and a tail-addition. A more linear alternative: ```java long databaseCount = databaseInfoMap.keySet().stream() .filter(name -> !INFORMATION_DATABASE.equals(name)) .filter(canSeeDB::test) .count(); if (canSeeDB.test(INFORMATION_DATABASE)) { databaseCount++; } ``` Semantically identical — preference call. Either way, please consider adding a brief comment explaining *why* `information_schema` is filtered out of the stream (defensive dedup in case ConfigNode ever returns it), since the synthesis logic differs from `ShowDBTask` which mutates the map via `put(INFORMATION_DATABASE, null)`. ########## iotdb-core/relational-grammar/src/main/antlr4/org/apache/iotdb/db/relational/grammar/sql/RelationalSql.g4: ########## @@ -203,6 +204,10 @@ showDatabasesStatement : SHOW DATABASES (DETAILS)? ; +countDatabasesStatement + : COUNT DATABASES Review Comment: Grammar rule is clean. Verified that both `COUNT` (line 1487) and `DATABASES` (line 1488) are already in the `nonReserved` list, so this rule introduces no new reserved keywords — good. ########## iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/Coordinator.java: ########## @@ -610,6 +611,7 @@ private IQueryExecution createQueryExecutionForTableModel( queryContext.setTimeOut(timeOut); queryContext.setStartTime(startTime); if (statement instanceof DropDB + || statement instanceof CountDB Review Comment: Nit: `CountDB` is inserted between `DropDB` and `ShowDB`. It reads more naturally placed next to `ShowDB` (both are read-side database operations), keeping `DropDB` adjacent to the other DDL siblings. ########## iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/execution/config/metadata/relational/CountDBTask.java: ########## @@ -0,0 +1,82 @@ +/* + * 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.iotdb.db.queryengine.plan.execution.config.metadata.relational; + +import org.apache.iotdb.commons.conf.IoTDBConstant; +import org.apache.iotdb.commons.schema.column.ColumnHeader; +import org.apache.iotdb.db.queryengine.common.header.DatasetHeader; +import org.apache.iotdb.db.queryengine.plan.execution.config.ConfigTaskResult; +import org.apache.iotdb.db.queryengine.plan.execution.config.IConfigTask; +import org.apache.iotdb.db.queryengine.plan.execution.config.executor.IConfigTaskExecutor; +import org.apache.iotdb.db.queryengine.plan.relational.sql.ast.CountDB; +import org.apache.iotdb.rpc.TSStatusCode; + +import com.google.common.util.concurrent.ListenableFuture; +import com.google.common.util.concurrent.SettableFuture; +import org.apache.tsfile.enums.TSDataType; +import org.apache.tsfile.read.common.block.TsBlockBuilder; + +import java.util.Collections; +import java.util.Map; +import java.util.function.Predicate; + +import static org.apache.iotdb.commons.schema.table.InformationSchema.INFORMATION_DATABASE; + +public class CountDBTask implements IConfigTask { + + private final CountDB node; + private final Predicate<String> canSeeDB; + + public CountDBTask(final CountDB node, final Predicate<String> canSeeDB) { + this.node = node; + this.canSeeDB = canSeeDB; + } + + @Override + public ListenableFuture<ConfigTaskResult> execute(final IConfigTaskExecutor configTaskExecutor) + throws InterruptedException { + return configTaskExecutor.countDatabases(node, canSeeDB); + } + + public static void buildTSBlock( + final Map<String, ?> databaseInfoMap, + final SettableFuture<ConfigTaskResult> future, + final Predicate<String> canSeeDB) { + // information_schema is synthesized in table model rather than returned from ConfigNode. + final long databaseCount = + databaseInfoMap.keySet().stream() + .filter(databaseName -> !INFORMATION_DATABASE.equals(databaseName)) + .filter(canSeeDB::test) + .count() + + (canSeeDB.test(INFORMATION_DATABASE) ? 1 : 0); + + final TsBlockBuilder builder = new TsBlockBuilder(Collections.singletonList(TSDataType.INT32)); + builder.getTimeColumnBuilder().writeLong(0L); + builder.getColumnBuilder(0).writeInt((int) databaseCount); Review Comment: Narrowing cast `(int) databaseCount`. INT32 is consistent with the tree-model `CountDatabaseTask` (datanode/.../metadata/CountDatabaseTask.java#L58), so this matches convention. Database counts are bounded in practice, so overflow is not a real concern — just flagging for awareness. ########## iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/sql/ast/CountDB.java: ########## @@ -0,0 +1,77 @@ +/* + * 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.iotdb.db.queryengine.plan.relational.sql.ast; + +import org.apache.iotdb.commons.queryengine.plan.relational.sql.ast.AstMemoryEstimationHelper; +import org.apache.iotdb.commons.queryengine.plan.relational.sql.ast.IAstVisitor; +import org.apache.iotdb.commons.queryengine.plan.relational.sql.ast.Node; +import org.apache.iotdb.commons.queryengine.plan.relational.sql.ast.NodeLocation; +import org.apache.iotdb.commons.queryengine.plan.relational.sql.ast.Statement; + +import com.google.common.collect.ImmutableList; +import org.apache.tsfile.utils.RamUsageEstimator; + Review Comment: `(AstVisitor<R, C>) visitor` — unchecked cast follows the existing project convention (see `ShowDB`, `ClearCache`, etc.), so this is fine as-is. Just flagging for context: if the AST hierarchy ever moves to a fully type-parameterized visitor, this site is one of many that will need updating. ########## iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/sql/util/DataNodeSqlFormatter.java: ########## @@ -134,7 +135,13 @@ public Void visitExplainAnalyze(ExplainAnalyze node, Integer indent) { @Override public Void visitShowDB(ShowDB node, Integer indent) { - builder.append("SHOW DATABASE"); + builder.append("SHOW DATABASES"); Review Comment: Good catch on the `SHOW DATABASE` → `SHOW DATABASES` singular/plural typo fix bundled in here. Worth a one-liner in the PR description so reviewers (and `git blame` archaeologists) can find it later. -- 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]
