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: using borsh instead of bincode for serde agg function state #13997

Merged
merged 5 commits into from Dec 16, 2023

Conversation

ariesdevil
Copy link
Member

@ariesdevil ariesdevil commented Dec 12, 2023

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

Summary

using borsh instead of bincode for serde agg function state

BTW: there are three crates that using my own patch version, after these merge to upstream, I'll update them to databend.

Tests

  • Unit Test
  • Logic Test
  • Benchmark Test
  • No Test - Explain why

This change is Reviewable

@github-actions github-actions bot added the pr-refactor this PR changes the code base without new features or bugfix label Dec 12, 2023
Cargo.toml Show resolved Hide resolved
@sundy-li sundy-li marked this pull request as ready for review December 13, 2023 06:41
@BohuTANG
Copy link
Member

What's the performance compare to bincode?

@ariesdevil
Copy link
Member Author

What's the performance compare to bincode?↳

I'm doing now, will be post to this PR later

@ariesdevil
Copy link
Member Author

ariesdevil commented Dec 13, 2023

│❯ cargo benchcmp bincode borsh                                                                                                                                                                                                                                                                                              │
│ name                            bincode ns/iter  borsh ns/iter  diff ns/iter   diff %  speedup                                                                                                                                                                                                                             │
│ tests::bench_borsh_bincode_ser  15               12                       -3  -20.00%   x 1.25


│ name                              bincode_de ns/iter  borsh_de ns/iter  diff ns/iter   diff %  speedup                                                                                                                                                                                                                     │
│ tests::bench_borsh_bincode_deser  32                  19                         -13  -40.62%   x 1.68

It seems that we don't need to worry about performance.

@ariesdevil
Copy link
Member Author

I'll go on modify other state ser/deser

@ariesdevil ariesdevil marked this pull request as draft December 13, 2023 14:39
@ariesdevil ariesdevil marked this pull request as ready for review December 13, 2023 14:49
@BohuTANG BohuTANG added the ci-benchmark Benchmark: run all test label Dec 13, 2023
Copy link
Contributor

Docker Image for PR

  • tag: pr-13997-15a8fb7

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

Copy link
Contributor

@ariesdevil ariesdevil added this pull request to the merge queue Dec 16, 2023
@BohuTANG BohuTANG removed this pull request from the merge queue due to a manual request Dec 16, 2023
@BohuTANG BohuTANG merged commit caad7d2 into datafuselabs:main Dec 16, 2023
68 checks passed
Cargo.toml Show resolved Hide resolved
@ariesdevil ariesdevil deleted the agg-function branch December 16, 2023 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-benchmark Benchmark: run all test pr-refactor this PR changes the code base without new features or bugfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: consider use borsh to serde aggregation state
3 participants