Skip to content

[QDP] Fix streaming host-to-device copy size - #1464

Open
viiccwen wants to merge 2 commits into
apache:mainfrom
viiccwen:fix-streaming-copy-size
Open

[QDP] Fix streaming host-to-device copy size#1464
viiccwen wants to merge 2 commits into
apache:mainfrom
viiccwen:fix-streaming-copy-size

Conversation

@viiccwen

@viiccwen viiccwen commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Related Issues

Closes #1463

Changes

  • Bug fix
  • New feature
  • Refactoring
  • Documentation
  • Test
  • CI/CD pipeline
  • Other

Why

The streaming Parquet encoder passed an f64 element count to a helper that expects a byte count. Consequently, amplitude and angle encoders copied only one eighth of every input chunk to GPU staging memory. The CUDA copy completed successfully, so callers received a state tensor with no error even though its amplitudes were incorrect.

Existing integration tests only checked the returned DLPack shape and therefore did not detect this numerical corruption.

How

  • Convert the Parquet chunk length from f64 elements to bytes before calling PipelineContext::async_copy_to_device.
  • Use checked_mul(size_of::<f64>()) and return a memory-allocation error if the byte-count calculation overflows.
  • Extend the existing angle streaming integration test instead of adding a separate overlapping test:
    • encode three distinct angle samples from Parquet;
    • download the returned complex128 DLPack tensor;
    • compare all 24 complex amplitudes with the analytical angle-encoding
      formula;
    • retain the existing shape and cleanup checks.

The regression test was also run against the previous implementation. It failed as expected: state[0] was 1.0, while the analytical result was 0.9316157966884513. The test passes with this change.

Checklist

  • Added or updated unit tests for all changes
  • Added or updated documentation for all changes (the copy-unit contract is documented inline; there is no user-facing API change)

Signed-off-by: viiccwen <vicwen@apache.org>
@viiccwen
viiccwen requested a review from 400Ping as a code owner August 5, 2026 07:47
@viiccwen
viiccwen requested review from ryankert01 and a lite review from Copilot August 5, 2026 07:47

Copilot AI 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.

Pull request overview

Fixes a correctness bug in the QDP streaming Parquet GPU encoder where an element count (f64 elements) was incorrectly passed to a helper expecting a byte count, causing truncated H2D copies and silently corrupted amplitude/angle results. Adds a stronger integration test that validates numerical output (not just shape) to prevent regressions.

Changes:

  • Convert streaming Parquet chunk length from element-count to byte-count before PipelineContext::async_copy_to_device, with overflow checking.
  • Extend the existing GPU angle streaming integration test to download the produced complex128 DLPack tensor and validate all amplitudes against the analytical formula.
  • Add a test helper to download complex128 CUDA DLPack tensors to host for verification.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
qdp/qdp-core/src/encoding/mod.rs Fixes H2D copy length by converting element counts to byte counts with overflow handling.
qdp/qdp-core/tests/gpu_angle_encoding.rs Strengthens the Parquet streaming angle test to validate numerical correctness by downloading and checking amplitudes.
qdp/qdp-core/tests/common/mod.rs Adds a CUDA DLPack complex128 download helper used by the new/extended integration test.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread qdp/qdp-core/tests/common/mod.rs Outdated
Signed-off-by: viiccwen <vicwen@apache.org>
@rich7420

rich7420 commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

LGTM.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] QDP streaming Parquet encoding copies an element count as a byte count

3 participants