-
Notifications
You must be signed in to change notification settings - Fork 175
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BUG] Register super extension on to_arrow #3030
Conversation
CodSpeed Performance ReportMerging #3030 will not alter performanceComparing Summary
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3030 +/- ##
==========================================
- Coverage 78.47% 78.47% -0.01%
==========================================
Files 610 610
Lines 71740 71746 +6
==========================================
+ Hits 56297 56301 +4
- Misses 15443 15445 +2
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah shucks yeah, I guess we missed these in the lazy import PR.
Do we have roundtrip tests that would catch this btw?
We do, but they will pass if the modules happen to be lazily imported by earlier test code. |
This is an issue where Daft Extension types were not getting converted to PyArrow properly. @jaychia discovered this while trying to write parquet with a tensor column, where the Extension metadata for tensor was getting dropped.
A simple test to reproduce the error:
Output:
It's not a tensor type! However if you uncomment the
_ensure_registered_super_ext_type()
, you will now see:The issue here is that the
class DaftExtension(pa.ExtensionType):
is not imported during the FFI, as it is now a lazy import that must be called via_ensure_registered_super_ext_type()
.This PR adds calls to this import in
to_arrow
for series and schema. However, I do not know if this is exhaustive, and I will give this more thought. @desmondcheongzx @samster25