Skip to content
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

feat(datafusion-7181): enable slicing of rows #4817

Closed

Conversation

wiedld
Copy link
Contributor

@wiedld wiedld commented Sep 14, 2023

Which issue does this PR close?

This is a change required in order to slice the row cursor in the cascaded sort order.

precursor for apache/datafusion#7181

Rationale for this change

During the cascaded merge, we may yield partial record batch (a.k.a. partial cursors) from a given merge node (in the cascade tree). As such, the cursor would need to be sliced as described here.

What changes are included in this PR?

Slicing the cursor.

Are there any user-facing changes?

No breaking changes.
Only a new API .slice() method, including documentation.

@github-actions github-actions bot added the arrow Changes to the arrow crate label Sep 14, 2023
@wiedld wiedld marked this pull request as ready for review September 14, 2023 18:00
Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

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

Just some minor nits, the fact this copies data does make me wonder if the motivating use-case might be better served doing something using Arc<Rows> and storing the slice manually though...

arrow-row/src/lib.rs Outdated Show resolved Hide resolved
offsets = offsets.iter_mut().map(|x| *x - start).collect();

if length > 0 {
assert_eq!(offsets.last().unwrap(), &buffer.len());
Copy link
Contributor

Choose a reason for hiding this comment

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

These are just sanity checks correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, exactly.

arrow-row/src/lib.rs Outdated Show resolved Hide resolved
@tustvold
Copy link
Contributor

I believe the conclusion upstream is that we don't need this, so closing. Feel free to reopen if I have misunderstood

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants