-
Notifications
You must be signed in to change notification settings - Fork 234
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
Support limit offset as ExecNode #450
Conversation
rust/src/io/exec/limit.rs
Outdated
let nrows = b.num_rows() as i64; | ||
if off > 0 { | ||
if off > nrows { | ||
// skip this batch if offset is more than num rows |
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.
let's add a todo to skip the read from child entirely if off > nrows
?
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.
this is only useful without any predicates so probably won't be very impactful
fn poll_next(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Option<Self::Item>> { | ||
Pin::into_inner(self).rx.poll_recv(cx) | ||
} | ||
} |
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.
Add some tests?
rust/src/io/exec/limit.rs
Outdated
/// Create a new execution node to handle limit offset. | ||
pub fn new( | ||
child: impl ExecNode + Unpin + Send + 'static, | ||
limit: Option<i64>, |
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.
should limit
not a Option value
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.
offline discussion: postgres supports offset without limit
89c4e9b
to
48ce7c1
Compare
rust/src/dataset/scanner.rs
Outdated
@@ -147,6 +147,17 @@ impl<'a> Scanner { | |||
Arc::new(projection.clone()), | |||
flat_knn_node, | |||
)) | |||
} else if self.limit.is_some() || self.offset.is_some() { |
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.
Could this work with other scanner, i.e., KNN?
for example, should it be the front of the ExecNode
so far.
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.
🚢
No description provided.