refactor!: Extract factory interface for ChunkInputStreamGenerators #5811
Conversation
niloc132
added
NoDocumentationNeeded
NoReleaseNotesNeeded
|
I'm not sure if I would actually qualify this as a "breaking change", will leave it up to reviewers. |
.../barrage/src/test/java/io/deephaven/extensions/barrage/chunk/BarrageColumnRoundTripTest.java
Outdated
Show resolved
Hide resolved
...age/src/main/java/io/deephaven/extensions/barrage/chunk/VectorChunkInputStreamGenerator.java
Outdated
Show resolved
Hide resolved
...ge/src/main/java/io/deephaven/extensions/barrage/chunk/VarListChunkInputStreamGenerator.java
Outdated
Show resolved
Hide resolved
|
|
||
| public class VarListChunkInputStreamGenerator<T> extends BaseChunkInputStreamGenerator<ObjectChunk<T, Values>> { | ||
| private static final String DEBUG_NAME = "VarListChunkInputStreamGenerator"; | ||
|
|
||
| private final Factory factory; |
There was a problem hiding this comment.
Future follow-up:
I'd sort of like to find a pattern where this isn't retained into a field, but just used in the constructor, since in theory we only need to create the input stream generator - like creating innerGenerator etc up front, but not sure about that.
Also might want to move the factory instance to the last param rather than the first, like the ChunkReader apis.
There was a problem hiding this comment.
Maybe we can save a functor to go from innerChunk -> innerGenerator instead?
| final ChunkType chunkType, | ||
| final Class<T> type, | ||
| final Class<?> componentType, | ||
| final Chunk<Values> chunk, |
There was a problem hiding this comment.
Why don't we read the chunk type from the chunk?
Future follow-up: use the ChunkReader.TypeInfo here instead of type/componentType/chunkType (and possible future need for arrow Field)?
There was a problem hiding this comment.
TypeInfo is good if it has the arrow field, too. If Chunk has a getChunkType.. oops; feel free to use it instead.
extensions/barrage/src/main/java/io/deephaven/extensions/barrage/chunk/BooleanChunkReader.java
Outdated
Show resolved
Hide resolved
| final ChunkType chunkType, | ||
| final Class<T> type, | ||
| final Class<?> componentType, | ||
| final Chunk<Values> chunk, |
There was a problem hiding this comment.
TypeInfo is good if it has the arrow field, too. If Chunk has a getChunkType.. oops; feel free to use it instead.
|
|
||
| public class VarListChunkInputStreamGenerator<T> extends BaseChunkInputStreamGenerator<ObjectChunk<T, Values>> { | ||
| private static final String DEBUG_NAME = "VarListChunkInputStreamGenerator"; | ||
|
|
||
| private final Factory factory; |
There was a problem hiding this comment.
Maybe we can save a functor to go from innerChunk -> innerGenerator instead?
Rather than relying on a static method to create CISG instances, extracts a factory so that the JS client can provide its own implementation of how CISG instances should be built.
Also cleans up ChunkReader/ChunkInputStreamGenerator split a bit more, since for now JS can only use the former.
BREAKING CHANGES: Removes ChunkInputStreamGenerator.makeInputStreamGenerator, use the default factory instance instead.
Partial #188