GH-43454: [C++][Python] Add Opaque canonical extension type by lidavidm · Pull Request #43458 · apache/arrow

lidavidm · 2024-07-28T23:57:20Z

Rationale for this change

Add the newly ratified extension type.

What changes are included in this PR?

The C++/Python implementation only.

Are these changes tested?

Yes

Are there any user-facing changes?

No.

GitHub Issue: [C++][Python] Add Opaque canonical extension type #43454

github-actions · 2024-07-28T23:57:47Z

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

jorisvandenbossche

Copying my review comments from #41823 (review)

For the Python bindings, looks good as well, just a few minor comments:

We probably want to add OpaqueScalar to pyarrow/__init__.py as well? (although I see we forgot this for the tensor type, which I assume you have been imitating)
Can you add the three new classes to test_extension_type_constructor_errors in test_misc.py?
Can you list the new type in the python API docs (docs/source/python/api/datatypes.rst, docs/source/python/api/arrays.rst
)? See eg #39652 for a previous PR adding a new type where this is added to the docs.

python/pyarrow/tests/test_extension_type.py

lidavidm · 2024-08-02T00:59:04Z

I believe I've fixed everything here, thanks for the reviews!

jorisvandenbossche

Haven't looked in detail at the C++ side again, but the Python changes look good!

lidavidm · 2024-08-06T00:39:47Z

Any further comments here?

joellubi · 2024-08-06T10:51:52Z

Any further comments here?

None for me. I based some of my work on Bool8 on the implementation here, so ideally we could get another C++ reviewer to approve as a third party.

pitrou

LGTM, a couple minor comments

pitrou · 2024-08-06T13:50:53Z

cpp/src/arrow/compute/kernels/scalar_cast_numeric.cc

Nit: checked_cast?

pitrou · 2024-08-06T14:04:03Z

cpp/src/arrow/extension/opaque_test.cc

Perhaps also compare with an extension type that is not Opaque?

Co-authored-by: Weston Pace <weston.pace@gmail.com>

lidavidm · 2024-08-14T00:09:33Z

I'm going to merge this in a bit given the radio silence

conbench-apache-arrow · 2024-08-14T15:35:31Z

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 6e7125b.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 38 possible false positives for unstable benchmarks that are known to sometimes produce them.

### Rationale for this change C++ and Python implementations of #43234 ### What changes are included in this PR? - Implement C++ `Bool8Type`, `Bool8Array`, `Bool8Scalar`, and tests - Implement Python bindings to C++, as well as zero-copy numpy conversion methods - TODO: docs waiting for rebase on #43458 ### Are these changes tested? Yes ### Are there any user-facing changes? Bool8 extension type will be available in C++ and Python libraries * GitHub Issue: #17682 Authored-by: Joel Lubinitsky <joellubi@gmail.com> Signed-off-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

rok · 2024-08-21T16:54:58Z

Apologies for the late question. Is opaque type intentionally not registered?

arrow/cpp/src/arrow/extension_type.cc

Lines 145 to 157 in f078942

    
           static void CreateGlobalRegistry() { 
        
             g_registry = std::make_shared<ExtensionTypeRegistryImpl>(); 
        
           #ifdef ARROW_JSON 
        
             // Register canonical extension types 
        
             auto fst_ext_type = 
        
                 checked_pointer_cast<ExtensionType>(extension::fixed_shape_tensor(int64(), {})); 
        
             ARROW_CHECK_OK(g_registry->RegisterType(fst_ext_type)); 
        
             auto bool8_ext_type = checked_pointer_cast<ExtensionType>(extension::bool8()); 
        
             ARROW_CHECK_OK(g_registry->RegisterType(bool8_ext_type)); 
        
           #endif 
        
           }

If it's an oversight I can add registration in #37298.

lidavidm · 2024-08-21T23:40:22Z

It was an oversight, thanks for spotting it

jorisvandenbossche · 2024-08-22T07:38:52Z

If it's an oversight I can add registration in #37298.

Let's do that as a separate PR (it should also require to update the tests slightly, and then it doesn't depend on the timeline of merging the other PR)

jorisvandenbossche · 2024-08-22T07:42:01Z

Opened #43787 to keep track of it.

…3788) This is to resolve #43787 > The Opaque extension type implementation for C++ (plus python bindings) was added in #43458, but it was not registered by default, which we should do for canonical extension types (see #43458 (comment)) Additionally, this adds `bool8` extension type builds with `ARROW_JSON=false` as discussed [here](5258819#r145613657) ### Rationale for this change Canonical types should be registered by default if possible (except e.g. if they can't be compiled due to `ARROW_JSON=false`). ### What changes are included in this PR? This adds default registration for `opaque`, changes when `bool8` is built and moves all canonical tests under the same test target. ### Are these changes tested? Changes are tested by previously existing tests. ### Are there any user-facing changes? `opaue` will now be registered by default and `bool8` will be present in case `ARROW_JSON=false` at build time. * GitHub Issue: #43787 Authored-by: Rok Mihevc <rok@mihevc.org> Signed-off-by: Rok Mihevc <rok@mihevc.org>

