Use Range<u64> as argument for the RangeQueryHandle::get()
#7395
Conversation
abey79
added
⛃ re_datastore
| @@ -219,7 +220,9 @@ impl RangeQueryHandle<'_> { | |||
| /// This is the most performant way to iterate over the dataset. | |||
| // | |||
| // TODO(cmc): This could be turned into an actual lazy iterator at some point. | |||
| pub fn get(&self, offset: u64, mut len: u64) -> Vec<RecordBatch> { | |||
| pub fn get(&self, row_range: Range<u64>) -> Vec<RecordBatch> { | |||
There was a problem hiding this comment.
This could use a
| pub fn get(&self, row_range: Range<u64>) -> Vec<RecordBatch> { | |
| pub fn get(&self, row_range: Range<u64>) -> Vec<RecordBatch> { | |
| debug_assert!(row_range.start <= row_range.end, "Negative range: {row_range}"); |
There was a problem hiding this comment.
I'm not 100% sure about this one. This is user input (via the SDK), so I think it should be validated a bit less... drastically? Here I opted for (silently) returning an empty dataframe if the range length is <= 0. We probably will have more explicit error handling in the future, but that seemed overkill for now.
There was a problem hiding this comment.
We probably will have more explicit error handling in the future
Yeah, an error-result would have been my expectation here.
There was a problem hiding this comment.
TBH, this very API change is what spawned this error condition in the first place. (offset: u64, len: u64) encodes better than Range<u64> the fact that "inverted ranges" are invalid.
With the fact that Range is not Copy and is occasionally annoying, this makes me question this entire PR. Thoughts?
There was a problem hiding this comment.
Ahh, interesting point.
I actually do prefer the offset/len structure as it is closer to offset / limit that shows up in SQL or other datafusion integrations.
|
Closing as not really desirable. |
What
Minor change of API suggested by @emilk: use
Range<u64>for theRangeQueryHandle::get(row_range)API.Checklist
mainbuild: rerun.io/viewernightlybuild: rerun.io/viewerCHANGELOG.mdand the migration guideTo run all checks from
main, comment on the PR with@rerun-bot full-check.