Copilot commented on code in PR #17270:
URL: https://github.com/apache/iotdb/pull/17270#discussion_r2928344727
##########
iotdb-core/antlr/src/main/antlr4/org/apache/iotdb/db/qp/sql/IoTDBSqlParser.g4:
##########
@@ -1239,6 +1240,11 @@ explain
: EXPLAIN (ANALYZE VERBOSE?)? selectStatement?
;
+// ---- Describe
+describe
+ : DESCRIBE selectStatement
Review Comment:
`DESCRIBE selectStatement` is added to the grammar, but there is no
corresponding handling in the statement AST construction (e.g.,
`org.apache.iotdb.db.queryengine.plan.parser.ASTVisitor` has no
`visitDescribe(...)`). With the current visitor defaults, `DESCRIBE <select>`
will be visited as just the inner `selectStatement`, effectively ignoring
`DESCRIBE` and executing the query normally. Please add a dedicated Statement
node + visitor handling so the DESCRIBE behavior is actually applied.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/execution/memory/TableModelStatementMemorySourceVisitor.java:
##########
@@ -130,6 +130,61 @@ public StatementMemorySource visitCountDevice(
node.getTsBlock(context.getAnalysis()), node.getDataSetHeader());
}
+ @Override
+ public StatementMemorySource visitDescribeQuery(
+ org.apache.iotdb.db.queryengine.plan.relational.sql.ast.DescribeQuery
node,
+ TableModelStatementMemorySourceContext context) {
+
Review Comment:
This `visitDescribeQuery` implementation uses many fully-qualified type
names, which makes the code hard to read and is likely to violate the
repository’s 100-char Checkstyle `LineLength` rule for non-import/package
lines. Please switch to normal imports (as done elsewhere in this file) and
rewrap long expressions to keep lines <= 100 chars.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/execution/memory/TableModelStatementMemorySourceVisitor.java:
##########
@@ -130,6 +130,61 @@ public StatementMemorySource visitCountDevice(
node.getTsBlock(context.getAnalysis()), node.getDataSetHeader());
}
+ @Override
+ public StatementMemorySource visitDescribeQuery(
+ org.apache.iotdb.db.queryengine.plan.relational.sql.ast.DescribeQuery
node,
+ TableModelStatementMemorySourceContext context) {
+
+ java.util.List<org.apache.iotdb.commons.schema.column.ColumnHeader>
columnHeaders =
+ new java.util.ArrayList<>();
+ columnHeaders.add(
+ new org.apache.iotdb.commons.schema.column.ColumnHeader(
+ "Column", org.apache.tsfile.enums.TSDataType.TEXT));
+ columnHeaders.add(
+ new org.apache.iotdb.commons.schema.column.ColumnHeader(
+ "Type", org.apache.tsfile.enums.TSDataType.TEXT));
+
+ org.apache.iotdb.db.queryengine.common.header.DatasetHeader datasetHeader =
+ new
org.apache.iotdb.db.queryengine.common.header.DatasetHeader(columnHeaders,
true);
+
+ java.util.List<String> columnNames = new java.util.ArrayList<>();
+ java.util.List<String> columnTypes = new java.util.ArrayList<>();
+
+ context
+ .getAnalysis()
+ .getOutputDescriptor()
+ .getVisibleFields()
+ .forEach(
+ field -> {
+ columnNames.add(field.getName().orElse("unknown"));
+ columnTypes.add(field.getType().toString());
+ });
+
+ org.apache.tsfile.read.common.block.TsBlockBuilder builder =
+ new org.apache.tsfile.read.common.block.TsBlockBuilder(
+ java.util.Arrays.asList(
+ org.apache.tsfile.enums.TSDataType.TEXT,
org.apache.tsfile.enums.TSDataType.TEXT));
+
+ for (int i = 0; i < columnNames.size(); i++) {
+ builder.getTimeColumnBuilder().writeLong(0);
+
+ builder
+ .getColumnBuilder(0)
+ .writeBinary(
+ new org.apache.tsfile.utils.Binary(
+
columnNames.get(i).getBytes(java.nio.charset.StandardCharsets.UTF_8)));
+ builder
+ .getColumnBuilder(1)
+ .writeBinary(
+ new org.apache.tsfile.utils.Binary(
+
columnTypes.get(i).getBytes(java.nio.charset.StandardCharsets.UTF_8)));
Review Comment:
When building the TsBlock here, you manually convert `String` to `byte[]`
with `StandardCharsets.UTF_8` and then wrap it in `Binary`. Elsewhere in the
codebase, memory-source blocks typically use `new Binary(str,
TSFileConfig.STRING_CHARSET)` (avoids an extra allocation and keeps charset
consistent). Please align with that pattern here.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/sql/ast/DescribeQuery.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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
Review Comment:
The ASF license header is malformed: it currently reads `Version 2.0 (the`
followed by `License");` (missing the opening quote before License). Please fix
the header to match the standard form used in the rest of the project.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/execution/QueryExecution.java:
##########
@@ -252,7 +252,7 @@ private ExecutionResult retry() {
}
private boolean skipExecute() {
- return analysis.canSkipExecute(context);
+ return analysis.isFailed() || analysis.isFinishQueryAfterAnalyze();
Review Comment:
`skipExecute()` no longer delegates to `analysis.canSkipExecute(context)`.
This drops the non-relational analysis logic that skips execution for queries
without a data source (see
`org.apache.iotdb.db.queryengine.plan.analyze.Analysis#canSkipExecute`), and
may cause unnecessary planning/execution work (or behavior changes) for such
queries. Please keep using `analysis.canSkipExecute(context)` (or re-implement
the missing conditions here) so the skip semantics remain consistent across
analysis implementations.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/transformation/datastructure/tv/ElasticSerializableTVList.java:
##########
@@ -284,4 +285,15 @@ public SerializableTVList get(int targetIndex) throws
IOException {
return internalTVList.get(targetIndex);
}
}
+
+ private Column ensureTimeColumn(Column column) {
+ if (column instanceof TimeColumn) {
+ return column;
+ }
+ long[] times = new long[column.getPositionCount()];
+ for (int i = 0; i < times.length; i++) {
+ times[i] = column.getLong(i);
+ }
+ return new TimeColumn(column.getPositionCount(), times);
+ }
Review Comment:
`ensureTimeColumn` copies the entire column by calling `getLong(i)` in a
loop. If this is only needed to satisfy `TsBlock`/serde requiring a concrete
`TimeColumn`, consider documenting that requirement here (and ideally keep the
return type as `TimeColumn`) so callers understand why a copy is necessary and
don’t accidentally introduce extra copies elsewhere.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/sql/ast/AstVisitor.java:
##########
@@ -449,6 +449,10 @@ protected R visitDescribeTable(DescribeTable node, C
context) {
return visitStatement(node, context);
}
+ protected R visitDescribeQuery(DescribeQuery node, C context) {
+ return visitStatement(node, context);
+ }
Review Comment:
Adding `visitDescribeQuery` to `AstVisitor` is not sufficient on its own:
visitors like `SqlFormatter` have a `visitNode` fallback that throws
`UnsupportedOperationException`, and there is currently no `visitDescribeQuery`
override there. Without adding formatter/traversal support, formatting/logging
a `DescribeQuery` will crash at runtime. Please implement `visitDescribeQuery`
in `SqlFormatter` (and any traversal visitors) to process/print the wrapped
query.
--
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]
