morningman commented on code in PR #69749:
URL: https://github.com/apache/doris/pull/69749#discussion_r4407997041
##########
fe/fe-core/src/main/java/org/apache/doris/load/routineload/RoutineLoadJob.java:
##########
@@ -438,6 +439,13 @@ protected void setOptional(CreateRoutineLoadInfo info)
throws UserException {
if (!StringUtils.isEmpty(info.getWorkloadGroupName())) {
jobProperties.put(WORKLOAD_GROUP, info.getWorkloadGroupName());
}
+
+ // Only write the key when the user declared it. An absent key means
"not declared" and
+ // keeps the existing implicit resolution, which is also what later
versions rely on to
+ // tell a pinned group apart from an inherited one.
+ if (!StringUtils.isEmpty(info.getComputeGroupName())) {
+ jobProperties.put(COMPUTE_GROUP, info.getComputeGroupName());
Review Comment:
Confirmed, and fixed in `7611a91f1d5`. The gap is in `getShowCreateInfo()`,
which this PR did not change. Its property list is the same on master and
branch-4.x and has no entry for `compute_group`, while SHOW CREATE MATERIALIZED
VIEW renders every MV property, so the MV half already round-trips. On master
no job could declare a group and the statement never carried the implicit one,
so a job recreated from it has always run in the recreating session's group.
For a job that declares its group, though, losing the declaration on export
defeats the point of declaring it. The statement now includes `compute_group`
from `getDeclaredComputeGroup()`, which follows ALTER, and only when a group is
declared. Jobs without a declaration keep their current output
(`RoutineLoadJobTest#testGetShowCreateInfo` is unchanged); writing the
effective group instead would turn an inherited group into a pinned one when
the statement is replayed.
`RoutineLoadJobTest#testGetShowCreateInfoCarriesDeclaredCom
puteGroup` checks the statement of a job that declares `cg_a` and is then
altered to `cg_b`, and `test_routine_load_compute_group` now also checks SHOW
CREATE ROUTINE LOAD after its ALTER.
The same list also leaves out `workload_group` and seven other properties
CREATE accepts (`send_batch_parallelism`, `load_to_single_tablet`,
`partial_update_new_key_behavior`, `unique_key_update_mode`, `enclose`,
`escape`, `empty_field_as_null`). That predates this PR and affects every
routine load job, so it is left to a separate change.
##########
fe/fe-core/src/main/java/org/apache/doris/job/extensions/mtmv/MTMVTask.java:
##########
@@ -1680,6 +1684,34 @@ private void setupComputeGroup(ConnectContext ctx) {
}
}
+ private void applyComputeGroup(ConnectContext ctx) {
Review Comment:
Confirmed, and fixed in `45cda592e43`. `dryRunRefresh()` builds its own
context with `MTMVPlanUtil.createMTMVContext()`, which copies the MV's
`workload_group` but sets no compute group. A non-empty delta was therefore
planned and run wherever `admin` resolves (its default group, otherwise the
first eligible one), and `NereidsCoordinator.enqueue` looked the workload group
up in that group; an empty delta is answered by the FE and never got that far.
The dry run now builds its context in `createDryRunConnectContext()`, which
binds the declared group and re-checks it before the delta query is built, as
the refresh paths in `MTMVTask` do. `EXPLAIN REFRESH` binds it to its planning
context as well, matching what the task does for its own planning context; it
runs nothing, so it does not re-check. An MV without a declaration keeps its
current resolution. Because the dry run only reads, the rows it returned were
correct; the problem was that it ran, and resolved its workload group, in a
group the MV is not bound to. `RefreshMTMVCommandTest` covers the binding of
both contexts (`testDryRunContextBindsDeclaredComputeGroup`,
`testExplainContextBindsDeclaredComputeGroup`), a dropped declared group
(`testDryRunRejectsDroppedDeclaredComputeGroup`), and an MV without a
declaration (`testContextsWithoutDeclarationKeepImplicitComputeGroup`).
The binding is not in `createMTMVContext()`. That initializer also builds
the contexts that only analyze the MV query (the rewrite cache built on user
queries, and the relation check after a base table change); a re-check there
would make a dropped group fail those user queries, and `MTMVTask` applies the
declared group and then the session's on top of it anyway. branch-4.x has
neither the dry run nor EXPLAIN REFRESH, so both paths exist only on master.
--
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]
