Skip to content

GH-43276: [Go][Parquet] Make DeltaBitPacking Encoders/Decoders Generic#43279

Merged
joellubi merged 9 commits intoapache:mainfrom
joellubi:gh-43276
Jul 17, 2024
Merged

GH-43276: [Go][Parquet] Make DeltaBitPacking Encoders/Decoders Generic#43279
joellubi merged 9 commits intoapache:mainfrom
joellubi:gh-43276

Conversation

@joellubi
Copy link
Copy Markdown
Member

@joellubi joellubi commented Jul 16, 2024

Rationale for this change

Fixes: #43276

The slice of miniBlockValues in the decoder was getting reset between calls to Decode, while the remaining internal state of the decoder was persisted. By making the decoder generic, it became possible to bring miniBlockValues into the base decoder.

The encoder was also made generic for consistency. By avoiding reflection, encoding performance is improved (from around 700 MB/s before to about 1100 MB/s after on my machine).

What changes are included in this PR?

  • Passing test case reproducing the reported bug.
  • Benchmarks for DeltaBinaryPacked
  • Refactor of deltaBitPackDecoder and deltaBitPackEncoder to make them generic.

Are these changes tested?

Yes

Are there any user-facing changes?

The DeltaBitPackDecoders should no longer panic when decoding multiple batches from a single page.

@github-actions
Copy link
Copy Markdown

⚠️ GitHub issue #43276 has been automatically assigned in GitHub to PR creator.

@joellubi joellubi marked this pull request as ready for review July 16, 2024 14:33
@joellubi joellubi requested a review from zeroshade as a code owner July 16, 2024 14:33
Copy link
Copy Markdown
Member

@zeroshade zeroshade left a comment

Choose a reason for hiding this comment

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

LGTM Thanks!

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Jul 17, 2024
@joellubi joellubi merged commit cf9f3db into apache:main Jul 17, 2024
@joellubi joellubi removed the awaiting merge Awaiting merge label Jul 17, 2024
@conbench-apache-arrow
Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit cf9f3db.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Go][Parquet] Delta Binary Packed encoding with null read batchsize panic

2 participants