…3788) This is to resolve #43787 > The Opaque extension type implementation for C++ (plus python bindings) was added in apache/arrow#43458, but it was not registered by default, which we should do for canonical extension types (see apache/arrow#43458 (comment)) Additionally, this adds `bool8` extension type builds with `ARROW_JSON=false` as discussed [here](apache/arrow@441ecb0#r145613657) ### Rationale for this change Canonical types should be registered by default if possible (except e.g. if they can't be compiled due to `ARROW_JSON=false`). ### What changes are included in this PR? This adds default registration for `opaque`, changes when `bool8` is built and moves all canonical tests under the same test target. ### Are these changes tested? Changes are tested by previously existing tests. ### Are there any user-facing changes? `opaue` will now be registered by default and `bool8` will be present in case `ARROW_JSON=false` at build time. * GitHub Issue: #43787 Authored-by: Rok Mihevc <rok@mihevc.org> Signed-off-by: Rok Mihevc <rok@mihevc.org>

github-actions bot added Component: C++ Component: Python awaiting committer review Awaiting committer review labels Jul 28, 2024

lidavidm force-pushed the gh-43454 branch from 99b9641 to 8219bbe Compare July 28, 2024 23:59

lidavidm marked this pull request as ready for review July 29, 2024 05:12

jorisvandenbossche reviewed Jul 30, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Jul 30, 2024

joellubi reviewed Jul 30, 2024

View reviewed changes

python/pyarrow/tests/test_extension_type.py Outdated Show resolved Hide resolved

lidavidm force-pushed the gh-43454 branch from 8219bbe to 642f77f Compare August 1, 2024 05:37

github-actions bot added Component: Documentation awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Aug 1, 2024

jorisvandenbossche approved these changes Aug 2, 2024

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Aug 2, 2024

joellubi mentioned this pull request Aug 2, 2024

GH-17682: [C++][Python] Bool8 Extension Type Implementation #43488

Merged

pitrou reviewed Aug 6, 2024

View reviewed changes

lidavidm force-pushed the gh-43454 branch from 642f77f to fdeced9 Compare August 7, 2024 08:25

github-actions bot added awaiting changes Awaiting changes and removed awaiting merge Awaiting merge labels Aug 8, 2024

lidavidm and others added 3 commits August 12, 2024 19:29

apacheGH-43454: [C++][Python] Add Opaque canonical extension type

1e3d3ff

Co-authored-by: Weston Pace <weston.pace@gmail.com>

Feedback

d29330a

More feedback

1e364a5

lidavidm force-pushed the gh-43454 branch from fdeced9 to 1e364a5 Compare August 12, 2024 23:29

github-actions bot removed the awaiting changes Awaiting changes label Aug 12, 2024

github-actions bot added the awaiting change review Awaiting change review label Aug 12, 2024

lidavidm merged commit 6e7125b into apache:main Aug 14, 2024

lidavidm removed the awaiting change review Awaiting change review label Aug 14, 2024

lidavidm deleted the gh-43454 branch August 14, 2024 01:41

lidavidm mentioned this pull request Aug 14, 2024

[C++][Python] Add Opaque canonical extension type #43454

Closed

jorisvandenbossche mentioned this pull request Aug 22, 2024

[C++] Register the new Opaque extension type by default #43787

Closed

rok mentioned this pull request Aug 22, 2024

GH-43787: [C++] Register the new Opaque extension type by default #43788

Merged

Conversation

lidavidm commented Jul 28, 2024 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions bot commented Jul 28, 2024

Uh oh!

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lidavidm commented Aug 2, 2024

Uh oh!

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Uh oh!

lidavidm commented Aug 6, 2024

Uh oh!

joellubi commented Aug 6, 2024

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

pitrou Aug 6, 2024

Choose a reason for hiding this comment

Uh oh!

pitrou Aug 6, 2024

Choose a reason for hiding this comment

Uh oh!

lidavidm Aug 8, 2024

Choose a reason for hiding this comment

Uh oh!

lidavidm commented Aug 14, 2024

Uh oh!

conbench-apache-arrow bot commented Aug 14, 2024

Uh oh!

rok commented Aug 21, 2024

Uh oh!

lidavidm commented Aug 21, 2024

Uh oh!

jorisvandenbossche commented Aug 22, 2024

Uh oh!

jorisvandenbossche commented Aug 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lidavidm commented Jul 28, 2024 •

edited by github-actions bot

Loading