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.

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

refactor(frontend,streaming): separate output pk and internal key in topNExecutor #2258

Merged
merged 2 commits into from
May 1, 2022

Conversation

lmatz
Copy link
Contributor

@lmatz lmatz commented May 1, 2022

What's changed and what's your intention?

As #2130 suggests, the internal key used in TopNExecutor is different from the output primary key of TopNExecutor.

This PR separates these two concepts.

Notice that as previously, the column used in both order by and primary keys will be deduplicated when used as the internal key. Only the backend, not the frontend, is now aware of this optimization.

Checklist

  • I have written necessary docs and comments

Refer to a related PR or issue link (optional)

closes #2130
closes #2022

@lmatz lmatz marked this pull request as ready for review May 1, 2022 06:04
@codecov
Copy link

codecov bot commented May 1, 2022

Codecov Report

Merging #2258 (d0c1db2) into main (65a1958) will increase coverage by 0.00%.
The diff coverage is 80.00%.

@@           Coverage Diff           @@
##             main    #2258   +/-   ##
=======================================
  Coverage   71.24%   71.24%           
=======================================
  Files         655      655           
  Lines       83523    83544   +21     
=======================================
+ Hits        59507    59522   +15     
- Misses      24016    24022    +6     
Flag Coverage Δ
rust 71.24% <80.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/stream/src/executor/top_n.rs 0.00% <0.00%> (ø)
src/stream/src/executor/top_n_appendonly.rs 0.00% <0.00%> (ø)
...rc/frontend/src/optimizer/plan_node/stream_topn.rs 52.83% <40.00%> (-17.02%) ⬇️
src/stream/src/executor_v2/top_n.rs 91.03% <95.91%> (+0.14%) ⬆️
src/stream/src/executor_v2/hash_join.rs 81.69% <100.00%> (ø)
src/stream/src/executor_v2/top_n_appendonly.rs 94.46% <100.00%> (+0.03%) ⬆️
...c/executor_v2/managed_state/aggregation/extreme.rs 90.02% <0.00%> (-0.27%) ⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@lmatz lmatz requested a review from fuyufjh May 1, 2022 06:29
Copy link
Contributor

@fuyufjh fuyufjh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

src/frontend/src/optimizer/plan_node/stream_topn.rs Outdated Show resolved Hide resolved
@lmatz lmatz enabled auto-merge (squash) May 1, 2022 10:08
@lmatz lmatz merged commit d6c7b69 into main May 1, 2022
@lmatz lmatz deleted the lz/top_n branch May 1, 2022 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants