sync flatbuffers schema with upstream icechunk#1
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for a v2 format in the FlatBuffers schema, including new generated classes and updated parsers for snapshots and array node data. It adds "extra" fields for extensibility and implements backward compatibility by falling back to v1 structures when v2 fields are absent. Feedback was provided regarding code duplication in the manifest file parsing logic within the snapshot parser.
| const manifestFiles: ManifestFileInfo[] = []; | ||
| for (let i = 0; i < manifestFilesLength; i++) { | ||
| const fbsMfi = fbsSnapshot.manifestFiles(i); | ||
| if (fbsMfi) { | ||
| const mfiIdObj = fbsMfi.id(); | ||
| if (!mfiIdObj) throw new Error("ManifestFileInfo missing id"); | ||
| manifestFiles.push({ | ||
| id: asObjectId12(readId12(mfiIdObj.bb!, mfiIdObj.bb_pos)), | ||
| sizeBytes: Number(fbsMfi.sizeBytes()), | ||
| numChunkRefs: fbsMfi.numChunkRefs(), | ||
| }); | ||
| const v2Length = fbsSnapshot.manifestFilesV2Length(); | ||
| if (v2Length > 0) { | ||
| for (let i = 0; i < v2Length; i++) { | ||
| const fbsMfi = fbsSnapshot.manifestFilesV2(i); | ||
| if (fbsMfi) { | ||
| const mfiIdObj = fbsMfi.id(); | ||
| if (!mfiIdObj) throw new Error("ManifestFileInfoV2 missing id"); | ||
| manifestFiles.push({ | ||
| id: asObjectId12(readId12(mfiIdObj.bb!, mfiIdObj.bb_pos)), | ||
| sizeBytes: Number(fbsMfi.sizeBytes()), | ||
| numChunkRefs: fbsMfi.numChunkRefs(), | ||
| }); | ||
| } | ||
| } | ||
| } else { | ||
| const v1Length = fbsSnapshot.manifestFilesLength(); | ||
| for (let i = 0; i < v1Length; i++) { | ||
| const fbsMfi = fbsSnapshot.manifestFiles(i); | ||
| if (fbsMfi) { | ||
| const mfiIdObj = fbsMfi.id(); | ||
| if (!mfiIdObj) throw new Error("ManifestFileInfo missing id"); | ||
| manifestFiles.push({ | ||
| id: asObjectId12(readId12(mfiIdObj.bb!, mfiIdObj.bb_pos)), | ||
| sizeBytes: Number(fbsMfi.sizeBytes()), | ||
| numChunkRefs: fbsMfi.numChunkRefs(), | ||
| }); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
There's significant code duplication in the logic for parsing v1 and v2 manifest files. This can be refactored to be more concise and improve maintainability by determining which version to use upfront and then using a single loop.
const manifestFiles: ManifestFileInfo[] = [];
const v2Length = fbsSnapshot.manifestFilesV2Length();
const useV2 = v2Length > 0;
const length = useV2 ? v2Length : fbsSnapshot.manifestFilesLength();
for (let i = 0; i < length; i++) {
const fbsMfi = useV2
? fbsSnapshot.manifestFilesV2(i)
: fbsSnapshot.manifestFiles(i);
if (fbsMfi) {
const mfiIdObj = fbsMfi.id();
if (!mfiIdObj) {
throw new Error(`ManifestFileInfo${useV2 ? "V2" : ""} missing id`);
}
manifestFiles.push({
id: asObjectId12(readId12(mfiIdObj.bb!, mfiIdObj.bb_pos)),
sizeBytes: Number(fbsMfi.sizeBytes()),
numChunkRefs: fbsMfi.numChunkRefs(),
});
}
}There was a problem hiding this comment.
easier to keep these separate in case things diverge further in the future.
1 participant
Syncs FlatBuffer schemas and generated code with earth-mover/icechunk@0488686 which reordered manifest_files_v2/extra fields in the Snapshot table.