#132 allow users to optionally provide an output buffer when calling transcode by baumgold · Pull Request #136 · JuliaIO/TranscodingStreams.jl

baumgold · 2023-03-15T14:37:03Z

An alternative to #134 to solve #132 without code-duplication.

…code

Moelf · 2023-03-15T15:05:35Z

I like this better

svilupp · 2023-03-15T22:15:31Z

Yeah, this is great. I was solving for minimum changes to the original code path, but this is much better/cleaner!

I’ll close my PR

baumgold · 2023-03-20T12:50:53Z

Is anyone with write-access available to review this PR so we can get it approved/merged/released? Thanks!

baumgold · 2023-03-23T10:53:41Z

@mkitti / @quinnj - are either of you able to review this? Thanks.

mkitti

Add tests for all the new signatures. See if you can consolidate the docstring into one.

quinnj · 2023-03-23T18:19:48Z

I'm going to request a review from @Drvi as well; will try to review myself a little later today

baumgold · 2023-03-23T20:00:34Z

@mkitti - Thanks for your review. I believe I addressed all of your suggestions. Could you please review again? Thanks.

Co-authored-by: Mark Kittisopikul <mkitti@users.noreply.github.com>

mkitti

Looks good to me. Let's give the others a chance to review. Ping me in a week.

baumgold · 2023-04-03T00:46:31Z

@quinnj / @Drvi / @mkitti - any new thoughts/comments? If not, shall we merge? Thanks.

mkitti

One more thought is that documenting the argument as ByteData might not be very useful. Rather we should just spell it out as Union{Vector{UInt8},Base.CodeUnits{UInt8}}.

Co-authored-by: Mark Kittisopikul <mkitti@users.noreply.github.com>

mkitti · 2023-04-03T14:06:44Z

Looks good to me. Will merge soon unless someone else speaks up.

JoaoAparicio · 2023-04-03T21:47:35Z

May I make a comment...

I note that as it is, there's no method with signature

transcode(::Type{TranscodingStreams.Codec}, ::Vector{UInt8}, ::Vector{UInt8})

There's

transcode(::Type{TranscodingStreams.Codec}, ::Vector{UInt8})

and there's

transcode(::Codec, ::Vector{UInt8}, ::Vector{UInt8})

As you can see the number of possibilities for manually defined methods grows combinatorially.

I would suggest that we fully resolve first the first argument, then the second etc. Then the combinations grow linearly. In this way we need only 3 manually defined methods to cover all combinations (right now there are 4 and not all combinations are covered)

mkitti · 2023-04-08T02:23:53Z

I'm feeling pretty good about where we are on this. I'm aiming to merge within the next 24 hours.

mkitti · 2023-04-09T14:09:03Z

I see no further comments or objections. Merging.

Please submit another pull request or issue to iterate further.

I will arrange for a release within the next five days. I'm currently targeting Tuesday.

I believe this to be a non-breaking change, but I will review further later.

JoaoAparicio · 2023-04-11T02:00:02Z

@mkitti this is where this will be used: apache/arrow-julia#422

mkitti · 2023-04-11T10:19:06Z

Released as 0.9.12

quinnj · 2023-04-11T20:03:59Z

Sorry for not being able to help review earlier. I think this is awesome. One note I want to follow up on: it seems that currently "bad things" happen when you try to pass in the same buffer as the input and output buffers to transcode. Can we add a validation check and throw an error in that case? (I just tried it on a 20mb stream and it blew up to 17.5gb before erroring!)

Or even better: can that be made to work? i.e. I have a use case in CloudStore.jl where people want to provide pre-allocated buffers in which to download a compressed file and then be able to decompress the data "in place". So they'd provide a big enough buffer for the final decompressed data, download the compressed data into the first part of the buffer, then do a decompress in place; is that possible?

baumgold · 2023-04-12T01:28:05Z

Can we add a validation check and throw an error in that case? (I just tried it on a 20mb stream and it blew up to 17.5gb before erroring!)

#140

Or even better: can that be made to work? i.e. I have a use case in CloudStore.jl where people want to provide pre-allocated buffers in which to download a compressed file and then be able to decompress the data "in place". So they'd provide a big enough buffer for the final decompressed data, download the compressed data into the first part of the buffer, then do a decompress in place; is that possible?

I don't know if this is possible. Is there data to show that this is a performance bottleneck? Can you just allocate a separate buffer?

The examples in many of the codec packages rely on being able to call `transcode` providing a `Codec` type and a string[^1][^2]. JuliaIO#136 removed type annotations from the trailing arguments for `transcode(::Type{C}, ...) where {C<:Codec}` causing a method ambiguity with `transcode(T, src::String)` in Julia `Base`[^3]. This adds an additional method to `Base.transcode` to resolve this ambiguity. FixesJuliaIO#139. [^1]: https://github.com/JuliaIO/CodecZlib.jl/tree/f9fddaa28c093c590a7a93358709df2945306bc7#usage [^2]: https://github.com/JuliaIO/CodecZstd.jl/tree/6327ffa9a3a12fc46d465dcfc8b30bed91cf284b#usage [^3]: https://github.com/JuliaLang/julia/blob/ff7b8eb00bf887f20bf57fb7e53be0070a242c07/base/c.jl#L306

