Implementing ByteStream interuption on infinite stream by simon-curtis · Pull Request #13552 · nushell/nushell

simon-curtis · 2024-08-06T22:01:22Z

Description

This PR should address #13530 by explicitly handling ByteStreams.

The issue can be replicated easily on linux by running:

open /dev/urandom | into binary | bytes at ..10

Would leave the output hanging and with no way to cancel it, this was likely because it was trying to collect the input stream and would not complete.

I have also put in an error to say that using negative offsets for a bytestream without a length cannot be used.

~/git/nushell> open /dev/urandom | into binary | bytes at (-1)..
Error: nu::shell::incorrect_value

  × Incorrect value.
   ╭─[entry #3:1:35]
 1 │ open /dev/urandom | into binary | bytes at (-1)..
   ·                                   ────┬─── ───┬──
   ·                                       │       ╰── encountered here
   ·                                       ╰── Negative range values cannot be used with streams that don't specify a length
   ╰────

User-Facing Changes

No operation changes, only the warning you get back for negative offsets

Tests + Formatting

Ran toolkit check pr with no errors or warnings

Manual testing of the example commands above

WindSoilder

Thank you! The idea of using ByteStream::from_fn and reader.bytes looks good to me!

Just some comments for a corner case and some test cases

crates/nu-command/src/bytes/at.rs

crates/nu-cmd-lang/src/example_support.rs

crates/nu-command/src/bytes/at.rs

WindSoilder

Thank you for the update! I left some comments about the position of tests. And CI is failed, you need to run cargo fmt to fix it :)

simon-curtis · 2024-10-23T20:13:14Z

All requested changes have been completed 😄

IanManske

Thanks for this PR! The take and skip commands already support binary streams, and their code could be reused for this command. I.e., bytes at ..$n is essentially take $n, bytes at $n.. is skip $n, and bytes at $n..$m is skip $n | take ($m - $n). ~~I think it would make more sense if the binary stream handling was extracted out from those commands into a separate file and then reused across all these commands (skip, take, and bytes at).~~

crates/nu-cmd-lang/src/example_support.rs

tests/repl/tests.rs

simon-curtis · 2024-10-23T21:07:03Z

Thanks for this PR! The take and skip commands already support binary streams, and their code could be reused for this command. I.e., bytes at ..$n is essentially take $n, bytes at $n.. is skip $n, and bytes at $n..$m is skip $n | take ($m - $n). I think it would make more sense if the binary stream handling was extracted out from those commands into a separate file and then reused across all these commands (skip, take, and bytes at).

I will take a look at this

simon-curtis · 2024-10-23T23:25:15Z

I have just pushed a fix for a performance bug where in the "relative index" branch would collect the entire ByteStream before limiting.

One thing I have noticed is bytes from a File doesn't have a length?

IanManske · 2024-10-24T03:20:58Z

Sorry, I should I have linked the code I was talking about. The take and skip commands I referred to before are other Nushell commands, not the take and skip defined on iterators in Rust.

https://github.com/nushell/nushell/blob/main/crates/nu-command/src/filters/skip/skip_.rs#L98C29-L114
https://github.com/nushell/nushell/blob/main/crates/nu-command/src/filters/take/take_.rs#L92-L105

You can reuse this code to implement bytes at (for byte streams).

simon-curtis · 2024-10-24T07:36:46Z

Dw I got what you meant, I just wanted to tidy up the code first

simon-curtis

All changes have been made. I have merged the logic for filters::skip and filters::take into the ByteStream impl along with a ByteStream::slice function - which is now used in bytes::at. I've also added some more tests for regression

IanManske

Thanks for all the tests and refactoring! A few comments:

crates/nu-command/src/bytes/at.rs

crates/nu-command/src/filters/skip/skip_.rs

crates/nu-command/src/filters/take/take_.rs

crates/nu-cmd-lang/src/example_support.rs

crates/nu-command/src/bytes/at.rs

simon-curtis · 2024-11-08T09:44:51Z

Thanks for the feedback btw, I'm learning a lot

sgvictorino · 2024-11-10T22:52:32Z

This also closes #13668

IanManske

Thanks for the changes! This is almost good to go, just a few last things.

crates/nu-command/src/bytes/at.rs

crates/nu-protocol/src/pipeline/byte_stream.rs

simon-curtis · 2024-11-18T18:27:58Z

I have moved logic into using the core IntRange instead of SubBytes as not to duplicate logic.

I have added a couple of functions on the Range to resolve the absolute range values where a length is known too, returning the original Bound value to allow the caller to apply the index as they wish.

In our case we both apply it by slicing Vec and using the new ByteStream::slice function.

I am still testing and need to format so hold out for a min

fdncred · 2024-12-10T12:34:49Z

what's the status here?

simon-curtis · 2024-12-18T09:21:58Z

I will fast forward and resolve merge conflicts tonight after work. AFAIK everything works and would be ready to merge in

- Removed explicit type annotation. - Changed panic to assert_eq

Co-authored-by: Ian Manske <ian.manske@pm.me>

WindSoilder · 2025-01-09T02:29:34Z

Hi, @IanManske . Can you take the last review when you get time?

IanManske

Thanks! This looks good at a glance, we'll land this to start testing in the wild.

WindSoilder requested changes Aug 12, 2024

View reviewed changes

crates/nu-command/src/bytes/at.rs Outdated Show resolved Hide resolved

crates/nu-command/src/bytes/at.rs Show resolved Hide resolved

simon-curtis commented Oct 10, 2024

View reviewed changes

crates/nu-cmd-lang/src/example_support.rs Outdated Show resolved Hide resolved

WindSoilder reviewed Oct 10, 2024

View reviewed changes

crates/nu-command/src/bytes/at.rs Outdated Show resolved Hide resolved

WindSoilder reviewed Oct 10, 2024

View reviewed changes

crates/nu-command/src/bytes/at.rs Show resolved Hide resolved

WindSoilder requested changes Oct 10, 2024

View reviewed changes

simon-curtis force-pushed the range-on-bytestream branch 3 times, most recently from c1c96b4 to 3d7519e Compare October 23, 2024 20:11

IanManske reviewed Oct 23, 2024

View reviewed changes

crates/nu-cmd-lang/src/example_support.rs Outdated Show resolved Hide resolved

tests/repl/tests.rs Outdated Show resolved Hide resolved

simon-curtis force-pushed the range-on-bytestream branch 5 times, most recently from d4abbf3 to b7b29ac Compare October 23, 2024 20:58

simon-curtis commented Nov 6, 2024

View reviewed changes

IanManske reviewed Nov 8, 2024

View reviewed changes

IanManske reviewed Nov 16, 2024

View reviewed changes

crates/nu-command/src/bytes/at.rs Outdated Show resolved Hide resolved

crates/nu-command/src/bytes/at.rs Outdated Show resolved Hide resolved

crates/nu-command/src/bytes/at.rs Outdated Show resolved Hide resolved

IanManske reviewed Nov 16, 2024

View reviewed changes

crates/nu-protocol/src/pipeline/byte_stream.rs Outdated Show resolved Hide resolved

crates/nu-protocol/src/pipeline/byte_stream.rs Outdated Show resolved Hide resolved

crates/nu-protocol/src/pipeline/byte_stream.rs Outdated Show resolved Hide resolved

simon-curtis force-pushed the range-on-bytestream branch from 310a78b to 6733f73 Compare November 19, 2024 11:20

read_byte_stream implemented for ByteStreams

cd6ff56

simon-curtis and others added 11 commits December 19, 2024 13:23

BytesAt command tests

b99d8c1

Requested changes

82c2ae1

Requested Changes

7efbfe0

- Removed explicit type annotation. - Changed panic to assert_eq

Corrected logic where ByteStream in equals ByteStream out

2a934e7

Abstracted away Skip/Take for ByteStream using existing filter code

88a785a

Clippy & test fixes

ced3866

Requested changes

946da5e

Clippy fixes

8e16bb6

Update at.rs

d78142f

Co-authored-by: Ian Manske <ian.manske@pm.me>

Moved to using IntRange over SubBytes

a13c71e

Tests passing

69f3738

simon-curtis force-pushed the range-on-bytestream branch from 6733f73 to 69f3738 Compare December 19, 2024 13:29

simon-curtis requested a review from WindSoilder December 19, 2024 13:31

fdncred added status:wait-until-after-nushell-release A:streaming Issues related to streaming data (or collecting data when it should be streamed) deprecated:pr-commands (deprecated: too vague) This PR changes our commands in some way labels Dec 19, 2024

Fixed clippy errors

cac6998

WindSoilder approved these changes Jan 9, 2025

View reviewed changes

sholderbach changed the title ~~Implementing ByteStream interuption on infinate stream~~ Implementing ByteStream interuption on infinite stream Jan 9, 2025

IanManske approved these changes Jan 11, 2025

View reviewed changes

IanManske merged commit f051628 into nushell:main Jan 11, 2025

github-actions bot added this to the v0.102.0 milestone Jan 11, 2025

Conversation

simon-curtis commented Aug 6, 2024

Description

User-Facing Changes

Tests + Formatting

Uh oh!

WindSoilder left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

WindSoilder left a comment

Choose a reason for hiding this comment

Uh oh!

simon-curtis commented Oct 23, 2024

Uh oh!

IanManske left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

simon-curtis commented Oct 23, 2024

Uh oh!

simon-curtis commented Oct 23, 2024

Uh oh!

IanManske commented Oct 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simon-curtis commented Oct 24, 2024

Uh oh!

simon-curtis left a comment

Choose a reason for hiding this comment

Uh oh!

IanManske left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

simon-curtis commented Nov 8, 2024

Uh oh!

sgvictorino commented Nov 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IanManske left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

simon-curtis commented Nov 18, 2024

Uh oh!

fdncred commented Dec 10, 2024

Uh oh!

simon-curtis commented Dec 18, 2024

Uh oh!

WindSoilder commented Jan 9, 2025

Uh oh!

IanManske left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

IanManske left a comment •

edited

Loading

IanManske commented Oct 24, 2024 •

edited

Loading

sgvictorino commented Nov 10, 2024 •

edited

Loading