feat: log compaction

[ion-elgreco]

Description

The description of the main changes of your pull request

Related Issue(s)

Documentation

[codecov]

Codecov Report

❌ Patch coverage is 5.95238% with 79 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.61%. Comparing base (e7b7885) to head (2e3aac9).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
python/src/lib.rs	0.00%	39 Missing ⚠️
crates/core/src/protocol/log_compaction.rs	0.00%	38 Missing ⚠️
crates/core/src/protocol/mod.rs	71.42%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4210      +/-   ##
==========================================
- Coverage   76.75%   76.61%   -0.14%     
==========================================
  Files         166      167       +1     
  Lines       48277    48332      +55     
  Branches    48277    48332      +55     
==========================================
- Hits        37053    37030      -23     
- Misses       9365     9444      +79     
+ Partials     1859     1858       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[ethan-tyler]

I'm gonna review today

[ethan-tyler]

Love this - added a couple of comments for your consideration.

[ethan-tyler]

Do we need docs for this new pub api?

[ion-elgreco]

We can add that later stage ;) for now its a hidden gem

[rtyler]

Quite straight-forward and simple. I do think @ethan-tyler has made some points worth addressing.

I don't really understand the point of these log compaction files compared to checkpoints, but that's not an @ion-elgreco question to resolve 😆 Seems like more over complication of the protocol. 🤷 🙈

If these do provide reader benefits, what if we fire-and-forget a task that tries to generate on of these files in the background after a write on version % 25 or something? If we succeed in writing a log compaction file, great, if we don't, who cares 😆

[ion-elgreco]

Quite straight-forward and simple. I do think @ethan-tyler has made some points worth addressing.

I don't really understand the point of these log compaction files compared to checkpoints, but that's not an @ion-elgreco question to resolve 😆 Seems like more over complication of the protocol. 🤷 🙈

If these do provide reader benefits, what if we fire-and-forget a task that tries to generate on of these files in the background after a write on version % 25 or something? If we succeed in writing a log compaction file, great, if we don't, who cares 😆

I assume kernel log replay makes use of these if it encounters them. I can do this in a commit hook in a follow up pr

[ion-elgreco]

Exact matching of our json logs vs this compaction log is not possible because in the compaction log we drop null columns, suggested by kernel, but our own action to bytes serializer does not. Maybe a small optimization we can do in the future is to also drop null columns

NOTE: Null columns should not be written to the JSON file. For example, if a row has columns [“a”, “b”] and the value of “b” is null, the JSON object should be written as { “a”: “…” }. Note that including nulls is technically valid JSON, but would bloat the log, therefore we recommend omitting them.

[ethan-tyler]

LGTM - Thanks for taking my feedback :)

[hntd187]

Exact matching of our json logs vs this compaction log is not possible because in the compaction log we drop null columns, suggested by kernel, but our own action to bytes serializer does not. Maybe a small optimization we can do in the future is to also drop null columns

NOTE: Null columns should not be written to the JSON file. For example, if a row has columns [“a”, “b”] and the value of “b” is null, the JSON object should be written as { “a”: “…” }. Note that including nulls is technically valid JSON, but would bloat the log, therefore we recommend omitting them.

This will eventually manifest as a bug of some sort since we're doing different things from kernel.

[ethan-tyler]

This will eventually manifest as a bug of some sort since we're doing different things from kernel.

I see your concern but I don't think this is a correctness bug. Both encodings are semantically equivalent for Delta replay. You should be able to validate this with replay/state equivalence and not byte matching. Worth a follow up to unify serializers across paths, but IMO not a blocker.

[hntd187]

This will eventually manifest as a bug of some sort since we're doing different things from kernel.

I see your concern but I don't think this is a correctness bug. Both encodings are semantically equivalent for Delta replay. You should be able to validate this with replay/state equivalence and not byte matching. Worth a follow up to unify serializers across paths, but IMO not a blocker.

Yeah, it's probably fine, it's more we don't know what we don't know. I'm just saying there is prolly some corner case that'll come up as a bug eventually on this. This still LGTM