The examples in many of the codec packages rely on being able to call `transcode` providing a `Codec` type and a string[^1][^2]. #136 removed type annotations from the trailing arguments for `transcode(::Type{C}, ...) where {C<:Codec}` causing a method ambiguity with `transcode(T, src::String)` in Julia `Base`[^3]. This adds an additional method to `Base.transcode` to resolve this ambiguity. Fixes#139. [^1]: https://github.com/JuliaIO/CodecZlib.jl/tree/f9fddaa28c093c590a7a93358709df2945306bc7#usage [^2]: https://github.com/JuliaIO/CodecZstd.jl/tree/6327ffa9a3a12fc46d465dcfc8b30bed91cf284b#usage [^3]: https://github.com/JuliaLang/julia/blob/ff7b8eb00bf887f20bf57fb7e53be0070a242c07/base/c.jl#L306

allow users to optionally provide an output buffer when calling trans…
f5b64a6
…code

baumgold mentioned this pull request Mar 15, 2023
in-place transcode when output size is known #134
Closed

mkitti requested changes Mar 23, 2023
View reviewed changes

Comment threadsrc/transcode.jl
Comment threadsrc/transcode.jl
Comment threadsrc/transcode.jl Outdated
Comment threadsrc/transcode.jl Outdated
Comment threadsrc/transcode.jl Outdated

quinnj requested a review from Drvi March 23, 2023 18:19

add tests and refactor docstring
9e5bb40

baumgold requested review from mkitti and removed request for Drvi March 23, 2023 20:00

mkitti reviewed Mar 24, 2023
View reviewed changes

Comment threadsrc/transcode.jl

nicer formating
090cb9c
Co-authored-by: Mark Kittisopikul <mkitti@users.noreply.github.com>

baumgold requested a review from mkitti March 25, 2023 10:53

new copydata! method
b6d5f9d

mkitti approved these changes Mar 25, 2023
View reviewed changes

mkitti reviewed Mar 28, 2023
View reviewed changes

Comment threadsrc/transcode.jl

reassociate docstring with correct method
2ce1efb

mkitti requested changes Apr 3, 2023
View reviewed changes

Comment threadsrc/transcode.jl Outdated
Comment threadsrc/transcode.jl Outdated
Comment threadsrc/transcode.jl Outdated

baumgoldand others added 5 commits April 3, 2023 09:42

Expand ByteData
49b5b82
Co-authored-by: Mark Kittisopikul <mkitti@users.noreply.github.com>

Expand ByteData
78d2b3b
Co-authored-by: Mark Kittisopikul <mkitti@users.noreply.github.com>

Expand ByteData
20e82d9
Co-authored-by: Mark Kittisopikul <mkitti@users.noreply.github.com>

clarify formatting
c010983

formatting
829c655

JoaoAparicio reviewed Apr 3, 2023
View reviewed changes

Comment threadsrc/transcode.jl Outdated
Comment threadsrc/transcode.jl Outdated
Comment threadsrc/transcode.jl Outdated

mkitti reviewed Apr 8, 2023
View reviewed changes

Comment threadsrc/transcode.jl

add doc-string
93c28d4

baumgold requested a review from mkitti April 8, 2023 02:02

mkitti reviewed Apr 8, 2023
View reviewed changes

Comment threadsrc/buffer.jl Outdated

JoaoAparicio approved these changes Apr 8, 2023
View reviewed changes

mkitti reviewed Apr 8, 2023
View reviewed changes

Comment threadsrc/transcode.jl

mkitti self-requested a review April 8, 2023 02:24

mkitti approved these changes Apr 8, 2023
View reviewed changes

mkitti self-assigned this Apr 8, 2023

mkitti requested review from bicycle1885, jakobnissen and quinnj April 8, 2023 02:25

baumgold added 2 commits April 7, 2023 22:58

replace keepbytes with marginpos
b3f444b

fix typeo
9a4b867

mkitti merged commit 3ece750 into JuliaIO:masterApr 9, 2023

JoaoAparicio mentioned this pull request Apr 11, 2023
Pre-allocate buffer apache/arrow-julia#422
Merged

baumgold mentioned this pull request Apr 12, 2023
assert that input and output buffers passed to transcode! are independent #140
Merged

This was referenced Apr 13, 2023
MethodError: transcode(::Type{CodecZlib.ZlibCompressor}, ::String) is ambiguous bauglir/Kroki.jl#41
Closed
transcode(::Type{CodecZlib.GzipCompressor}, ::String) is ambiguous on v0.9.12 #139
Closed

bauglir mentioned this pull request Apr 14, 2023
Resolve transcode method ambiguity #141
Merged

nhz2 mentioned this pull request Jul 5, 2024
universal in-place decompression interface #132
Open

Uh oh!

Conversation

baumgold commented Mar 15, 2023

Uh oh!

Moelf commented Mar 15, 2023

Uh oh!

svilupp commented Mar 15, 2023

Uh oh!

baumgold commented Mar 20, 2023

Uh oh!

baumgold commented Mar 23, 2023

Uh oh!

mkitti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

quinnj commented Mar 23, 2023

Uh oh!

baumgold commented Mar 23, 2023

Uh oh!

Uh oh!

mkitti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

baumgold commented Apr 3, 2023

Uh oh!

mkitti 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!

Uh oh!

mkitti commented Apr 3, 2023

Uh oh!

JoaoAparicio commented Apr 3, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mkitti commented Apr 8, 2023

Uh oh!

mkitti commented Apr 9, 2023

Uh oh!

JoaoAparicio commented Apr 11, 2023

Uh oh!

mkitti commented Apr 11, 2023

Uh oh!

quinnj commented Apr 11, 2023

Uh oh!

baumgold commented Apr 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mkitti left a comment •
edited
Loading

JoaoAparicio commented Apr 3, 2023•
edited
Loading