-
Notifications
You must be signed in to change notification settings - Fork 602
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(codecv1): support zstd compression #1288
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1288 +/- ##
===========================================
- Coverage 56.11% 56.07% -0.05%
===========================================
Files 156 156
Lines 12131 12156 +25
===========================================
+ Hits 6807 6816 +9
- Misses 4798 4809 +11
- Partials 526 531 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
c170767
to
6a72f17
Compare
277b0be
to
ffe2023
Compare
1b97dac
to
0a46938
Compare
21bfc67
to
4240be9
Compare
@@ -10,6 +10,7 @@ import ( | |||
"math/big" | |||
"strings" | |||
|
|||
"github.com/colinlyguo/zstd" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should move this repo to scroll later?
we should review colinlyguo/zstd@d3f94bd and colinlyguo/zstd@fcb551e first |
we can initialize a new repo under scroll-tech (forked from https://github.com/DataDog/zstd), and I can move these commits to it, for review. |
discussed with @georgehao, we can package a zstd rust |
@@ -204,7 +205,7 @@ func (c *DAChunk) Hash() (common.Hash, error) { | |||
} | |||
|
|||
// NewDABatch creates a DABatch from the provided encoding.Batch. | |||
func NewDABatch(batch *encoding.Batch) (*DABatch, error) { | |||
func NewDABatch(batch *encoding.Batch, withCompression bool) (*DABatch, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to make sure I understand correctly: when withCompression = true
, the only things that change are: blob
, BlobVersionedHash
, right? So z
, DataHash
etc. won't change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
z
would be changed because z
is influenced by blobVersionedHash
.
@@ -10,6 +10,7 @@ import ( | |||
"math/big" | |||
"strings" | |||
|
|||
"github.com/colinlyguo/zstd" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you plan to change this before merging this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
migrated to this repo: https://github.com/scroll-tech/da-codec
|
Purpose or design rationale of this PR
Use https://github.com/DataDog/zstd as a base repo, and:
CompressScrollBatchBytes
in this commit.v1.5.6
in this commit, reusing scripts in: Update vendored zstd to 1.5.5 DataDog/zstd#125.zstd
togozstd
, because scroll-tech repo already has a repo named https://github.com/scroll-tech/zstd).PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
Deployment tag versioning
Has
tag
incommon/version.go
been updated or have you addedbump-version
label to this PR?Breaking change label
Does this PR have the
breaking-change
label?