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: implement Puffin format #2721

Closed
wants to merge 2 commits into from

Conversation

zhongzc
Copy link
Contributor

@zhongzc zhongzc commented Nov 9, 2023

I hereby agree to the terms of the GreptimeDB CLA

What's changed and what's your intention?

Part of #2705

implement {sync|async} {reader|writer} for Puffin.

Checklist

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.

Refer to a related PR or issue link (optional)

Signed-off-by: Zhenchi <[email protected]>
Copy link

codecov bot commented Nov 9, 2023

Codecov Report

Merging #2721 (1c3e469) into develop (0cd6dac) will decrease coverage by 0.29%.
Report is 1 commits behind head on develop.
The diff coverage is 96.55%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2721      +/-   ##
===========================================
- Coverage    85.17%   84.88%   -0.29%     
===========================================
  Files          765      775      +10     
  Lines       123940   125020    +1080     
===========================================
+ Hits        105565   106125     +560     
- Misses       18375    18895     +520     

Copy link
Member

Choose a reason for hiding this comment

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

Why this is binary

Copy link
Contributor Author

@zhongzc zhongzc Nov 9, 2023

Choose a reason for hiding this comment

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

only github knows

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.dev works fine

Copy link
Contributor

Choose a reason for hiding this comment

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

Why does the mod.rs occupy 8.25KB?

By the way, can we remove mod.rs? Most crates in the project don't use mod.rs:
https://doc.rust-lang.org/edition-guide/rust-2018/path-changes.html#no-more-modrs

Copy link
Contributor

@evenyag evenyag left a comment

Choose a reason for hiding this comment

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

Should we create a new repo for the puffin crate? We can create a repo under our team. @waynexia @v0y4g3r

This PR is too large and I'd suggest breaking it into 3 or 4 small PRs that each is less than 1k line

  • init crate and file formats
  • writer
  • reader
  • partial reader

Copy link
Contributor

Choose a reason for hiding this comment

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

Why does the mod.rs occupy 8.25KB?

By the way, can we remove mod.rs? Most crates in the project don't use mod.rs:
https://doc.rust-lang.org/edition-guide/rust-2018/path-changes.html#no-more-modrs

@zhongzc
Copy link
Contributor Author

zhongzc commented Nov 10, 2023

Why does the mod.rs occupy 8.25KB?

It's average. 40B/line * 200lines = 8KB

can we remove mod.rs?

Sure.

Should we create a new repo for the puffin crate

We can move out when the implementation becomes more mature and stable. Multiple repos will affect development efficiency.

breaking it into 3 or 4 small PRs

good suggestion

@zhongzc zhongzc marked this pull request as draft November 10, 2023 04:15
@v0y4g3r
Copy link
Contributor

v0y4g3r commented Nov 10, 2023

Maybe we should create a puffin-rs crate?

@zhongzc
Copy link
Contributor Author

zhongzc commented Nov 10, 2023

Maybe we should create a puffin-rs crate?

Do you mean creating a new repo?

@zhongzc zhongzc closed this Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants