Skip to content

Add Hegel coverage for Arrow extension round trips - #11361

Draft
connortsui20 wants to merge 1 commit into
developfrom
ct/arrow-roundtrip-properties
Draft

connortsui20 wants to merge 1 commit into
developfrom
ct/arrow-roundtrip-properties

Conversation

@connortsui20

@connortsui20 connortsui20 commented Nov 9, 2026 •

Copy link
Copy Markdown
Member

Summary

Builds on #11360.

Plugin tests need to check complete dtype identity and array values across nesting boundaries. Generate each plugin's metadata and storage in its owning crate, with shared transport checks in vortex-arrow, so neither vortex-array nor vortex-arrow needs to depend on the plugin crates.

Changes

Use the workspace's existing Hegel dependency. Each generated type is checked at the top level and inside structs, lists, fixed-size lists, map values, dictionaries, and run-end arrays, including nullability, explicit fields, and targetless exports. Top-level FixedShapeTensor exports still require an explicit field.

Keep runs bounded to 100 Hegel cases per suite, at most 32 leaf rows, and three extra nesting levels. Independent draws let Hegel shrink metadata choices, null masks, and values, and failures print a replay blob.

The fuzz target wraps storage from the existing arbitrary array generator. Hegel generation has a separate feature from the shared transport checks because Hegel's native backend cannot link under libFuzzer's coverage flags on macOS. Seeded tests cover all 15 included type families. Variant is deliberately excluded pending #11359.

The existing generators exercise the typed adapters through the owning crates' initializers. The adapter's policy and malformed-metadata regressions live in #11360, so this layer keeps its generated transport coverage without duplicating those cases.

Validation
  • The property-test commit is unchanged apart from rebasing onto the new stack.
  • Pre-push hook: DCO checks and workspace Clippy with all targets, all features, and warnings denied.
  • Runtime tests, doctests, and fuzz runs were not repeated.

@connortsui20
connortsui20 added this pull request to stack #11362 November 9, 2026 18:16
@connortsui20 connortsui20 mentioned this pull request Nov 9, 2026
12 of 49 tasks
@connortsui20
connortsui20 force-pushed the ct/arrow-roundtrip-properties branch 5 times, most recently from fc51d62 to f2987e6 Compare November 9, 2026 22:12
@connortsui20
connortsui20 force-pushed the ct/arrow-roundtrip-properties branch from f2987e6 to 73f31f1 Compare November 10, 2026 14:52
@connortsui20
connortsui20 force-pushed the ct/arrow-roundtrip-properties branch 2 times, most recently from f32781e to 73a471c Compare November 10, 2026 16:16
@connortsui20
connortsui20 force-pushed the ct/arrow-roundtrip-properties branch from 73a471c to 53f31ab Compare November 10, 2026 16:52
@connortsui20
connortsui20 removed this pull request from stack #11362 November 10, 2026 16:55
@connortsui20
connortsui20 force-pushed the ct/arrow-roundtrip-properties branch from 53f31ab to d5c2d05 Compare November 10, 2026 18:18
Base automatically changed from ct/arrow-roundtrip-fixes to develop November 10, 2026 23:12
@connortsui20
connortsui20 force-pushed the ct/arrow-roundtrip-properties branch from d5c2d05 to 9e3f744 Compare November 10, 2026 23:12
@connortsui20
connortsui20 marked this pull request as ready for review November 11, 2026 13:26
@connortsui20

Copy link
Copy Markdown
Member Author

I think I want to also delete a lot of the existing tests too

@codspeed

codspeed Bot commented Nov 12, 2026 •

Copy link
Copy Markdown

Merging this PR will degrade performance by 11.55%

2 warnings

⚠️ 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

⚠️ 3 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

❌ 1 regressed benchmark
✅ 1920 untouched benchmarks
⏩ 50 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
❌ Walltime compare_int_eq_neon 4.3 µs 4.8 µs -11.55%

Tip

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


Comparing ct/arrow-roundtrip-properties (0d6def1) with develop (d77b8f7)

Open in CodSpeed

Footnotes

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

@connortsui20
connortsui20 force-pushed the ct/arrow-roundtrip-properties branch 2 times, most recently from 8e2125f to 35520db Compare November 12, 2026 15:51
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
@connortsui20
connortsui20 force-pushed the ct/arrow-roundtrip-properties branch from 35520db to 0d6def1 Compare November 12, 2026 15:56

Copy link
Copy Markdown
Member Author

Merge #11184 before this PR. It establishes the contract that metadata determines extension storage, and reads must reject stored storage that disagrees with that declaration. The conformance tests here need to target that contract.

