Skip to content

Add slice and limit methods to ReadData - #156

Merged
cmhulbert merged 22 commits into
saalfeldlab:masterfrom
bogovicj:splittable-readdata
Jun 24, 2025
Merged

Add slice and limit methods to ReadData#156
cmhulbert merged 22 commits into
saalfeldlab:masterfrom
bogovicj:splittable-readdata

Conversation

@bogovicj

@bogovicj bogovicj commented Jun 2, 2025

Copy link
Copy Markdown
Contributor

Also add split method, though less sure how or whether we will use it.

bogovicj added 5 commits June 2, 2025 16:36
* add default methods
* concrete implementation in ByteArraySplittableReadData
* add test
* add two implementations
* add KeyValueAccess size and createReadData methods
* update ReadDataTests
* because every ReadData is splittable
@bogovicj

bogovicj commented Jun 3, 2025

Copy link
Copy Markdown
Contributor Author

Adds KeyValueAccess.createReadData which enables implementations to return a backend-specific lazy- and ReadData, that can enable partial reading if the backend allows. The abstract class KeyValueAccessLazyReadData handles the main logic.

Two implemenations are provided: FileLazyReadData and HttpLazyReadData, both of which can only be made through their respective KeyValueAccess.createReadData methods.

Main questions have to do with the behavior of slice methods relating to boundary conditions. The current behavior is ensured by ReadDataTests:

  • calling slice with a negative offset immediately fails
  • calling slice with a too-large length succeeds, but will fail when attempting to materialize / read
    • failing immediately would be possible but would require a backend check for data length, which we'd like to avoid

@bogovicj
bogovicj requested review from cmhulbert and tpietzsch June 3, 2025 18:05
* document KeyValueAccessLazyReadData
@bogovicj
bogovicj marked this pull request as ready for review June 3, 2025 18:27
@bogovicj

bogovicj commented Jun 3, 2025

Copy link
Copy Markdown
Contributor Author

Note: This will need revising assuming we merge #155

@bogovicj bogovicj mentioned this pull request Jun 5, 2025
@cmhulbert

cmhulbert commented Jun 9, 2025

Copy link
Copy Markdown
Contributor

Overall looks good to me. Something I'm thinking of though

  • I still think we should try and remove split and use slice for now, unless we find a use for it, in which we can add it.
  • I'm not sure we need an explicit lazySlice that is separate from slice. I'd like to make the assumption that all ReadData are "lazy" and if I ever need it materialized, I would just call material on it. I don't think that's always going to be true, but I think that's probably how I would treat an arbitrary ReadData

@bogovicj

bogovicj commented Jun 9, 2025

Copy link
Copy Markdown
Contributor Author

I'm not sure we need an explicit lazySlice that is separate from slice

just to clarify - lazySlice is a private method in KeyValueAccessLazyReadData that is used by the public slice api method, so this doesn't really "add" a lazyslice method

@cmhulbert
cmhulbert self-requested a review June 13, 2025 13:27

@cmhulbert cmhulbert left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

clean up imports

Comment thread src/main/java/org/janelia/saalfeldlab/n5/readdata/ByteArrayReadData.java Outdated
Comment thread src/main/java/org/janelia/saalfeldlab/n5/KeyValueAccessLazyReadData.java Outdated
bogovicj added 3 commits June 13, 2025 11:35
…plittable-readdata

# Conflicts:
#	src/main/java/org/janelia/saalfeldlab/n5/FileSystemKeyValueAccess.java
#	src/main/java/org/janelia/saalfeldlab/n5/HttpKeyValueAccess.java
#	src/main/java/org/janelia/saalfeldlab/n5/readdata/AbstractInputStreamReadData.java
Comment thread src/main/java/org/janelia/saalfeldlab/n5/readdata/ReadData.java Outdated
Comment thread src/test/java/org/janelia/saalfeldlab/n5/readdata/ReadDataTests.java Outdated
Comment thread src/test/java/org/janelia/saalfeldlab/n5/readdata/ReadDataTests.java Outdated
Comment thread src/test/java/org/janelia/saalfeldlab/n5/readdata/ReadDataTests.java Outdated
Comment thread src/test/java/org/janelia/saalfeldlab/n5/readdata/ReadDataTests.java Outdated
Comment thread src/test/java/org/janelia/saalfeldlab/n5/readdata/ReadDataTests.java Outdated
Comment thread src/main/java/org/janelia/saalfeldlab/n5/KeyValueAccess.java Outdated
Comment thread src/main/java/org/janelia/saalfeldlab/n5/KeyValueAccessLazyReadData.java Outdated
Comment thread src/main/java/org/janelia/saalfeldlab/n5/KeyValueAccessLazyReadData.java Outdated
Comment thread src/main/java/org/janelia/saalfeldlab/n5/KeyValueAccessLazyReadData.java Outdated
bogovicj and others added 7 commits June 16, 2025 16:08
* rename helper method
* rm unused byte[]
* and remove KeyValueAccessReadData class
* because KVA.createReadData is preferred
* add LazyRead interface,
* add HttpLazyRead, FileLazyRead
* add KeyValueAccessReadData using LazyRead
* remove unused KeyValueAccessLazyReadData
* make KeyValueAccessReadData package-private
@cmhulbert
cmhulbert merged commit 7724f25 into saalfeldlab:master Jun 24, 2025
2 of 4 checks passed
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.

3 participants