Copilot commented on code in PR #63732:
URL: https://github.com/apache/doris/pull/63732#discussion_r3309438861
##########
fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java:
##########
@@ -1369,9 +1369,9 @@ public void checkQuerySlotCount(String slotCnt) {
@VarAttrDef.VarAttr(name = COLOCATE_MAX_PARALLEL_NUM, needForward = true,
fuzzy = false)
public int colocateMaxParallelNum = 128;
- @VarAttrDef.VarAttr(name = PARALLEL_PIPELINE_TASK_NUM, fuzzy = true,
needForward = true,
+ @VarAttrDef.VarAttr(name = PARALLEL_PIPELINE_TASK_NUM, fuzzy = false,
needForward = true,
setter = "setPipelineTaskNum")
- public int parallelPipelineTaskNum = 0;
+ public int parallelPipelineTaskNum = 1;
Review Comment:
Changing the default of `parallelPipelineTaskNum` from `0` to `1` is a much
broader behavior change than what the PR describes. Per
`getParallelExecInstanceNum` (SessionVariable.java:4450-4456), `0` means
auto-parallelism (`(MinPipelineExecutorSize + 1) / 2`, capped by
`maxInstanceNum`), while `1` forces every fragment to run with a single
parallel instance per BE. This affects parallelism for all queries on all
clusters — not just non-distinct aggregates on single-BE deployments — and
effectively disables intra-fragment parallelism by default for every operator
type (scans, joins, sorts, window, etc.). That is a significant performance
regression for the typical multi-core BE case and is independent of the stated
optimization in `SplitAggWithoutDistinct`/`RequestPropertyDeriver`. If the goal
is only to gate the new single-instance aggregate behavior, please keep the
default `0` (auto) and instead detect "single instance" using the resolved
instance count (e.g. `ConnectContext
.getTotalInstanceNum() == 1`) without changing the global default. If the
default change is intentional, it should be called out explicitly in the PR
description/release note and validated separately.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java:
##########
@@ -3718,7 +3718,6 @@ public void initFuzzyModeVariables() {
Random random = new SecureRandom();
this.feDebug = true;
this.enableConditionCache = Config.pull_request_id % 2 == 0;
- this.parallelPipelineTaskNum = random.nextInt(8);
this.parallelPrepareThreshold = random.nextInt(32) + 1;
Review Comment:
Removing the fuzzy randomization of `parallelPipelineTaskNum` reduces test
coverage diversity for this variable across regression runs (it previously
cycled through 0..7, including auto-parallel and single-instance modes).
Combined with changing the default to `1`, fuzzy runs will now uniformly
exercise only single-instance parallelism for any test that does not explicitly
set this variable, which may mask parallelism-related regressions. Consider
keeping the fuzzy randomization (or replacing it with a randomization that
still includes `0` and values `>1`) to retain coverage of the auto and
multi-instance code paths.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/AggregateUtils.java:
##########
@@ -90,6 +90,18 @@ public static boolean maybeUsingStreamAgg(List<Expression>
groupExpressions, Agg
&& param.aggPhase.isLocal();
}
+ /** Whether the plan will run with one fragment instance on one BE. */
+ public static boolean isSingleExecutionInstance(ConnectContext
connectContext) {
+ int beNumber =
connectContext.getSessionVariable().getBeNumberForTest();
+ if (beNumber < 0) {
+ beNumber =
connectContext.getEnv().getClusterInfo().getAllBackendByCurrentCluster(true).size();
+ }
+ beNumber = Math.max(1, beNumber);
+ String clusterName =
connectContext.getSessionVariable().resolveCloudClusterName(connectContext);
+ int parallelInstance = Math.max(1,
connectContext.getSessionVariable().getParallelExecInstanceNum(clusterName));
+ return beNumber == 1 && parallelInstance == 1;
Review Comment:
`isSingleExecutionInstance` re-implements logic that already exists on
`ConnectContext`: `getAliveBeNumber()` (which itself uses
`SystemInfoService.getBackendsNumber(true)` and already honors
`beNumberForTest`) and `getParallelInstanceNum()`, and these are combined as
`getTotalInstanceNum()`. This new helper could be expressed as
`connectContext.getTotalInstanceNum() == 1` (or `getAliveBeNumber() == 1 &&
getParallelInstanceNum() == 1` if you want to require both factors to be
exactly 1 rather than just the product). Reusing the existing helpers avoids
divergent handling of `beNumberForTest` and cluster resolution between this
code path and `SystemInfoService.getBackendsNumber`/`CostModel`.
--
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]
