Refine `StatefulPlanner.plan` by lgbo-ustc · Pull Request #22 · bigo-sg/velox

lgbo-ustc · 2026-01-28T11:47:12Z

No description provided.

Co-authored-by: Cursor <cursoragent@cursor.com>

lgbo-ustc · 2026-01-30T02:03:26Z

@copilot

Copilot · 2026-01-30T02:03:34Z

@lgbo-ustc I've opened a new pull request, #23, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot

Pull request overview

This PR refactors the StatefulPlanner::plan method by replacing static methods with instance methods and introducing a more modular transformation architecture. The changes improve code organization by extracting node-specific transformation logic into dedicated methods.

Changes:

Converted StatefulPlanner from a collection of static utility functions into a class with instance methods that hold context
Extracted operator transformation logic into separate methods per operator type (e.g., transformWatermarkAssignerOperator, transformStreamJoinOperator)
Removed unused includes and consolidated operator ID generation into a single static function

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
velox/experimental/stateful/StatefulPlanner.h	Updated class to have instance methods and member variables, added protected constructor and private transformation methods
velox/experimental/stateful/StatefulPlanner.cpp	Refactored monolithic transformation methods into specialized per-operator-type methods, removed unused includes

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-30T02:08:19Z

velox/experimental/stateful/StatefulPlanner.h

      StateBackend* stateBackend);
-
+ protected:
+    StatefulPlanner(exec::DriverCtx* ctx, StateBackend* stateBackend) : ctx_(ctx),stateBackend_(stateBackend)  {}


Missing space after comma in initializer list. Add space after ctx_(ctx), to maintain consistent formatting.

Suggested change

StatefulPlanner(exec::DriverCtx* ctx, StateBackend* stateBackend) : ctx_(ctx),stateBackend_(stateBackend) {}

StatefulPlanner(exec::DriverCtx* ctx, StateBackend* stateBackend) : ctx_(ctx), stateBackend_(stateBackend) {}

Copilot · 2026-01-30T02:08:19Z

velox/experimental/stateful/StatefulPlanner.cpp

+            aggsHandlerNode,
+            std::make_unique<AggsHandleFunction>(), // TODO: not complete yet
+            0, // stateRetentionTime: default to 0
+            aggsHandlerNode->generateUpdateBefore());


The parameter order has changed compared to the removed code. In the old code, generateUpdateBefore() was the 5th parameter and needRetraction() was the 6th. Now a hardcoded 0 appears as the 5th parameter with a comment indicating it's stateRetentionTime. Verify that this change is intentional and that the correct parameters are being passed in the correct order.

Suggested change

aggsHandlerNode->generateUpdateBefore());

aggsHandlerNode->generateUpdateBefore(),

aggsHandlerNode->needRetraction());

Copilot · 2026-01-30T02:08:19Z

velox/experimental/stateful/StatefulPlanner.cpp

+            deduplicateNode->minRetentionTime(),
+            deduplicateNode->rowtimeIndex(),


Parameter order changed: previously rowtimeIndex() was the 3rd parameter and minRetentionTime() was the 4th. Now they are swapped. This will cause incorrect behavior if the constructor signature hasn't changed accordingly.

Suggested change

deduplicateNode->minRetentionTime(),

deduplicateNode->rowtimeIndex(),

deduplicateNode->rowtimeIndex(),

deduplicateNode->minRetentionTime(),

Copilot · 2026-01-30T02:08:20Z

velox/experimental/stateful/StatefulPlanner.cpp

