Skip to content

Decode stored DecimalType and PType values strictly - #10755

Merged
connortsui20 merged 4 commits into
developfrom
ct/strict-enum-decoding
Oct 26, 2026
Merged

connortsui20 merged 4 commits into
developfrom
ct/strict-enum-decoding

Conversation

@connortsui20

@connortsui20 connortsui20 commented Oct 23, 2026 •

Copy link
Copy Markdown
Member

Summary

Prost decodes an enum value it does not know as the first variant, so a stored DecimalType or PType outside its enum silently reads as I8 or U8. The reader then decodes the buffer or child at the wrong width, which either fails later with an unrelated error or returns wrong values. Readers need to reject such values before a later release adds a variant, such as a wider integer PType. The read path for old decimal arrays in #10701 also depends on the stored values_type, because it compares that width with the canonical width.

Changes

Each metadata struct with one of these fields gets a get_<field>() accessor that returns an error naming the field and the raw value, and deserialize calls it before reading any buffer or child. The protobuf dtype decoder does the same through pb::PType::try_from. Files from current writers read as before, and compat.py check --mode all passes against every published fixture.

The handwritten enum fields use prost(int32), so prost no longer generates lenient enum getters or setters. The protobuf tags and encoded bytes stay the same.

For review, read the commits one at a time.

API Changes

Removes the generated enum getters and setters from the affected handwritten metadata structs. Decoding uses the strict get_<field>() methods.

🤖 Generated with Claude Code

@codspeed

codspeed Bot commented Oct 23, 2026 •

Copy link
Copy Markdown

Merging this PR will degrade performance by 11.33%

4 warnings, 1 note

⚠️ Walltime results may be unreliable

Some benchmarks used the Walltime instrument outside CodSpeed Macro Runners, which can make the results inconsistent.

Learn more about Macro Runners

See affected benchmarks

⚠️ Unable to measure the execution of 2 benchmarks

These benchmarks measured no execution time, so their results are not comparable and count as unchanged. This usually happens when the compiler removes the code under test.

Prevent compiler optimizations

See affected benchmarks

⚠️ 15 benchmarks are dominated by system calls

System calls cannot be simulated consistently and are left out of the measure. The reported value understates the real cost. Switch to the Walltime instrument to measure them.

Learn more about system calls

See affected benchmarks

⚠️ Base and head ran on different runner types

Some benchmarks with significant performance changes were compared across different runner types, so the changes might not come from the code. Run base and head on the same runner type, or use CodSpeed Macro Runners for more stable results.

Learn more about Macro Runners

Open the report in CodSpeed to investigate

ℹ️ 40 benchmarks have no flamegraph

The flamegraph wasn't generated: the symbols of the profiling trace could not be resolved, because no information about the language frames was collected.

See affected benchmarks

❌ 1 regressed benchmark
✅ 2438 untouched benchmarks
⏩ 12 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
❌ CPU Simulation test_scan_filter[int] 3 ms 3.4 ms -11.33%

Tip

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


Comparing ct/strict-enum-decoding (07368ee) with develop (f476db4)

Open in CodSpeed

Footnotes

  1. 12 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. ↩

Comment thread vortex-array/src/test_harness/mod.rs Outdated
@connortsui20
connortsui20 force-pushed the ct/strict-enum-decoding branch from d5116b9 to 26cd657 Compare October 26, 2026 15:45
@connortsui20
connortsui20 marked this pull request as ready for review October 26, 2026 15:56
@connortsui20
connortsui20 marked this pull request as draft October 26, 2026 16:21
@connortsui20 connortsui20 added the changelog/break A breaking API change label Oct 26, 2026 — with ChatGPT Codex Connector
@connortsui20 connortsui20 removed the changelog/fix A bug fix label Oct 26, 2026
@connortsui20
connortsui20 added this pull request to stack #10832 October 26, 2026 18:08
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
@connortsui20
connortsui20 force-pushed the ct/strict-enum-decoding branch from 957e9ea to 07368ee Compare October 26, 2026 18:38
dict_len: u32,
#[prost(uint32, repeated, tag = "3")]
dict: Vec<u32>,
#[prost(enumeration = "PType", tag = "4")]

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

So in general I think we don't want to do this anymore

@connortsui20
connortsui20 marked this pull request as ready for review October 26, 2026 18:59
@connortsui20
connortsui20 merged commit 423354d into develop Oct 26, 2026
89 of 90 checks passed
@connortsui20
connortsui20 deleted the ct/strict-enum-decoding branch October 26, 2026 19:14
connortsui20 added a commit that referenced this pull request Oct 26, 2026
Depends on #10755.

## Summary

Public metadata fields expose the stored layout as Rust API and let
callers bypass typed accessors. Restrict them to the owning crate while
preserving the encoded metadata.

## Changes

Add getters for CUDA’s Zstd metadata and a constructor and CRS getter
for spatial callers. The TUI uses the existing flat-layout accessor.
Internal struct literals and metadata golden files stay unchanged.

Formatting and clippy passed. Tests passed apart from
`chunked_input_with_mixed_dimensions_returns_error`, which fails
identically on the base. GPU tests were skipped without `nvcc`.

## API Changes

Metadata fields are now `pub(crate)`. Direct field access and struct
literals from other crates no longer compile.

Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/break A breaking API change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants