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

Remove fallibility from paruqet RleEncoder (#2226) #2259

Merged
merged 1 commit into from Aug 2, 2022

Conversation

tustvold
Copy link
Contributor

@tustvold tustvold commented Aug 1, 2022

Which issue does this PR close?

Closes #2226

Rationale for this change

Follow on to #2231

What changes are included in this PR?

RLEEncoder is no longer fallible and so we can simplify its interface

Are there any user-facing changes?

No, the encoding module is experimental

@github-actions github-actions bot added the parquet Changes to the parquet crate label Aug 1, 2022
@@ -132,23 +132,21 @@ impl RleEncoder {
}

/// Encodes `value`, which must be representable with `bit_width` bits.
/// Returns true if the value fits in buffer, false if it doesn't, or
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am very happy to see this gone, it made for a very confusing interface imo

@tustvold tustvold changed the title Remove fallibility from RLEEncoder (#2226) Remove fallibility from RleEncoder (#2226) Aug 1, 2022
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Looks like a great improvement to me

@@ -244,10 +239,9 @@ impl RleEncoder {
);
self.num_buffered_values = 0;
self.repeat_count = 0;
Ok(())
Copy link
Contributor

Choose a reason for hiding this comment

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

So it seems like the flush_* functions are the root of the chain and then the "can't fail" gets to percolate up? 👍

@alamb alamb merged commit 4222f5a into apache:master Aug 2, 2022
@alamb alamb changed the title Remove fallibility from RleEncoder (#2226) Remove fallibility from paruqet RleEncoder (#2226) Aug 2, 2022
@ursabot
Copy link

ursabot commented Aug 2, 2022

Benchmark runs are scheduled for baseline = 9a4b1c9 and contender = 4222f5a. 4222f5a is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

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

Successfully merging this pull request may close these issues.

Automatically Grow Parquet BitWriter Buffer
3 participants