Repository navigation
Compare nested extension types by storage in Vortex ID exports - #11351
Conversation
Signed-off-by: Connor Tsui <[email protected]>
Merging this PR will improve performance by 10.48%2 warnings
Performance Changes
Tip Curious why performance improved? Comment Comparing 
Footnotes
|
| /// Returns whether `lhs` and `rhs` have the same storage layout, which means that they are equal, | ||
| /// ignoring nullability, after every nested extension type is replaced by its storage type. The | ||
| /// types at the top level are compared as they are. | ||
| /// | ||
| /// An export compares the type that its target imports as with the storage type this way, because | ||
| /// an import can lose the metadata of a nested extension type, such as the version of a UUID, or | ||
| /// not know its Vortex ID. The export of each nested field checks that field when it reaches it. | ||
| fn eq_storage_layout(lhs: &DType, rhs: &DType) -> bool { |
There was a problem hiding this comment.
I wonder if we should put this somewhere else? maybe not because I can't think of any other code that should know this
There was a problem hiding this comment.
I think we can move it once we have another use for it, right now this is very arrow specific
Summary
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.uuidfield 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 aUuid(v4)field can no longer export to its ownto_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.
ArrowImportVTabledoes not require that today, and a plugin for a canonical Arrow type such asarrow.uuidhas 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_datatypeignores. 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.