After rebasing, this PR also needs a testing redesign: the properties should replace ordinary positive unit coverage, with independent expectations and useful extension-specific checks. Adding round-trip properties while leaving all the old happy-path tests intact is not the intended result.

Handoff for a future agent: merge order, API migration, and the conformance test redesign

1. Resume from the actual branch state

At this handoff, the published head of this PR is 0d6def18cad145f475b506ec739a59fa9dc47cd2 on ct/arrow-roundtrip-properties. The inspected head of #11184 is e6b34f0cafc1431f5ec11a3f91da5760ac36c79f on ct/ext-storage-declaration. These are checkpoints, not assumptions about the heads when work resumes.

Refresh #11184, resolve its conflicts, complete review and validation, and merge it first. It was still draft and conflicting at this handoff, and its listed CI jobs were skipped. Do not treat that as a merge-ready result. Keep the production contract change and the test redesign separately reviewable, and preserve existing regression coverage until its replacement is demonstrated.

Then rebase this PR onto the updated develop containing #11184. Inspect the landed code because the API can change during review. Do not add compatibility plumbing to support both extension construction APIs.

There are also interrupted, uncommitted edits in the original vortex2 worktree, limited to vortex-arrow/src/test_harness/generate.rs, mod.rs, and round_trip.rs. They started a paired storage generator, an independent Arrow checker, and nonidentity dictionary/unequal-run fixtures. They are unfinished, are not in the published commit, and have not passed validation. An initial cargo check -p vortex-arrow --features _test-harness-hegel --lib failed with E0283 at the untyped .collect() passed to StructArray::try_new in generate.rs. Inspect these edits before reuse or replacement, preserve unrelated work, and do not count the conformance redesign as implemented.

2. Migrate the fixtures to the landed extension API

The following describes the inspected #11184 patch. Confirm it against the final merged version.

Area Required update
Dtype construction ExtDType::try_new and try_with_vtable take top-level nullability instead of a caller-supplied storage dtype.
Array construction ExtensionArray::try_new_from_vtable keeps accepting storage, but its metadata must now declare that storage exactly.
Vector Replace EmptyMetadata with VectorMatcherMetadata::try_new(element_ptype, dimensions).
Fixed-shape tensor Use FixedShapeTensorMetadata::new(element_ptype, shape). Include element type in generated and explicit metadata cases.
Native spatial types Add the coordinate dimension with SpatialMetadata::with_dimension. The public Dimension re-export becomes available.
WKB Keep the dimension absent. WKB metadata rejects a coordinate dimension.
Metadata decoding deserialize_metadata receives the stored storage dtype. SpatialMetadata no longer implements prost::Message, so use the extension serialization APIs.
Temporal, UUID, and JSON fixtures Update direct dtype constructor calls. Their fundamental storage declarations do not gain extra metadata fields.

Update both the owning crate property suites and the dtype factory in fuzz/src/arrow_roundtrip.rs. Most of this migration belongs in the fixture builders. The Arrow representations and serialized metadata bytes are intended to stay compatible, so independent Arrow expectations should largely survive.

Metadata properties must test (serialized metadata, stored storage dtype) -> complete metadata. Bytes alone no longer recover all in-memory facts. Vector metadata bytes remain empty, while element type and width come from stored storage. Tensor element type and native geometry dimension are also recovered from storage.

Move wrong-storage checks to the boundary that can receive wrong storage: dtype deserialization, array construction, or scalar construction. Use the full dtype/plugin deserialization path when testing exact storage agreement, rather than assuming the lower-level deserialize_metadata callback performs that check.

Do not assume that every mutation of stored storage must fail. A different valid vector width or float element type can describe another valid vector, since its bytes leave those facts out. A nullable vector element must fail. A tensor list width that disagrees with its serialized shape must fail. Derive each expected outcome from the type's contract.

Add a direct property that changing top-level nullability preserves metadata and nested storage constraints. Deserializing storage forms that differ only in top-level nullability must reconstruct the same metadata.

3. Make generated conformance own ordinary successful behavior

The current suites mostly build a Vortex array, let Vortex choose an Arrow field, and ask Vortex to read its own output. That can miss matching import/export mistakes. The existing numeric generator is also limited to 0..100, and each generated list layer uses one shared width.

Generate a small typed logical case beside each extension's tests: source values, nullability, metadata, and shape information. Build the Vortex array and the reference Arrow field/array separately from that case. Expected Arrow layouts and native metadata must come from arrow-rs, GeoArrow, or test-owned mappings, never from the Vortex exporter under test.

