Skip to content

Check the Arrow type of a Vortex ID export target - #11301

Merged
connortsui20 merged 1 commit into
developfrom
ct/arrow-ext-export-guard
Nov 9, 2026
Merged

connortsui20 merged 1 commit into
developfrom
ct/arrow-ext-export-guard

Conversation

@connortsui20

@connortsui20 connortsui20 commented Nov 6, 2026 •

Copy link
Copy Markdown
Member

Summary

Since #11224, an export to an Arrow field that names a Vortex ID only checks the metadata, so the field's Arrow type can silently change what the values mean. A millisecond timestamp exported to a Timestamp(Second) field that names vortex.timestamp comes back as seconds with no error, where the same export failed before #11224. This already shipped in 0.92.0 and 0.93.0.

Also does some refactoring, which might make it a bit harder to review, but I think the code is much easier to understand now.

Changes

The Arrow type of the target must now import as the storage or as the type itself, and an extension array fails on a target that names a different known Vortex ID. A plain array still exports to such a target, since plugins like Vector pass their target on to their storage. The import also no longer wraps a type twice, which is a smaller fix from the same review.

API Changes

  • An export to a target that names a Vortex ID fails if it would change the type of the values.

@connortsui20
connortsui20 force-pushed the ct/arrow-ext-export-guard branch from 21038a8 to 4820228 Compare November 6, 2026 15:58
@connortsui20 connortsui20 added the changelog/fix A bug fix label Nov 6, 2026
@connortsui20
connortsui20 marked this pull request as ready for review November 6, 2026 16:00
@connortsui20
connortsui20 force-pushed the ct/arrow-ext-export-guard branch from 4820228 to 0679fa4 Compare November 6, 2026 16:10
@connortsui20
connortsui20 marked this pull request as draft November 6, 2026 16:16
@connortsui20

Copy link
Copy Markdown
Member Author

actually I think we can make a better abstraction

@connortsui20
connortsui20 force-pushed the ct/arrow-ext-export-guard branch 3 times, most recently from 574134e to b490ec1 Compare November 6, 2026 16:48
Follow-up to #11224, from its review.

An export to a target that names the Vortex ID of the array checked only
the metadata, so a millisecond timestamp exported to a `Timestamp(Second)`
field returned its raw values as seconds. The Arrow type of the target must
now import as the storage, or as the type itself. The metadata check
compares bytes in every session. An extension array fails on a target that
names another known Vortex ID instead of taking that type.

The import no longer wraps a field whose storage already has the type that
it names, such as an Arrow timestamp that names `vortex.timestamp`. Map
entries skip the Vortex ID rebuild, which matches the dtype import.

Signed-off-by: Connor Tsui <[email protected]>
@connortsui20
connortsui20 force-pushed the ct/arrow-ext-export-guard branch from b490ec1 to 21414d4 Compare November 6, 2026 16:52
@connortsui20
connortsui20 marked this pull request as ready for review November 6, 2026 17:07
@codspeed

codspeed Bot commented Nov 6, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 15.94%

1 warning

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

❌ 1 regressed benchmark
✅ 1916 untouched benchmarks
⏩ 47 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
❌ CPU Simulation ArrowExportVTable_to_arrow_field 38.6 µs 45.9 µs -15.94%

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-ext-export-guard (21414d4) with develop (7854a19)

Open in CodSpeed

Footnotes

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

@robert3005

Copy link
Copy Markdown
Contributor

I think you should do a pass over the comments in the arrow export - map_entries_ignore_a_vortex_idand ArrowSession::executeand session.rs -> DataType::Map path

@connortsui20

Copy link
Copy Markdown
Member Author

@robert3005 Im going to merge this now and do the comment pass in a follow-up PR where we additionally constrain Vortex -> Arrow -> Vortex must roundtrip

@connortsui20
connortsui20 merged commit 1c60cac into develop Nov 9, 2026
121 of 122 checks passed
@connortsui20
connortsui20 deleted the ct/arrow-ext-export-guard branch November 9, 2026 14:56
connortsui20 added a commit that referenced this pull request Nov 9, 2026
## Summary

- Tracking Issue: #11114

Follow-up to #11301, which checks that the Arrow type of a field that
names a Vortex ID imports as the storage of the array. That import can
lose information about nested extension types: the UUID plugin imports
every `arrow.uuid` field without a version, and a session that does not
know a nested Vortex ID imports it as plain storage. So since #11301,
some valid exports fail. For example, an extension type whose storage is
a struct with a `Uuid(v4)` field can no longer export to its own
`to_arrow_storage_field`, which worked before.

## Changes

The check now compares nested extension types by their storage types,
and keeps the exact comparison at the top level. Nested fields keep
their checks, because the exporter checks each nested field when it
reaches it, and the temporal executor still rejects a time unit that
does not match.

The alternative was to require every import plugin to give back exactly
the type that it exported. `ArrowImportVTable` does not require that
today, and a plugin for a canonical Arrow type such as `arrow.uuid` has
no standard place for Vortex-only metadata such as the UUID version.
Exact round trips for the plugins that Vortex owns are still worth
having, but as a separate goal with its own property tests, not as an
assumption of this check.

This PR also does the comment pass from the review of #11301 on the
export dispatch, the map import, and the map test. The map import still
runs import plugins for the extension name of the entries field, which
`from_arrow_datatype` ignores. The comment now says so instead of
claiming that the two match, and a test relies on those plugins running,
so the behavior is unchanged. The metadata check that the import and the
export both copied is now one helper. New tests cover the nested case
(it fails without the fix), an alias of a Vortex ID, and a plain array
that exports to a field that names a Vortex ID.

Signed-off-by: Connor Tsui <[email protected]>
@connortsui20 connortsui20 mentioned this pull request Nov 9, 2026
12 of 49 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/fix A bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants