CLI/merge: Avoid duplication of identical schemas #982
Conversation
| // Reset struct members | ||
| m.schemaIdByHash = make(map[string]uint16) | ||
| m.schemaIDs = make(map[schemaID]uint16) | ||
| m.channelIDs = make(map[channelID]uint16) | ||
| m.nextChannelID = 1 | ||
| m.nextSchemaID = 1 |
There was a problem hiding this comment.
Here I have to reset the schemaIdByHash as we might have a new writer where the schemas haven't been written to yet.
In general, I don't fully understand the purpose of the mcapMerger struct. Wouldn't a single function be sufficient? I don't think users are supposed to call mcapMerger.addSchema themselves? But maybe I don't fully understand Go's concept of private members yet.
There was a problem hiding this comment.
I think this could definitely be written as a single function. There are no users of mcapMerger since it's private. I don't remember the details about mcapMerger but I'm pretty sure it only exists to assist with whatever remapping of channel/schema IDs are required, and you could write that either way.
There was a problem hiding this comment.
are you doing this resetting because you anticipate mergeInputs will be called multiple times?
There was a problem hiding this comment.
^^ this seems reasonable to me (though I don't think it actually will be called that way), as would writing a single function -- maybe that's why you were asking.
Patch looks good to me generally, thanks!
There was a problem hiding this comment.
are you doing this resetting because you anticipate mergeInputs will be called multiple times?
It's called multiple times in the tests. The resetting was required to make it succeed.
### Public-Facing Changes CLI/merge: Avoid duplication of identical schemas ### Description - Removes some unused members of the McapMerger struct - Avoid duplication of identical schemas when merging mcap files. Schemas are considered identical if their md5 hash sum over the schema name, encoding and data is equal.
Public-Facing Changes
CLI/merge: Avoid duplication of identical schemas
Description