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

chore(query): refactor agg state merge #13153

Merged
merged 13 commits into from
Oct 11, 2023
Merged

chore(query): refactor agg state merge #13153

merged 13 commits into from
Oct 11, 2023

Conversation

sundy-li
Copy link
Member

@sundy-li sundy-li commented Oct 9, 2023 •
edited by drmingdrmer
Loading

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

Summary about this PR

  • Closes #issue

This change is Reviewable

@vercel
Copy link

vercel bot commented Oct 9, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
databend ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 11, 2023 3:22am

@sundy-li sundy-li added the ci-benchmark Benchmark: run all test label Oct 9, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 9, 2023

Docker Image for PR

  • tag: pr-13153-34d68fb

note: this image tag is only available for internal use,
please check the internal doc for more details.

@sundy-li
Copy link
Member Author

askbend:summary

@databend-bot
Copy link
Member

PR Summary(By llmchain.rs):

  • Refactoring of Aggregate Functions

    • The aggregate functions across multiple files have been refactored. The deserialize function has been renamed to merge and the merge function has been renamed to merge_states. The logic inside these functions has been updated to improve the merging of states. This change is aimed at improving the readability and maintainability of the code.

  • Removal of serialize and deserialize Methods

    • The serialize and deserialize methods have been removed from several structs and replaced with the merge and merge_states methods. This change simplifies the code and reduces redundancy.

  • Introduction of Serialize and DeserializeOwned Traits

    • The Serialize and DeserializeOwned traits have been introduced in several structs and traits. This change allows for automatic implementation of serialization and deserialization, reducing the need for manual implementation.

  • Removal of temp_values and temp_place Fields

    • The temp_values and temp_place fields have been removed from several structs. This change simplifies the code and reduces memory usage.

  • Changes in AggregateFunction Implementations

    • The serialize, merge, and merge_states functions in the AggregateFunction implementations have been updated to use the new merge function and serialization/deserialization methods. This change improves the consistency and readability of the code.

@sundy-li sundy-li marked this pull request as ready for review October 10, 2023 01:43
@github-actions github-actions bot added the pr-chore this PR only has small changes that no need to record, like coding styles. label Oct 10, 2023
@sundy-li sundy-li added ci-benchmark Benchmark: run all test and removed ci-benchmark Benchmark: run all test labels Oct 10, 2023
@sundy-li sundy-li mentioned this pull request Oct 10, 2023
Merged
@github-actions
Copy link
Contributor

Docker Image for PR

  • tag: pr-13153-f323acc

note: this image tag is only available for internal use,
please check the internal doc for more details.

RinChanNOWWW
RinChanNOWWW suggested changes Oct 10, 2023
View reviewed changes
Copy link
Contributor

@RinChanNOWWW RinChanNOWWW left a comment

Choose a reason for hiding this comment

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

The arena_holder in HashTablePayload can also be removed.

RinChanNOWWW
RinChanNOWWW reviewed Oct 10, 2023
View reviewed changes
Copy link
Contributor

@RinChanNOWWW RinChanNOWWW left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Others LGTM.

@sundy-li sundy-li added this pull request to the merge queue Oct 11, 2023
Merged via the queue into datafuselabs:main with commit f98837e Oct 11, 2023
59 checks passed
@sundy-li sundy-li deleted the agg-merge2 branch October 11, 2023 04:10
andylokandy pushed a commit to andylokandy/databend that referenced this pull request Nov 27, 2023
@sundy-li @andylokandy
chore(query): refactor agg state merge (datafuselabs#13153)
0d880f8 
* chore(query): refactor agg state merge

* chore(query): refactor agg state merge

* chore(query): refactor agg state merge

* chore(query): refactor agg state merge

* chore(query): refactor agg state merge

* chore(query): refactor agg state merge

* chore(query): fix render bug

* feat(query): add test test_box_render_block

* feat(query): remove useless holder
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhang2014 zhang2014 zhang2014 approved these changes

@RinChanNOWWW RinChanNOWWW RinChanNOWWW approved these changes

Assignees
No one assigned
Labels
ci-benchmark Benchmark: run all test pr-chore this PR only has small changes that no need to record, like coding styles.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sundy-li @databend-bot @zhang2014 @RinChanNOWWW