YQ supported override nodes placement #19787
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🟢 |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for specifying explicit node placement for query execution stages via the OverridePlanner pragma.
- Introduces a new
TasksNodesfield in the physical stage protobuf to carry node hints. - Implements JSON parsing and validation in
ApplyOverridePlannerSettingsand integrates it into query compilation. - Updates the executer to consume
TasksNodes, routing tasks to specified nodes or falling back to the existing planner.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| ydb/core/protos/kqp_physical.proto | Added TasksNodes repeated field to TKqpPhyStage. |
| ydb/core/kqp/query_compiler/kqp_query_compiler.cpp | Created ApplyOverridePlannerSettings and wired it into TKqpQueryCompiler. |
| ydb/core/kqp/executer_actor/kqp_scan_executer.cpp | Adjusted BuildComputeTasks call to include an explicit (empty) node-hint vector. |
| ydb/core/kqp/executer_actor/kqp_executer_impl.h | Added GetStageNodesHint helper and updated task-building logic to use hints. |
| ydb/core/kqp/executer_actor/kqp_data_executer.cpp | Flagged ResourceSnapshotRequired for overridden stages and passed hints to compute tasks. |
Comments suppressed due to low confidence (2)
ydb/core/kqp/executer_actor/kqp_executer_impl.h:1026
- [nitpick] The word "Planer" in
GetStageNodesHintand related variables is misspelled; consider renaming toGetStageNodesPlannerorGetStageNodesPlanand renamingnodesPlanertonodesPlannerfor clarity.
std::function<ui32(ui32 taskIdx)> GetStageNodesHint(const NKqpProto::TKqpPhyStage& stage, const TVector<NKikimrKqp::TKqpNodeResources>& resourceSnapshot) {
ydb/core/kqp/query_compiler/kqp_query_compiler.cpp:496
- Consider adding unit tests for
ApplyOverridePlannerSettings, covering valid and invalid JSON inputs (including node lists) to ensure overrides are parsed and errors are reported correctly.
TIssues ApplyOverridePlannerSettings(const TString& overridePlannerJson, NKqpProto::TKqpPhyQuery& queryProto) {
|
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
| for (ui32 i = 0; i < partitionsCount; ++i) { | ||
| auto& task = TasksGraph.AddTask(stageInfo); | ||
| task.Meta.Type = TTaskMeta::TTaskType::Compute; | ||
| if (!nodesPlanner) { | ||
| task.Meta.Type = TTaskMeta::TTaskType::Compute; |
There was a problem hiding this comment.
что тут происходит, зачем это?
| @@ -1023,6 +1023,27 @@ class TKqpExecuterBase : public TActor<TDerived> { | |||
| } | |||
| } | |||
|
|
|||
| std::function<ui32(ui32 taskIdx)> GetStageNodesHint(const NKqpProto::TKqpPhyStage& stage, const TVector<NKikimrKqp::TKqpNodeResources>& resourceSnapshot) { | |||
There was a problem hiding this comment.
зачем аж std::function ? можно проще?
There was a problem hiding this comment.
я предлагаю подумать и отказаться от этой идеи как таковой, а решать задачу в какой то другой парадигме. например если задача резделения нагрузки то запрещать использовать какие-то ноды.
назначать сотни тасок по нодам не имеет примерно никакого юзкейса
Changelog entry
Supported override nodes placement in pragma
OverridePlannerChangelog category
Description for reviewers