v1 milestone - #5
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ab3565bec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // The DenseInfo timestamp column is a parallel array: it must cover every node or | ||
| // none. Emit it only when all nodes carry a timestamp, otherwise omit it entirely so | ||
| // the read side reports `timestamp: None` instead of inventing the epoch. | ||
| let write_timestamps = nodes.iter().all(|node| node.timestamp.is_some()); |
There was a problem hiding this comment.
When a dense block contains both timestamped nodes and nodes with timestamp: None, this all-or-nothing check omits the timestamp column for the entire block. Since PbfWriter batches arbitrary nodes into 8000-element blocks, a single timestamp-less node makes every timestamped node in that block round-trip as None; fall back to sparse encoding for mixed timestamp presence or otherwise preserve the timestamped nodes.
Useful? React with 👍 / 👎.
| }, | ||
| // The visible flag defaults to true when absent (proto2 get_visible() returns | ||
| // false for unset optional fields, which would wrongly mark elements deleted). | ||
| visible: info.has_visible() && info.get_visible(), |
There was a problem hiding this comment.
For sparse Node/Way/Relation info records where the optional visible field is absent, this expression returns false, even though absent visibility is the normal current-data case and the adjacent comment says it must be treated as visible. That makes external sparse elements appear deleted and causes callers that inspect visible to get wrong data; this should default to true when has_visible() is false.
Useful? React with 👍 / 👎.
| if self.has_invisible_elements { | ||
| header_block | ||
| .required_features | ||
| .push("HistoricalInformation".to_string()); |
There was a problem hiding this comment.
If the first automatic flush happens before any invisible element is written, for example the first 8000 elements are visible and element 8001 has visible = false, write_header runs with this flag still false and the header can never be amended. The later block still serializes visible=false, producing a PBF missing the required HistoricalInformation feature for downstream readers; declare it up front or defer header emission until all elements have been seen.
Useful? React with 👍 / 👎.
1 participant
No description provided.