Share small one-way assertions that import independently built Arrow inputs and export to independently expected Arrow representations. Also check canonical fields and schemas against independent expectations. Keep round-trip assertions for composition and nested transport. Test JSON's supported string layouts and WKB's binary layouts, including explicit targets and the canonical default behavior.

Keep the shared harness free of extension-specific policies. Use ordinary local generators and check functions, not a trait that forces every extension to implement every check. Keep Hegel's native backend out of the libFuzzer dependency path, and preserve existing fuzz seed decoding.

Strengthen the generators:

  • Draw numeric values at their actual widths, including negative numbers, fractions, and boundaries. For byte-preserving floating-point transport, compare bits where needed and ignore payloads beneath nulls.
  • Generate unequal list lengths, empty children, useful null patterns, repeated/reordered dictionary codes, and unequal run lengths.
  • Construct valid values for native semantic checks: ordered rect bounds, closed polygon rings, valid WKB, and UUID bytes with the declared version.
  • Keep arrays and nesting bounded. Sample metadata and deep combinations instead of expanding the entire metadata matrix for every generated case.

4. Reuse cases for extension-specific properties

Use separate properties with shared local generators. A tensor stride test must not run the Arrow matrix first, and a UUID aggregate test must not depend on Arrow transport.

Area Checks to add or consolidate
UUID Metadata encoding/decoding, native decoding against the uuid crate, and sortedness/min/max against source byte arrays. Include strict ordering, duplicates, nulls, and empty inputs according to the aggregate contracts.
JSON Exact text and native-value preservation, including whitespace, escapes, Unicode, arrays, and objects.
Vector and tensor Valid dtype construction, metadata serialization, and tensor physical shape/strides against the existing slow reference. Keep tensor layout checks independent of array allocation so useful shape cases are not constrained by row storage.
Spatial CRS/dimension preservation, native coordinate decoding, and WKB decoding against source geometries.
Shared array behavior Slice, take, and filter preserve values, nulls, lengths, and extension metadata. Compute expected selections from the source case, and execute lazy results through the batch path before comparison.

Keep vector numerical algorithms, spatial predicates, and unrelated core conformance suites outside this pass. The additional properties above have direct references and reuse the same logical cases.

5. Delete the tests that the stronger properties supersede

Audit tests from the rebased tree, including changes introduced by #11184. Remove whole redundant test functions and unused helpers, not only individual value assertions.

  • Remove spatial's eight nested_arrow_round_trip cases and their check_nested_arrow_round_trip helper. The shared container properties replace that coverage.
  • Replace ordinary vector array/schema round trips, JSON import/export examples, spatial positive field/import/export checks, and overlapping UUID checks with the stronger properties.
  • Replace repetitive tensor metadata and layout examples with generated reference checks. Reuse the existing slow stride reference rather than copying it.
  • Consolidate positive dtype-validation examples into construction and deserialization conformance. Do not retain a redundant positive test only as a smoke test.
  • Preserve focused rejection tests, literal wire-compatibility fixtures, stream/FFI tests, export-option tests, and distinct routes such as extension-valued map keys unless the replacement explicitly covers the same contract.

For each deletion, identify its replacement property and any important trigger that remains explicit. The acceptance criterion is stronger coverage with less duplicate scaffolding, not a fixed deletion count.

Use Hegel explicit examples with the same check functions as generated cases, but keep only good examples. Do not move every old fixture into an attribute. Useful directed cases include a zero-width array with nonzero row count, nulls masking stored values, nonidentity dictionary selection, unequal runs, and a nontrivial tensor permutation containing a zero axis. Preserve relevant historical bug triggers. Finite dispatch choices such as supported temporal units and coordinate dimensions can use compact deterministic mapping checks, while deep combinations remain generated.

6. Validate and hand off

Local targeted tests and CI checks are authorized for this implementation. Run the affected crate tests and fuzz seed tests, scoped formatting and Clippy, and targeted docs and MSRV checks that match CI's flags. Keep workspace-wide execution in CI. Follow the repository guidance and the pinned formatting toolchain.

The last published commit includes two earlier CI fixes that must survive the rebase: vortex-arrow/_test-harness enables vortex-array/_test-harness, and the fuzz module's rustdoc links use fully qualified targets. Passing a workspace build can hide missing dependency feature wiring, so include the standalone/no-dev-dependency feature configuration relevant to MSRV.

Review the complete diff with $rust-style, including test organization, comments, docs, module registrations, public test-harness contracts, and all callers. Report which old tests each property replaces, what distinct examples remain, and the checks actually run. Do not claim that the properties replace contracts they never exercise.

@connortsui20
connortsui20 marked this pull request as draft November 12, 2026 16:18

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant