Skip to content

feat(cuda): support column projection and dictionary decoding in file scans - #9934

Open
0ax1 wants to merge 9 commits into
developfrom
ad/cuda-projected-file-scans
Open

0ax1 wants to merge 9 commits into
developfrom
ad/cuda-projected-file-scans

Conversation

@0ax1

@0ax1 0ax1 commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add column projection to CUDA scans, avoiding reads and decoding for unselected columns.
  • Decode dictionaries for consistent Arrow stream schemas, preserving defaults for other exports.
  • Cache schemas without reading a batch.
  • Preserve row-block boundaries, register the CUDA layout edition, and generate the C header.

@0ax1
0ax1 requested a review from myrrc September 17, 2026 16:31
@0ax1 0ax1 added the changelog/feature A new feature label Sep 17, 2026
@0ax1
0ax1 enabled auto-merge (squash) September 17, 2026 16:36
@codspeed

codspeed Bot commented Sep 17, 2026

Copy link
Copy Markdown

Merging this PR will regress 1 benchmark

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 3 improved benchmarks
❌ 1 regressed benchmark
✅ 2243 untouched benchmarks
⏩ 176 skipped benchmarks1
🗄️ 1 archived benchmark run2

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
WallTime arrow_checked_add_u32_neon[16384] 12.9 µs 20.4 µs -36.55%
Simulation decompress[u64, (4000, 1024)] 87.1 µs 71.6 µs +21.78%
Simulation allocate_drop_bytes[0] 635.5 ns 527.2 ns +20.55%
WallTime filtered_sink_i64_avx2[OneNullInEight] 26.2 µs 23.3 µs +12.4%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing ad/cuda-projected-file-scans (744abf1) with develop (75b6225)

Open in CodSpeed

Footnotes

  1. 176 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

  2. 1 benchmark was run, but is now archived. If it was deleted in another branch, consider rebasing to remove it from the report. Instead if it was added back, click here to restore it.

Signed-off-by: Alexander Droste <alexander.droste@protonmail.com>
Signed-off-by: Alexander Droste <alexander.droste@protonmail.com>
Signed-off-by: Alexander Droste <alexander.droste@protonmail.com>
Signed-off-by: Alexander Droste <alexander.droste@protonmail.com>
Signed-off-by: Alexander Droste <alexander.droste@protonmail.com>
Signed-off-by: Alexander Droste <alexander.droste@protonmail.com>
Signed-off-by: Alexander Droste <alexander.droste@protonmail.com>
Signed-off-by: Alexander Droste <alexander.droste@protonmail.com>
Signed-off-by: Alexander Droste <alexander.droste@protonmail.com>
@0ax1
0ax1 force-pushed the ad/cuda-projected-file-scans branch from cada5a9 to 744abf1 Compare September 18, 2026 13:45

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.

this looks very odd.....

@vortex-data vortex-data deleted a comment from github-actions Bot Sep 21, 2026
@vortex-data vortex-data deleted a comment from github-actions Bot Sep 21, 2026
@robert3005

Copy link
Copy Markdown
Contributor

@claude review

@github-actions

github-actions Bot commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @robert3005's task in 2m 49s —— View job


Claude PR Review

  • Read changed CUDA FFI, arrow, and layout files
  • Analyze projection, dictionary decoding, and schema caching logic
  • Review tests
  • Post review feedback

Reviewed against origin/develop (PR base 75b6225). This is a well-structured, well-tested change. Projection, the DictionaryExport policy, dtype-derived schema caching, and the CUDA edition registration all look coherent. A few points worth a look below.


Correctness / design notes

1. build.rs regenerates a checked-in header on every build — CI git status fragility.
vortex-cuda/ffi/build.rs runs cbindgen and writes cinclude/vortex_cuda.h (a committed file) into the source tree, then shells out to clang-format. CLAUDE.md notes CI asserts git status --porcelain is empty after a build. Two ways this can drift and turn red:

  • If the CI build environment's clang-format differs (or is absent — the script only warns, leaving the header unformatted), the regenerated header won't match the committed, formatted one.
  • Any cbindgen version drift produces different output.

Writing into the source tree from a build script is also generally discouraged (cargo expects build output in OUT_DIR). Since the file is committed, consider whether generation should be a checked/--verify step rather than an unconditional in-tree rewrite, or at least ensure the committer's clang-format/cbindgen versions match CI. Worth confirming the git-status-clean CI job passes on a clean environment.

2. CUDA edition registration (vortex-cuda/src/layout.rs).
The process-global REGISTRATION_LOCK guarding declare_family + register_edition + enable_edition is sound for serializing concurrent clones that share one editions registry. Two things to confirm:

  • declare_family("cuda") is per-session (called on session.editions()), so two distinct default sessions each declaring the family is fine. If declare_family were ever process-global, the session.editions().find(&CUDA_EDITION) guard wouldn't prevent a second distinct session from re-declaring → vortex_expect panic. Please confirm the registry is per-session.
  • The guard couples family declaration to the edition being absent. If a session ever had the cuda family declared but not CUDA_EDITION registered, declare_family would re-run and fail. This is an unlikely edge case, but the two registrations aren't independently guarded.

The concurrency test (test_cuda_registration_preserves_edition_policy) exercises the clone path nicely; a distinct-session case would round out coverage.

3. test_array allocation change (ffi src/lib.rs tests) — this is the "very odd" spot @joseph-isaacs flagged.
The switch from Arc::into_raw(Arc::new(..)) + Arc::decrement_strong_count to Box::into_raw(Box::new(..)) + vortex_ffi::vx_array_free is actually a correctness improvement, not a regression: vx_array is a box_wrapper! over ArrayRef, so production vx_array::new allocates a Box<ArrayRef> and vx_array_free/into_box reconstruct a Box. The old test helpers allocated/freed as an Arc, which mismatched the real wrapper representation. Aligning the tests to Box + the real vx_array_free is correct and removes latent UB if a test array were ever passed to an FFI fn that calls into_box. Might be worth a one-line comment saying so, since it reads oddly at first glance.

Smaller observations

  • projected_scan (ffi src/lib.rs): validating each name via fields.find(name) and then select(..).optimize(..) gives a clean per-name error — good. Duplicates are already rejected earlier in scan_columns via the HashSet, so this path is safe. Using SplitBy::LayoutSubSplitting { max_rows } correctly subdivides (never merges) layout boundaries, matching the doc promise that batches preserve on-disk boundaries. 👍
  • arrow/mod.rs next_array now returns a released end-of-stream marker instead of Option; consumers detect EOS via array.array.release.is_none(). The batch_lengths test relies on this — consistent.
  • Decode-mode schema staging in export_stream_array correctly computes the dtype-derived schema before export (error ordering) but only caches it after check_device. The get_schema-without-pulling-a-batch behavior for Decode is a nice latency win and is covered by scan_decodes_dictionaries_and_reuses_session_resources.
  • scan_export_ctx returning a per-scan ctx with DictionaryExport::Decode without mutating the shared session is the right call; the test asserting session policy stays Preserve and the allocator/pinned-pool identities are reused is a good guard against accidental session reconstruction.
  • Negative tests in projection.rs (usize::MAX len, misaligned pointer, 0xff invalid UTF-8, null-with-nonzero-len) look intentional and thorough despite reading oddly.

Not blocking

  • The codspeed regression (arrow_checked_add_u32_neon) is in an unrelated NEON compute benchmark and is almost certainly noise from a shared/hosted runner (the report itself warns about mixed runtime environments), not attributable to these CUDA-only changes.

Overall this looks solid and is already approved. The main thing I'd double-check before merge is item #1 (generated-header git status cleanliness in CI).
· ad/cuda-projected-file-scans

This branch has not been deployed

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

Labels

changelog/feature A new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants