Copilot commented on code in PR #54822:
URL: https://github.com/apache/doris/pull/54822#discussion_r2279599367
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -8466,6 +8467,75 @@ public LogicalPlan
visitWarmUpCluster(DorisParser.WarmUpClusterContext ctx) {
return new WarmUpClusterCommand(warmUpItems, srcCluster, dstCluster,
isForce, isWarmUpWithTable, properties);
}
+ @Override
+ public LogicalPlan visitWarmUpSelect(DorisParser.WarmUpSelectContext ctx) {
+ LogicalPlan relation;
+ if (ctx.warmUpSingleTableRef() == null) {
+ relation = new
LogicalOneRowRelation(StatementScopeIdGenerator.newRelationId(),
+ ImmutableList.of(new Alias(Literal.of(0))));
Review Comment:
Creating a LogicalOneRowRelation when no table reference is provided doesn't
make sense for WARM UP SELECT, which should require a table to warm up. This
logic should throw an exception instead.
```suggestion
throw new AnalysisException("WARM UP SELECT requires a table
reference to warm up.");
```
##########
fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java:
##########
@@ -228,6 +228,8 @@ public class StmtExecutor {
// The profile of this execution
private final Profile profile;
private Boolean isForwardedToMaster = null;
+ // Handler to process and send blackhole query results
+ private BlackholeResultHandler blackholeResultHandler = new
BlackholeResultHandler();
Review Comment:
Creating a new BlackholeResultHandler instance for every StmtExecutor is
inefficient. Consider making it static or using a shared instance since it
appears to be stateless between queries.
```suggestion
private static final BlackholeResultHandler BLACKHOLE_RESULT_HANDLER =
new BlackholeResultHandler();
```
##########
regression-test/suites/external_table_p0/cache/test_hive_warmup_select.groovy:
##########
@@ -0,0 +1,95 @@
+// 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.
+
+suite("test_hive_warmup_select",
"p0,external,hive,external_docker,external_docker_hive") {
+ String enabled = context.config.otherConfigs.get("enableHiveTest")
+ if (enabled == null || !enabled.equalsIgnoreCase("true")) {
+ logger.info("diable Hive test.")
Review Comment:
Typo in log message: 'diable' should be 'disable'
```suggestion
logger.info("disable Hive test.")
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -8466,6 +8467,75 @@ public LogicalPlan
visitWarmUpCluster(DorisParser.WarmUpClusterContext ctx) {
return new WarmUpClusterCommand(warmUpItems, srcCluster, dstCluster,
isForce, isWarmUpWithTable, properties);
}
+ @Override
+ public LogicalPlan visitWarmUpSelect(DorisParser.WarmUpSelectContext ctx) {
+ LogicalPlan relation;
+ if (ctx.warmUpSingleTableRef() == null) {
+ relation = new
LogicalOneRowRelation(StatementScopeIdGenerator.newRelationId(),
+ ImmutableList.of(new Alias(Literal.of(0))));
+ } else {
+ relation = visitWarmUpSingleTableRef(ctx.warmUpSingleTableRef());
+ }
+
+ LogicalPlan filter = withFilter(relation,
Optional.ofNullable(ctx.whereClause()));
+
+ List<Expression> projectList =
visitNamedExpressionSeq(ctx.namedExpressionSeq());
+
+ for (Expression expr : projectList) {
+ if (!isSimpleColumnReference(expr)) {
+ throw new AnalysisException("WARM UP SELECT only supports
simple column references, "
+ + "aggregate functions and complex expressions are not
allowed");
+ }
+ }
+
+ LogicalProject project = new LogicalProject(projectList, filter);
+
+ if (Config.isNotCloudMode() &&
(!ConnectContext.get().getSessionVariable().isEnableFileCache())) {
+ throw new AnalysisException("WARM UP SELECT requires session
variable"
+ + " enable_file_cache=true");
+ }
+
+ if (Config.isCloudMode() &&
ConnectContext.get().getSessionVariable().isDisableFileCache()) {
+ throw new AnalysisException("WARM UP SELECT requires session
variable"
+ + " disable_file_cache=false in cloud mode");
+ }
+
+ LogicalSink<?> sink = new UnboundBlackholeSink<>(project);
+ return withExplain(sink, ctx.explain());
+ }
+
+ @Override
+ public LogicalPlan
visitWarmUpSingleTableRef(DorisParser.WarmUpSingleTableRefContext ctx) {
+ List<String> nameParts =
visitMultipartIdentifier(ctx.multipartIdentifier());
+
+ // Create a simple UnboundRelation for warm up queries
+ UnboundRelation relation = new UnboundRelation(
+ StatementScopeIdGenerator.newRelationId(),
+ nameParts);
+
+ LogicalPlan checkedRelation =
LogicalPlanBuilderAssistant.withCheckPolicy(relation);
+ LogicalPlan plan = withTableAlias(checkedRelation, ctx.tableAlias());
+ return plan;
+ }
+
+ /**
+ * Check if an expression is a simple column reference (not aggregate
functions or complex expressions)
+ */
+ private boolean isSimpleColumnReference(Expression expr) {
+ // Allow simple column references
+ if (expr instanceof Slot) {
+ return true;
+ }
+
+ // Allow star expressions (*)
+ if (expr instanceof UnboundStar) {
+ return true;
+ }
+
Review Comment:
The comment mentions rejecting function calls and arithmetic expressions,
but the method doesn't check for UnboundAlias which could wrap complex
expressions. Consider unwrapping aliases to check the underlying expression.
```suggestion
// Unwrap UnboundAlias recursively
while (expr instanceof UnboundAlias) {
expr = ((UnboundAlias) expr).child();
}
// Allow simple column references
if (expr instanceof Slot) {
return true;
}
// Allow star expressions (*)
if (expr instanceof UnboundStar) {
return true;
}
```
##########
be/src/pipeline/exec/blackhole_sink_operator.cpp:
##########
@@ -0,0 +1,208 @@
+// 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.
+
+#include "blackhole_sink_operator.h"
+
+#include <fmt/format.h>
+#include <gen_cpp/PaloInternalService_types.h>
+
+#include <sstream>
+
+#include "common/logging.h"
+#include "common/status.h"
+#include "pipeline/dependency.h"
+#include "runtime/exec_env.h"
+#include "runtime/result_buffer_mgr.h"
+#include "runtime/runtime_state.h"
+#include "util/mysql_row_buffer.h"
+#include "util/runtime_profile.h"
+#include "vec/core/block.h"
+
+namespace doris {
+namespace pipeline {
+
+BlackholeSinkOperatorX::BlackholeSinkOperatorX(
+ int operator_id, const int dest_id, const TDataStreamSink& sink,
+ const std::vector<TPlanFragmentDestination>& destinations)
+ : Base(operator_id, 0, dest_id), _t_data_stream_sink(sink),
_destinations(destinations) {}
+
+Status BlackholeSinkOperatorX::prepare(RuntimeState* state) {
+ std::shared_ptr<ResultBlockBufferBase> sender_base = nullptr;
+ RETURN_IF_ERROR(state->exec_env()->result_mgr()->create_sender(
+ state->query_id(), 4096, &sender_base, state, false, nullptr));
+ _sender =
std::dynamic_pointer_cast<vectorized::MySQLResultBlockBuffer>(sender_base);
+ return Status::OK();
+}
+
+Status BlackholeSinkOperatorX::init(const TDataSink& tsink) {
+ RETURN_IF_ERROR(DataSinkOperatorXBase::init(tsink));
+ return Status::OK();
+}
+
+Status BlackholeSinkOperatorX::sink(RuntimeState* state, vectorized::Block*
block, bool eos) {
+ auto& local_state = get_local_state(state);
+ SCOPED_TIMER(local_state.exec_time_counter());
+ COUNTER_UPDATE(local_state.rows_input_counter(), (int64_t)block->rows());
+
+ if (block && block->rows() > 0) {
+ RETURN_IF_ERROR(_process_block(state, block));
+ }
+
+ if (eos) {
+ _collect_cache_metrics(state, local_state);
+
+ RETURN_IF_ERROR(_send_cache_metrics_batch(state, local_state));
+ }
+
+ return Status::OK();
+}
+
+Status BlackholeSinkOperatorX::_process_block(RuntimeState* state,
vectorized::Block* block) {
+ auto& local_state = get_local_state(state);
+
+ // Update metrics - count rows and bytes processed
+ local_state._rows_processed += block->rows();
+ local_state._bytes_processed += block->bytes();
+
+ // Update runtime counters
+ if (local_state._rows_processed_timer) {
+ COUNTER_UPDATE(local_state._rows_processed_timer, block->rows());
+ }
+ if (local_state._bytes_processed_timer) {
+ COUNTER_UPDATE(local_state._bytes_processed_timer, block->bytes());
+ }
+
+ // The BLACKHOLE discard the data
Review Comment:
Grammar error in comment: 'The BLACKHOLE discard the data' should be 'The
BLACKHOLE discards the data'
```suggestion
// The BLACKHOLE discards the data
```
--
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]
