Skip to content

Conversation

aditanase
Copy link
Contributor

Description

Fix serde error

@github-actions github-actions bot added the binding/rust Issues for the Rust crate label Jun 20, 2025
Copy link

codecov bot commented Jun 20, 2025

Codecov Report

Attention: Patch coverage is 0% with 10 lines in your changes missing coverage. Please review.

Project coverage is 74.22%. Comparing base (2cc8081) to head (1883b98).
Report is 15 commits behind head on main.

Files with missing lines Patch % Lines
crates/core/src/kernel/snapshot/serde.rs 0.00% 9 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3548      +/-   ##
==========================================
- Coverage   74.23%   74.22%   -0.02%     
==========================================
  Files         150      150              
  Lines       44739    44742       +3     
  Branches    44739    44742       +3     
==========================================
- Hits        33213    33210       -3     
- Misses       9384     9386       +2     
- Partials     2142     2146       +4     

☔ 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.

@rtyler
Copy link
Member

rtyler commented Jun 29, 2025

@aditanase what error was actually cropping up that led you to this change? I would like to add a regression test to make sure I understand the problem, and I'm happy to write it, but some steps would be helpful

@rtyler rtyler marked this pull request as draft July 11, 2025 01:05
@rtyler rtyler added the mre-needed Whether an MRE needs to be provided label Jul 11, 2025
@adragomir adragomir deleted the fix-serde branch July 17, 2025 08:49
@aditanase
Copy link
Contributor Author

aditanase commented Aug 5, 2025

@rtyler @ion-elgreco sorry for the late reply, I was out on vacation for most of July.
It's a very obvious error when trying to deserialize a table that also includes checkpoint files:

Message: called Option::unwrap() on a None value
Location: .../delta-rs/crates/core/src/kernel/snapshot/serde.rs:103

I went in trying to deal with the Option and realized that the code was just updated for the commits branch above, but not for checkpoints, so I just assumed that people don't test with more complex tables, and proceeded to make it consistent. On a second look, serde probably never worked well since it was serializing nanos and deserializing millis.

@aditanase aditanase marked this pull request as ready for review August 5, 2025 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
binding/rust Issues for the Rust crate mre-needed Whether an MRE needs to be provided
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants