This is an automated email from the ASF dual-hosted git repository.
yiguolei pushed a commit to branch branch-2.1
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/branch-2.1 by this push:
new c83c2e18bc0 [fix](planner) fix fallback to legacy planner compute
wrong result (#49913)
c83c2e18bc0 is described below
commit c83c2e18bc0e12a981675579c734683c10f300d9
Author: 924060929 <lanhuaj...@selectdb.com>
AuthorDate: Thu Apr 10 23:22:18 2025 +0800
[fix](planner) fix fallback to legacy planner compute wrong result (#49913)
### What problem does this PR solve?
doris 3.x not need this fix because the legacy planner already be
deleted
what's wrong:
1. when nereids planner can not parse the sql, doris will fallback to
legacy planner and maybe handle this case, but we not set the
`enable_nereids_planner` to `false` for legacy planner
2. the `ignore_storage_data_distribution` only support when
`enable_nereids_planner` is `true`
3. the legacy planner will use one phase aggregation when input fragment
only has one instance
4. the legacy planner maybe compute one instance by invoke
`OlapScanNode#getNumInstances()` if `enable_nereids_planner == true &&
ignore_storage_data_distribution == true`, even the data place at
multiple backends, then the aggregation will compute wrong result
so when fallback to legacy planner, the legacy planner should set
`enable_nereids_planner` to `false`, then the input OlapScanNode will
not compute one instance, and the AggregationNode will use two phases to
compute the right result.
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [x] Regression test
- [ ] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
- Behavior changed:
- [x] No.
- [ ] Yes. <!-- Explain the behavior change -->
- Does this need documentation?
- [x] No.
- [ ] Yes. <!-- Add document PR link here. eg:
https://github.com/apache/doris-website/pull/1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR
should merge into -->
---
.../java/org/apache/doris/qe/StmtExecutor.java | 7 +-
.../test_fallback_to_legacy_planner.groovy | 90 ++++++++++++++++++++++
2 files changed, 96 insertions(+), 1 deletion(-)
diff --git a/fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java
b/fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java
index 8f1a6036950..c268ff4d4a4 100644
--- a/fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java
+++ b/fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java
@@ -894,12 +894,17 @@ public class StmtExecutor {
&& !(parsedStmt instanceof TransactionStmt)) {
throw new TException("This is in a transaction, only insert,
commit, rollback is acceptable.");
}
+ SessionVariable sessionVariable = context.getSessionVariable();
+ if (!(parsedStmt instanceof SetStmt) && !(parsedStmt instanceof
UnsetVariableStmt)) {
+ sessionVariable.disableNereidsPlannerOnce();
+ }
+
// support select hint e.g. select /*+ SET_VAR(query_timeout=1) */
sleep(3);
analyzeVariablesInStmt();
if (!context.isTxnModel()) {
// analyze this query
- analyze(context.getSessionVariable().toThrift());
+ analyze(sessionVariable.toThrift());
if (isForwardToMaster()) {
// before forward to master, we also need to set
profileType in this node
diff --git
a/regression-test/suites/correctness/test_fallback_to_legacy_planner.groovy
b/regression-test/suites/correctness/test_fallback_to_legacy_planner.groovy
new file mode 100644
index 00000000000..e3ff8bc780a
--- /dev/null
+++ b/regression-test/suites/correctness/test_fallback_to_legacy_planner.groovy
@@ -0,0 +1,90 @@
+// 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.
+
+import org.apache.doris.regression.util.JdbcUtils
+
+import java.util.stream.Collectors
+
+suite("test_fallback_to_legacy_planner") {
+ multi_sql """
+ create database if not exists test_legacy_planner;
+ use test_legacy_planner;
+ set enable_nereids_planner=false;
+
+ drop view if exists view_fallback_to_legacy_planner;
+ drop table if exists test_fallback_to_legacy_planner;
+
+ CREATE TABLE `test_fallback_to_legacy_planner`(
+ `id` int NULL
+ )
+ ENGINE=OLAP
+ DUPLICATE KEY(`id`)
+ DISTRIBUTED BY HASH(`id`) BUCKETS 3
+ PROPERTIES ( "replication_allocation" = "tag.location.default: 1");
+
+ insert into test_fallback_to_legacy_planner select number from
numbers('number'='100');
+
+ CREATE VIEW `view_fallback_to_legacy_planner` AS SELECT
+
`default_cluster:test_legacy_planner.test_fallback_to_legacy_planner`.`id` AS
`id`
+ FROM
`default_cluster:test_legacy_planner`.`test_fallback_to_legacy_planner`;
+ """
+
+ def assertResult = { String sqlStr, String expectLabel, int
expectFragmentNum, List<List<Object>> expectResult ->
+ def (explainResult, meta) =
JdbcUtils.executeQueryToList(context.getConn(), "explain ${sqlStr}")
+ def resultLabel = meta.getColumnLabel(1)
+ log.info("explain result:\n${explainResult.get(0).get(0)}")
+
+ def explainResultString = explainResult.stream()
+ .map { it.get(0) }
+ .collect(Collectors.joining("\n"))
+
+ assertEquals(expectLabel, resultLabel)
+ assertEquals(expectFragmentNum, explainResultString.findAll("PLAN
FRAGMENT").size())
+
+ test {
+ sql "$sqlStr"
+ result(expectResult)
+ }
+ }
+ sql "set ignore_storage_data_distribution=true"
+ sql "set enable_local_shuffle=true"
+ sql "set enable_pipeline_x_engine=true"
+ sql "set enable_pipeline_engine=true"
+
+ sql "set enable_nereids_planner=false"
+ assertResult(
+ "select count(*) from view_fallback_to_legacy_planner",
+ "Explain String(Old Planner)",
+ 2, // fragment num == 2
+ [[100L]]
+ )
+
+ sql "set enable_nereids_planner=true"
+ assertResult(
+ "select count(*) from view_fallback_to_legacy_planner",
+ "Explain String(Old Planner)",
+ 2, // fragment num == 2
+ [[100L]]
+ )
+
+ assertResult(
+ "select count(*) from test_fallback_to_legacy_planner",
+ "Explain String(Nereids Planner)",
+ 2, // fragment num == 2
+ [[100L]]
+ )
+}
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@doris.apache.org
For additional commands, e-mail: commits-h...@doris.apache.org