+        std::shared_ptr<KeySelector> sortKeySelector =
+            std::make_shared<KeySelector>(


Changed from std::unique_ptr to std::shared_ptr for sortKeySelector. The variable is then moved on line 453, which is unusual for a shared_ptr. Verify this ownership change is intentional and that the shared ownership semantics are required.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-30T02:22:20Z

velox/experimental/stateful/StatefulPlanner.cpp

        std::make_unique<KeySelector>(
-            std::move(joinNode->leftPartFuncSpec()->create(INT_MAX, false)),
-            op->pool(),
+            joinNode->leftPartFuncSpec()->create(INT_MAX, false),


The magic number INT_MAX appears multiple times throughout the code (lines 163, 168, 194, 199, 224, 228, 271, 275, 309, 328, 446). Consider defining this as a named constant to improve code clarity and maintainability.

Copilot · 2026-01-30T02:22:20Z

velox/experimental/stateful/StatefulPlanner.cpp

+            ctx_,
+            topNNode,
+            std::move(op),
+            sortKeySelector,


Potential use-after-move issue: sortKeySelector is moved on line 452 and then used again on line 453. After std::move(op) on line 452, using sortKeySelector on the next line will result in passing a moved-from object.

- Refactor nodeToStatefulOperator/nodeToOperator to transform* methods - Add support for GroupWindowAggsHandlerNode, GroupAggsHandlerNode, DeduplicateNode, StreamTopNNode - Fix RowTimeDeduplicateRanker parameter order: minRetentionTime should come before rowtimeIndex - Fix GroupAggregator parameter order: use stateRetentionTime=0 and generateUpdateBefore - Remove unused headers: CallbackSink, Exchange, HashBuild, Merge, NestedLoopJoinBuild, RoundRobinPartitionFunction - Add FIXME comment for stateRetentionTime handling in GroupAggregator

lgbo-ustc and others added 3 commits January 28, 2026 09:51

refind StatefulPlanner::plan

5b84f92

refine StatefulPlanner

e2c291c

Co-authored-by: Cursor <cursoragent@cursor.com>

clear

c530fc1

lgbo-ustc force-pushed the task_build branch from 0952a58 to 14820c7 Compare January 30, 2026 02:02

lgbo-ustc marked this pull request as ready for review January 30, 2026 02:03

Copilot AI mentioned this pull request Jan 30, 2026

Refine StatefulPlanner.plan #23

Closed

update

2f81a17

lgbo-ustc force-pushed the task_build branch from 14820c7 to 2f81a17 Compare January 30, 2026 02:04

lgbo-ustc requested a review from Copilot January 30, 2026 02:07

Copilot AI reviewed Jan 30, 2026

View reviewed changes

fix

9435733

lgbo-ustc force-pushed the task_build branch from 888db29 to 9435733 Compare January 30, 2026 02:19

lgbo-ustc requested a review from Copilot January 30, 2026 02:21

Copilot AI reviewed Jan 30, 2026

View reviewed changes

lgbo-ustc force-pushed the task_build branch from 66a43f7 to 89303d9 Compare January 30, 2026 02:39

Format StatefulPlanner.h to match Velox code style

a52e618

lgbo-ustc merged commit adcb064 into gluten-0530 Jan 30, 2026

lgbo-ustc mentioned this pull request Jan 30, 2026

Bump velox bigo-sg/velox4j#22

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refine `StatefulPlanner.plan`#22

Refine `StatefulPlanner.plan`#22
lgbo-ustc merged 7 commits intogluten-0530from
task_build

lgbo-ustc commented Jan 28, 2026

Uh oh!

lgbo-ustc commented Jan 30, 2026

Uh oh!

Copilot AI commented Jan 30, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 30, 2026

Uh oh!

Copilot AI Jan 30, 2026

Uh oh!

Copilot AI Jan 30, 2026

Uh oh!

Copilot AI Jan 30, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 30, 2026

Uh oh!

Copilot AI Jan 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	StatefulPlanner(exec::DriverCtx* ctx, StateBackend* stateBackend) : ctx_(ctx),stateBackend_(stateBackend) {}
	StatefulPlanner(exec::DriverCtx* ctx, StateBackend* stateBackend) : ctx_(ctx), stateBackend_(stateBackend) {}

	aggsHandlerNode->generateUpdateBefore());
	aggsHandlerNode->generateUpdateBefore(),
	aggsHandlerNode->needRetraction());

		deduplicateNode->minRetentionTime(),
		deduplicateNode->rowtimeIndex(),

		std::shared_ptr<KeySelector> sortKeySelector =
		std::make_shared<KeySelector>(

Conversation

lgbo-ustc commented Jan 28, 2026

Uh oh!

lgbo-ustc commented Jan 30, 2026

Uh oh!

Copilot AI commented Jan 30, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants