Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[[YQL]] Adapt WideLastCombinerWrapper to work with spilling in LLVM #5416

Closed
Darych opened this issue Jun 10, 2024 · 2 comments · Fixed by #6277 or #7304
Closed

[[YQL]] Adapt WideLastCombinerWrapper to work with spilling in LLVM #5416

Darych opened this issue Jun 10, 2024 · 2 comments · Fixed by #6277 or #7304
Assignees
@Tony-Romanov
Labels
area/yql YQL query language issues

Comments

@Darych
Copy link
Collaborator

Darych commented Jun 10, 2024

Currently, spilling in WideLastCombiner works just when LLVM is disabled.
It will be great to be able to use spilling in WideLastCombiner when LLVM is turned on.
@Darych Darych added the area/yql YQL query language issues label Jun 10, 2024
@Tony-Romanov
Copy link
Collaborator

Tony-Romanov commented Jun 12, 2024 •
edited
Loading

Get rid of using comp nodes inside state.
https://github.com/Tony-Romanov/ydb/blob/main/ydb/library/yql/minikql/comp_nodes/mkql_wide_combine.cpp#L352
Comp nodes must be used only directly from DoCalculate body.

@Tony-Romanov
Copy link
Collaborator

Tony-Romanov commented Jun 28, 2024 •
edited
Loading

Another problem that I didn’t notice at first.
Please return back members instead of this methods:
https://github.com/ydb-platform/ydb/blob/main/ydb/library/yql/minikql/comp_nodes/mkql_wide_combine.cpp#L415-L442

As here:
https://github.com/ydb-platform/ydb/blob/main/ydb/library/yql/minikql/comp_nodes/mkql_wide_combine.cpp#L307-L308

@Tony-Romanov Tony-Romanov linked a pull request Jul 4, 2024 that will close this issue
Update LLVM part of combiner with spilling. #6277
Merged
@alexv-smirnov alexv-smirnov changed the title Adapt WideLastCombinerWrapper to work with spilling in LLVM [[YQL]] Adapt WideLastCombinerWrapper to work with spilling in LLVM Jul 9, 2024
@Tony-Romanov Tony-Romanov reopened this Jul 23, 2024
@Tony-Romanov Tony-Romanov linked a pull request Jul 31, 2024 that will close this issue
Implement LLVM for WideLastCombiner with spilling. #7304
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Tony-Romanov Tony-Romanov

Labels
area/yql YQL query language issues
Projects
None yet
Milestone
No milestone
2 participants
@Darych @Tony-Romanov