Repository navigation
Check the Arrow type of a Vortex ID export target - #11301
Conversation
21038a8 to
4820228
Compare
4820228 to
0679fa4
Compare
|
actually I think we can make a better abstraction |
574134e to
b490ec1
Compare
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]>
b490ec1 to
21414d4
Compare
Merging this PR will degrade performance by 15.94%1 warning
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing Footnotes
|
|
I think you should do a pass over the comments in the arrow export - |
|
@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 |
## 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]>
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 namesvortex.timestampcomes 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
Vectorpass 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