Conversation
…ny branch. should have been gated by human readibility
xav-db
added a commit
that referenced
this pull request
Mar 3, 2026
<!-- greptile_comment -->
<h3>Greptile Summary</h3>
Fixes `Date` deserialization for binary (bincode) formats by adding an
`is_human_readable()` check in the `Deserialize` impl. Previously,
`Date` unconditionally used `deserialize_any(DateVisitor)` which was
ambiguous for binary formats — bincode didn't know the serialized data
was a string, causing deserialization failures. The fix routes binary
formats through explicit `String::deserialize` +
`Date::parse_from_string`, while keeping the visitor-based approach for
JSON.
- **Core fix** in `helix-db/src/protocol/date.rs`: Binary deserializers
now explicitly decode a `String` and parse it, matching the
`serialize_str()` used by the `Serialize` impl
- **Test coverage** is thorough: new unit test for `Date` bincode
roundtrip, integration tests for Node/Edge/Vector with Date properties,
and end-to-end traversal tests for nodes and vectors with Date
properties
- **Un-ignores** the previously-failing
`test_date_bincode_serialization` migration test
- **`.gitignore`**: adds `**/dist` and `**/node_modules` patterns
(unrelated housekeeping)
<details><summary><h3>Important Files Changed</h3></summary>
| Filename | Overview |
|----------|----------|
| helix-db/src/protocol/date.rs | Core fix: adds `is_human_readable()`
check to Date Deserialize impl, routing binary formats (bincode) through
explicit `String::deserialize` + `parse_from_string` instead of
`deserialize_any(DateVisitor)`. Adds `test_bincode_roundtrip` unit test.
Fix is correct and necessary. |
| helix-db/src/protocol/value.rs | Adds `Value::Date` to the bincode
roundtrip test coverage. No logic changes, only test enhancement. |
| helix-db/src/protocol/custom_serde/integration_tests.rs | Adds three
new integration tests for Date property round-trips: node, edge, and
vector bincode serialization with Date properties. Well-structured tests
following existing patterns. |
|
helix-db/src/helix_engine/tests/traversal_tests/node_traversal_tests.rs
| Adds `test_n_from_id_with_date_property` end-to-end test: creates a
node with a Date property, writes/reads from storage, and verifies the
property round-trips correctly. |
|
helix-db/src/helix_engine/tests/traversal_tests/vector_traversal_tests.rs
| Adds two new tests: `test_search_v_with_non_date_properties`
(baseline) and `test_search_v_with_date_property` (validates Date
properties survive vector storage round-trips). |
| helix-db/src/helix_engine/storage_core/storage_migration_tests.rs |
Removes `#[ignore]` from `test_date_bincode_serialization` (now that the
fix enables it to pass), updates print format message, and reorders
imports alphabetically. |
| .gitignore | Adds `**/dist` and `**/node_modules` ignore patterns.
Missing trailing newline at end of file. |
</details>
</details>
<details><summary><h3>Flowchart</h3></summary>
```mermaid
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Value::Date(d) Serialize"] --> B{is_human_readable?}
B -->|Yes - JSON| C["serialize_str(rfc3339)"]
B -->|No - bincode| D["serialize_newtype_variant(12, 'Date', d)"]
D --> E["Date::serialize → serialize_str(rfc3339)"]
F["Deserialize Value"] --> G{is_human_readable?}
G -->|Yes - JSON| H["deserialize_any(ValueVisitor)"]
G -->|No - bincode| I["deserialize_enum → variant 12"]
I --> J["variant_data.newtype_variant()"]
J --> K["Date::deserialize"]
K --> L{is_human_readable?}
L -->|Yes| M["deserialize_any(DateVisitor)"]
L -->|No - NEW FIX| N["String::deserialize"]
N --> O["Date::parse_from_string"]
O --> P["Value::Date(date) ✅"]
H --> Q["visit_str / visit_i64 etc."]
M --> R["DateVisitor::visit_str"]
```
</details>
<sub>Last reviewed commit: 2c5d580</sub>
<!-- greptile_other_comments_section -->
<!-- /greptile_comment -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #872
Greptile Summary
Fixed date deserialization bug where bincode (non-human-readable format) was incorrectly using the generic visitor path instead of properly deserializing RFC3339 strings.
Key Changes:
Date::deserialize()to checkis_human_readable()and route bincode through string deserialization pathtest_date_bincode_serializationnow that the bug is resolvedThe fix ensures that when bincode serializes dates as RFC3339 strings (via the
Serializeimpl), theDeserializeimpl properly handles this by parsing the string back to aDateTime<Utc>for non-human-readable deserializers.Important Files Changed
is_human_readable()to route non-human-readable formats (like bincode) to string deserialization path, added test coverageValue::Datecase to bincode roundtrip test for completeness#[ignore]from date bincode test now that the issue is fixed, updated comment to reflect RFC3339 serialization formatLast reviewed commit: f40cc7b