Skip to content

GH-48245: [C++][Parquet] Simplify GetVlqInt#48237

Merged
pitrou merged 2 commits into
apache:mainfrom
pitrou:getvlqint
Nov 25, 2025
Merged

GH-48245: [C++][Parquet] Simplify GetVlqInt#48237
pitrou merged 2 commits into
apache:mainfrom
pitrou:getvlqint

Conversation

@pitrou

@pitrou pitrou commented Nov 24, 2025

Copy link
Copy Markdown
Member

Rationale for this change

The BitReader::GetVlqInt implementation currently tries to read first from the cached value before falling back to reading from the buffer.

But this doesn't bring any benefit, since both code paths lead to the same processing step afterwards. So we can remove the code path that tries to read from the cached value. This will also make it easier to support big-endian platforms.

Are these changes tested?

Yes, by existing tests.

Are there any user-facing changes?

No.

@github-actions

Copy link
Copy Markdown

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

@pitrou

pitrou commented Nov 24, 2025

Copy link
Copy Markdown
Member Author

@ursabot please benchmark lang=C++

@voltrondatabot

Copy link
Copy Markdown

Benchmark runs are scheduled for commit 3550957. Watch https://buildkite.com/apache-arrow and https://conbench.ursa.dev for updates. A comment will be posted here when the runs are complete.

@conbench-apache-arrow

Copy link
Copy Markdown

Thanks for your patience. Conbench analyzed the 4 benchmarking runs that have been run so far on PR commit 3550957.

There were 4 benchmark results indicating a performance regression:

The full Conbench report has more details.

@WillAyd WillAyd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm

@github-actions github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Nov 25, 2025
@pitrou pitrou changed the title EXP: [C++][Parquet] Simplify GetVlqInt GH-48245: [C++][Parquet] Simplify GetVlqInt Nov 25, 2025
@pitrou pitrou marked this pull request as ready for review November 25, 2025 08:44
@pitrou pitrou merged commit 11d64b5 into apache:main Nov 25, 2025
42 of 44 checks passed
@pitrou pitrou removed the awaiting committer review Awaiting committer review label Nov 25, 2025
@pitrou pitrou deleted the getvlqint branch November 25, 2025 10:25
@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 11d64b5.

There were no benchmark performance regressions. 🎉

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

Mottl pushed a commit to Mottl/arrow that referenced this pull request May 26, 2026
### Rationale for this change

The `BitReader::GetVlqInt` implementation currently tries to read first from the cached value before falling back to reading from the buffer.

But this doesn't bring any benefit, since both code paths lead to the same processing step afterwards. So we can remove the code path that tries to read from the cached value. This will also make it easier to support big-endian platforms.

### Are these changes tested?

Yes, by existing tests.

### Are there any user-facing changes?

No.

Authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
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.

3 participants