refactor(exec): Move window files under exec/window#17710
Closed
zhli1142015 wants to merge 1 commit into
Closed
Conversation
✅ Deploy Preview for meta-velox canceled.
|
meta-cla
Bot
added
the
CLA Signed
Build Impact AnalysisSelective Build Targets (building these covers all 317 affected)Total affected: 317/581 targets
Affected targets (317)Directly changed (3)
Transitively affected (314)
Slow path • Graph generated from PR branch |
mbasmanova
requested changes
Jun 3, 2026
Contributor
mbasmanova
left a comment
mbasmanova
left a comment
There was a problem hiding this comment.
Thank you for the quick turnaround!
- Consider keeping public-facing files in
exec/andexecnamespace — only move implementation details intoexec/window/:Window.h/.cpp— the operator itself. Other operators (HashJoin,Aggregate,TableScan) live inexec/.WindowFunction.h/.cpp— the public API that window function authors implement against. Similar toAggregateFunction.hwhich lives inexec/.- The rest (
WindowBuild,WindowPartition,PeerGroupComputation,KRangeFrameBound, etc.) are internal implementation details — these belong inexec/window/.
zhli1142015
force-pushed
the
move-window-files-to-exec-window
branch
from
710faa0 to
55c53f8
Compare
Contributor
Author
|
Thank you, @mbasmanova, this makes a lot of sense! I've updated the PR accordingly:
The operator and public API reference the internal types through the |
mbasmanova
approved these changes
Jun 3, 2026
Contributor
mbasmanova
left a comment
mbasmanova
left a comment
There was a problem hiding this comment.
Thank you for the updates!
WindowPartition.his also part of the public API — every window function implementation uses it (FirstLastValue,LeadLag,Rank,Ntile, etc.). Keep it inexec/alongsideWindow.handWindowFunction.h.- There should be no changes outside
exec/at all. Public APIs (Window,WindowFunction,WindowPartition) stay inexec/— same files, same namespace, same include paths. Only internal implementation details move.
mbasmanova
added
the
ready-to-merge
zhli1142015
force-pushed
the
move-window-files-to-exec-window
branch
from
55c53f8 to
5ad74d9
Compare
Contributor
Author
|
Thanks for the review and approval, @mbasmanova! I've updated the PR accordingly: |
mbasmanova
reviewed
Jun 3, 2026
| functions::prestosql::registerAllScalarFunctions(); | ||
| parse::registerTypeResolver(); | ||
| window::prestosql::registerAllWindowFunctions(); | ||
| velox::window::prestosql::registerAllWindowFunctions(); |
Contributor
There was a problem hiding this comment.
do we need these changes outside of exec/ dir?
Contributor
Author
There was a problem hiding this comment.
Good catch, thank you! You're right — this one wasn't needed. I've reverted it here and in the other files that don't reference the moved headers (LocalPartitionTest.cpp, WindowFunctionRegistryTest.cpp, and the two window benchmarks); those are now unchanged from main. The only remaining qualification is in WindowTest.cpp, which directly includes the moved exec/window build headers, so within facebook::velox::exec the unqualified window:: now refers to exec::window and has to be spelled velox::window::prestosql explicitly. No files outside velox/exec/ change.
zhli1142015
force-pushed
the
move-window-files-to-exec-window
branch
from
5ad74d9 to
400f129
Compare
The window operator code mixes public API with internal implementation details in velox/exec/. Window, WindowFunction, and WindowPartition are the public API used by window function implementations; the build strategies, aggregate-as-window adapter, and frame/peer-group helpers are internal to the operator. Move the internal pieces to velox/exec/window/ under the facebook::velox::exec::window namespace, leaving the public API (Window, WindowFunction, WindowPartition) unchanged in velox/exec/ and the facebook::velox::exec namespace. No files outside velox/exec/ change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
zhli1142015
force-pushed
the
move-window-files-to-exec-window
branch
from
400f129 to
e89c26d
Compare
|
@bikramSingh91 has imported this pull request. If you are a Meta employee, you can view this in D107420866. |
Collaborator
|
Very nice ! When I had attempted this few years ago there weren't as many files as now, so this is great to see ! |
aditi-pandit
approved these changes
Jun 3, 2026
Collaborator
aditi-pandit
left a comment
aditi-pandit
left a comment
There was a problem hiding this comment.
Thanks !
I had abandoned this idea few years ago as there weren't so many files in window, but its amazing to see there are so many now :) Have ideas for some more if I get to it :)
|
@bikramSingh91 merged this pull request in 2e129c5. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Preparatory, mechanical refactor for #17558. The window subsystem lived as a flat set of files under
velox/exec/. This moves the internal implementation files into a dedicatedvelox/exec/window/directory andfacebook::velox::exec::windownamespace, while keeping the public-facing files invelox/exec/and thefacebook::velox::execnamespace.Stay in
velox/exec/(execnamespace):Window.h/.cpp— the operator itself, alongsideHashJoin,Aggregate, andTableScan.WindowFunction.h/.cpp— the public API that window function authors implement against, likeAggregateFunction.h.Move under
velox/exec/window/(exec::windownamespace):WindowBuildand its variants,WindowPartition,AggregateWindow,KRangeFrameBound,PeerGroupComputation,WindowPartitionAccessor— internal implementation details.There are no behavior changes; only file locations, the enclosing namespace of the internal files, and references to them change. The operator and the public API reference the internal types through the
window::namespace.