Skip to content

add range coalescing option#18

Merged
Shane98c merged 6 commits intomainfrom
Shane98c/range-coalescing
Apr 27, 2026
Merged

add range coalescing option#18
Shane98c merged 6 commits intomainfrom
Shane98c/range-coalescing

Conversation

@Shane98c
Copy link
Copy Markdown
Collaborator

@Shane98c Shane98c commented Apr 26, 2026

Adds opt-in range coalescing for icechunk chunk payload reads.

  • New src/reader/range-coalescer.ts adapters expose icechunk's two read paths as zarrita AsyncReadable:
    • makeStorageStore — native chunks via Storage
    • makeUrlStore — virtual chunks via direct HTTP, with optional conditional headers for validateChecksums
  • ReadSession keeps a bounded LRU of coalescer-wrapped stores, partitioned by URL, fetch client, and checksum identity so conditional headers and per-client state never bleed across a coalesced batch.
  • Zarrita remains a true optional peer dep — the adapters mirror its AsyncReadable shape locally and only run user-supplied withRangeCoalescing.
  • Handles servers that ignore Range (HTTP 200 fallback) by slicing the requested window out of the full body, plus suffix-length range support.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces range coalescing for chunk payload reads by integrating with zarrita's withRangeCoalescing utility. This allows concurrent reads from the same backing object to be merged into fewer, larger requests, improving efficiency for both native and virtual chunks. The implementation includes a new range-coalescer.ts module, an LRU cache for coalesced stores in ReadSession, and updates to the IcechunkStore API to expose the rangeCoalescing option. Feedback is provided regarding the restoration of range details in error messages for native chunk fetch failures to maintain observability.

Comment thread src/reader/session.ts
Comment on lines 561 to 563
throw new Error(
`Storage returned ${data.length} bytes for ${path} range ${range.start}-${range.end - 1}; expected ${payload.length} bytes`,
`Storage returned ${data.length} bytes for ${path}; expected ${payload.length} bytes`,
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The error message for native chunk fetch failures has been simplified, removing the specific range information. While cleaner, this might make debugging harder if a storage backend returns an unexpected length. Consider keeping the range details in the error message for better observability.

Suggested change
throw new Error(
`Storage returned ${data.length} bytes for ${path} range ${range.start}-${range.end - 1}; expected ${payload.length} bytes`,
`Storage returned ${data.length} bytes for ${path}; expected ${payload.length} bytes`,
);
throw new Error(
`Storage returned ${data.length} bytes for ${path} range ${payload.offset}-${payload.offset + payload.length - 1}; expected ${payload.length} bytes`,
);

@Shane98c Shane98c merged commit b166ddf into main Apr 27, 2026
12 checks passed
@Shane98c Shane98c deleted the Shane98c/range-coalescing branch April 27, 2026 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant