-
Notifications
You must be signed in to change notification settings - Fork 411
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce Chunk
component-level helpers and UnitChunk
#6990
Conversation
Chunk
component-level helpers and UnitChunk
Interesting -- you're thinking of materializing a LatestAt chunk on-demand for the query results? |
&self, | ||
component_name: &ComponentName, | ||
row_index: usize, | ||
) -> Option<ChunkResult<Box<dyn ArrowArray>>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, and in all the signatures below, I think we should try to keep result types on the outside as in: ChunkResult<Option<Box<dyn ArrowArray>>>
This seems like it lets us handle serialization errors consistently and then end up with Option<Box<dyn ArrowArray>>
inline with:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had ChunkResult<Option<Box<dyn ArrowArray>>>
at the beginning, and it was just as awkward but for different reasons.
In the end I ended up with that variant, but ultimately I don't really have a strong opinion for one or the other.
Two notes, though:
- I purposefully ignored / stayed away from Proposal: avoid empty arrays #6602, as I quickly realized that this PR stream would be complicated enough without attempting to modify our existing semantics. I don't mind revisiting that in the future (and switching the
Option<Result>
), but for now I don't think this should have any bearing on the current PR. - I wanted these Chunk APIs to be low-level enough so that more high-level semantics could be built on top of them for the common cases. These high-level APIs are introduced later in this PR stream, at the
re_query
layer.
component_name: &ComponentName, | ||
row_index: usize, | ||
instance_index: usize, | ||
) -> Option<ChunkResult<Box<dyn ArrowArray>>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If an missing component array is semantically consider "empty", then a row_index within an empty array should be an error. I'm not sure we need an Option here at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like I commented above: I wanted these APIs to be a lower-level building block. It can be valuable to the caller to know whether the component as a whole is missing with a single API.
Higher-level APIs with the kind of semantics you're describing are introduced later in this PR stream at the re_query
layer, and we can decide to treat the missing component as an error there.
80bcf4d
to
2510837
Compare
(I assume you got your answer while reviewing the next PR!) |
This introduces all the usual crazy helpers for when you want to retrieve some very particular piece of data out of a chunk, in one (hopefully) neat, consistent package.
In particular this adds
UnitChunk
, a wrapper type forChunk
with is guaranteed to only ever hold one row of data, which is going to be very useful when introducing the newChunk
-based latest-at API later on.Chunk
s #6989Checklist
main
build: rerun.io/viewernightly
build: rerun.io/viewerCHANGELOG.md
and the migration guideTo run all checks from
main
, comment on the PR with@rerun-bot full-check
.