Skip to content

Conversation

@sjperkins
Copy link
Contributor

@sjperkins sjperkins commented Jan 20, 2023

Rationale for this change

C++ Extension Types are not correctly exposed in pyarrow

What changes are included in this PR?

__arrow_ext_class__ and __arrow_ext_scalar_class__ have been moved from ExtensionType to BaseExtensionType in types.pxi.

Are these changes tested?

Yes, a test has been added to test_cython.py. There may be better locations for this, but the existing cython testing machinery here is useful for generating a C++ extension type on the fly.

Are there any user-facing changes?

I don't believe there are any user-facing changes as __arrow_ext_class__ and __arrow_ext_scalar_class__ are moved into a base class.

@sjperkins sjperkins requested a review from AlenkaF as a code owner January 20, 2023 09:58
@github-actions
Copy link

@github-actions
Copy link

⚠️ GitHub issue #33801 has been automatically assigned in GitHub to PR creator.

Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for this PR! (and sorry for the slow feedback)

Looking good, added some comments.

@jorisvandenbossche
Copy link
Member

@sjperkins FYI I also opened an issue about having custom subclasses for extension types implemented in C++ (instead of using the base class BaseExtensionType/ExtensionArray): #33997

@sjperkins
Copy link
Contributor Author

@sjperkins FYI I also opened an issue about having custom subclasses for extension types implemented in C++ (instead of using the base class BaseExtensionType/ExtensionArray): #33997

Thanks @jorisvandenbossche for mentioning this, as well as the review. I'm intending to address your review comments next week.

@sjperkins
Copy link
Contributor Author

Is there anything else needed from me on this PR?

Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, nothing else needed, this looks good! (and thanks for the ping :))

@sjperkins
Copy link
Contributor Author

Thanks for the review and merging @jorisvandenbossche

@sjperkins sjperkins deleted the extension-type-fixes branch February 16, 2023 13:58
@ursabot
Copy link

ursabot commented Feb 16, 2023

Benchmark runs are scheduled for baseline = b40fb2c and contender = 1333545. 1333545 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.4% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.26% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.38% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 13335454 ec2-t3-xlarge-us-east-2
[Finished] 13335454 test-mac-arm
[Finished] 13335454 ursa-i9-9960x
[Finished] 13335454 ursa-thinkcentre-m75q
[Finished] b40fb2c7 ec2-t3-xlarge-us-east-2
[Failed] b40fb2c7 test-mac-arm
[Finished] b40fb2c7 ursa-i9-9960x
[Finished] b40fb2c7 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@ursabot
Copy link

ursabot commented Feb 16, 2023

['Python', 'R'] benchmarks have high level of regressions.
test-mac-arm

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.

[Python] C++ Extension Types aren't correctly exposed in pyarrow

3 participants