Skip to content

Conversation

@pitrou
Copy link
Member

@pitrou pitrou commented Oct 30, 2025

Rationale for this change

The benchmark was instantiating the ColumnReader with less values than it would later attempt to read. The ReadBatch method would then return 0 prematurely and the loop would never progress.

Are these changes tested?

Manually and by continuous benchmarking.

Are there any user-facing changes?

No.

The benchmark was instantiating the ColumnReader with less values than it would later attempt to read.
The `ReadBatch` method would then return 0 prematurely and the loop would never progress.
@pitrou
Copy link
Member Author

pitrou commented Oct 30, 2025

@ursabot please benchmark lang=C++

@voltrondatabot
Copy link

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

Comment on lines 206 to +207
std::shared_ptr<Int64Reader> reader =
BuildReader(src, state.range(1), codec, schema.get());
BuildReader(src, kNumValues, codec, schema.get());
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the fix: we were instantiating the ColumnReader with the read batch size (state.range(1)) instead of the total number of values (state.range(0)).

@pitrou pitrou marked this pull request as ready for review October 30, 2025 16:04
@pitrou pitrou requested a review from wgtmac as a code owner October 30, 2025 16:04
@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Oct 30, 2025
@conbench-apache-arrow
Copy link

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

There were 80 benchmark results indicating a performance regression:

The full Conbench report has more details.

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.

2 participants