Fix defragment_symbol_data dropping user metadata (#9889436827)#2991
Open
himsheda wants to merge 3 commits intoman-group:masterfrom
Open
Fix defragment_symbol_data dropping user metadata (#9889436827)#2991himsheda wants to merge 3 commits intoman-group:masterfrom
himsheda wants to merge 3 commits intoman-group:masterfrom
Conversation
himsheda
requested review from
IvoDD,
alexowens90 and
poodlewars
as code owners
himsheda
force-pushed
the
fix/defragment-symbol-data-drops-metadata
branch
from
765da9e to
b313aeb
Compare
phoebusm
approved these changes
Mar 30, 2026
| timestamp=result.timestamp, | ||
| ) | ||
| version_query = self._get_version_query(result.version) | ||
| _, udm = self.version_store.read_metadata(symbol, version_query) |
Collaborator
There was a problem hiding this comment.
The metadata should already available in the C++ layer at this point. Consider returning it directly to avoid extra API call and storage read.
phoebusm
reviewed
Mar 30, 2026
phoebusm
requested changes
Mar 30, 2026
Collaborator
phoebusm
left a comment
phoebusm
left a comment
There was a problem hiding this comment.
#2991 (comment)
Returning the metadata directly from C++ maybe too much scope for this PR. It would be sufficient to just update the docstring in Library to document that the returned metadata will be None
himsheda
force-pushed
the
fix/defragment-symbol-data-drops-metadata
branch
from
f6539a8 to
8ca9081
Compare
Author
Handled. @phoebusm Please have a look |
defragment_symbol_data_impl passed std::nullopt to collate_and_write for user_meta, silently discarding the metadata that had been correctly read into pipeline_context->user_meta_ from the previous version. Fix by passing the actual user_meta_ptr from the pipeline context. Also fix the Python wrapper which hardcoded metadata=None in the returned VersionedItem regardless of what was stored; it now reads the metadata back from the newly-written version using the existing read_metadata pattern. Add regression test test_defragment_preserves_metadata.
himsheda
force-pushed
the
fix/defragment-symbol-data-drops-metadata
branch
from
8ca9081 to
2ff5427
Compare
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
2 participants
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.
defragment_symbol_data_impl passed std::nullopt to collate_and_write for user_meta, silently discarding the metadata that had been correctly read into pipeline_context->user_meta_ from the previous version. Fix by passing the actual user_meta_ptr from the pipeline context.
Also fix the Python wrapper which hardcoded metadata=None in the returned VersionedItem regardless of what was stored; it now reads the metadata back from the newly-written version using the existing read_metadata pattern.
Add regression test test_defragment_preserves_metadata.
Reference Issues/PRs
#2611
What does this implement or fix?
Any other comments?
Checklist
Checklist for code changes...