Skip to content

GH-41562: [C++][Parquet] Decoding: Fix num_value handling in ByteStreamSplitDecoder#41565

Merged
pitrou merged 5 commits intoapache:mainfrom
mapleFU:fix-byte-stream-split-problem
May 7, 2024
Merged

GH-41562: [C++][Parquet] Decoding: Fix num_value handling in ByteStreamSplitDecoder#41565
pitrou merged 5 commits intoapache:mainfrom
mapleFU:fix-byte-stream-split-problem

Conversation

@mapleFU
Copy link
Copy Markdown
Member

@mapleFU mapleFU commented May 7, 2024

Rationale for this change

This problem is raised from #40094 . Original bug fixed here: #34140 , but this is corrupt in #40094 .

What changes are included in this PR?

Refine checking

Are these changes tested?

  • Will add

Are there any user-facing changes?

Bugfix

@mapleFU mapleFU requested a review from wgtmac as a code owner May 7, 2024 08:26
@github-actions github-actions bot added the awaiting review Awaiting review label May 7, 2024
@github-actions
Copy link
Copy Markdown

github-actions bot commented May 7, 2024

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

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels May 7, 2024
@mapleFU mapleFU force-pushed the fix-byte-stream-split-problem branch from b51608a to ddd0cde Compare May 7, 2024 14:44
Comment on lines +259 to +261
// In current implementations, `num_values` is the `num_values` field in the
// data page header, which may greater than the number of values in the data
// buffer.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
// In current implementations, `num_values` is the `num_values` field in the
// data page header, which may greater than the number of values in the data
// buffer.
// `num_values` comes from the data page header, and may be greater than the number of
// physical values in the data buffer if there are some omitted (null) values.
// `len`, on the other hand, is the size in bytes of the data buffer and
// directly relates to the number of physical values.

@pitrou
Copy link
Copy Markdown
Member

pitrou commented May 7, 2024

@github-actions crossbow submit -g cpp

@github-actions
Copy link
Copy Markdown

github-actions bot commented May 7, 2024

Revision: 7daee1c

Submitted crossbow builds: ursacomputing/crossbow @ actions-655ed96d55

Task Status
test-alpine-linux-cpp GitHub Actions
test-build-cpp-fuzz GitHub Actions
test-conda-cpp GitHub Actions
test-conda-cpp-valgrind GitHub Actions
test-cuda-cpp GitHub Actions
test-debian-12-cpp-amd64 GitHub Actions
test-debian-12-cpp-i386 GitHub Actions
test-fedora-39-cpp GitHub Actions
test-ubuntu-20.04-cpp GitHub Actions
test-ubuntu-20.04-cpp-bundled GitHub Actions
test-ubuntu-20.04-cpp-minimal-with-formats GitHub Actions
test-ubuntu-20.04-cpp-thread-sanitizer GitHub Actions
test-ubuntu-22.04-cpp GitHub Actions
test-ubuntu-22.04-cpp-20 GitHub Actions
test-ubuntu-22.04-cpp-emscripten GitHub Actions
test-ubuntu-22.04-cpp-no-threading GitHub Actions
test-ubuntu-24.04-cpp GitHub Actions
test-ubuntu-24.04-cpp-gcc-14 GitHub Actions

Copy link
Copy Markdown
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

Thanks a lot for fixing this, @mapleFU .
CI failures seem unrelated.

@pitrou pitrou merged commit 9cf0ee7 into apache:main May 7, 2024
@pitrou pitrou removed the awaiting committer review Awaiting committer review label May 7, 2024
@github-actions github-actions bot added the awaiting committer review Awaiting committer review label May 7, 2024
@mapleFU mapleFU deleted the fix-byte-stream-split-problem branch May 7, 2024 15:57
@conbench-apache-arrow
Copy link
Copy Markdown

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

There were no benchmark performance regressions. 🎉

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

raulcd pushed a commit that referenced this pull request May 8, 2024
…amSplitDecoder (#41565)

### Rationale for this change

This problem is raised from  #40094 . Original bug fixed here: #34140 , but this is corrupt in #40094 .

### What changes are included in this PR?

Refine checking

### Are these changes tested?

* [x] Will add

### Are there any user-facing changes?

Bugfix

* GitHub Issue: #41562

Authored-by: mwish <maplewish117@gmail.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
vibhatha pushed a commit to vibhatha/arrow that referenced this pull request May 25, 2024
…teStreamSplitDecoder (apache#41565)

### Rationale for this change

This problem is raised from  apache#40094 . Original bug fixed here: apache#34140 , but this is corrupt in apache#40094 .

### What changes are included in this PR?

Refine checking

### Are these changes tested?

* [x] Will add

### Are there any user-facing changes?

Bugfix

* GitHub Issue: apache#41562

Authored-by: mwish <maplewish117@gmail.com>
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.

2 participants