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

feat(streaming): support value meta in hash agg #1860

Merged
merged 2 commits into from
Apr 15, 2022
Merged

Conversation

xx01cyx
Copy link
Contributor

@xx01cyx xx01cyx commented Apr 15, 2022

What's changed and what's your intention?

Please explain IN DETAIL what the changes are in this PR and why they are needed:

  • Summarize your change
    • Support value meta in hash agg executor v2.
    • Introduce HashCode struct to bypass async closure problem.

Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests

@xx01cyx xx01cyx requested a review from fuyufjh April 15, 2022 04:59
@CLAassistant
Copy link

CLAassistant commented Apr 15, 2022

CLA assistant check
All committers have signed the CLA.

@xx01cyx xx01cyx changed the title Value meta hashagg feat(streaming): support value meta in hash agg Apr 15, 2022
@codecov
Copy link

codecov bot commented Apr 15, 2022

Codecov Report

Merging #1860 (681839b) into main (850aca0) will increase coverage by 0.10%.
The diff coverage is 98.46%.

@@            Coverage Diff             @@
##             main    #1860      +/-   ##
==========================================
+ Coverage   70.81%   70.91%   +0.10%     
==========================================
  Files         607      610       +3     
  Lines       79593    79685      +92     
==========================================
+ Hits        56363    56509     +146     
+ Misses      23230    23176      -54     
Flag Coverage Δ
rust 70.91% <98.46%> (+0.10%) ⬆️

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

Impacted Files Coverage Δ
src/batch/src/task/hash_shuffle_channel.rs 0.00% <0.00%> (ø)
src/common/src/util/ordered/serde.rs 99.51% <ø> (+6.69%) ⬆️
src/common/src/array/data_chunk.rs 94.69% <100.00%> (+0.04%) ⬆️
src/common/src/hash/key.rs 85.90% <100.00%> (+0.39%) ⬆️
src/storage/src/storage_value.rs 78.66% <100.00%> (+18.66%) ⬆️
src/storage/src/write_batch.rs 80.00% <100.00%> (+0.68%) ⬆️
.../src/executor/managed_state/aggregation/extreme.rs 90.26% <100.00%> (+0.37%) ⬆️
...ream/src/executor/managed_state/aggregation/mod.rs 79.45% <100.00%> (+0.57%) ⬆️
src/stream/src/executor_v2/agg.rs 97.22% <100.00%> (+0.07%) ⬆️
src/stream/src/executor_v2/global_simple_agg.rs 97.07% <100.00%> (+0.01%) ⬆️
... and 53 more

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

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


/// Consistent hash value to be set in value meta. Used for grouping the kv together in
/// storage. Each state will have the same consistent hash value.
consistent_hash_value: u16,
Copy link
Contributor

Choose a reason for hiding this comment

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

We may propose some shorter name for consistent_hash_value 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. What about place_key?

@xx01cyx xx01cyx merged commit 308fc29 into main Apr 15, 2022
@xx01cyx xx01cyx deleted the value-meta-hashagg branch April 15, 2022 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants