ARROW-15545: [Python][C++] Support casting to extension type #14106

milesgranger · 2022-09-13T08:11:32Z

github-actions · 2022-09-13T09:53:54Z

https://issues.apache.org/jira/browse/ARROW-15545

jorisvandenbossche · 2022-09-13T10:28:08Z

I think ideally we handle this on the C++ side, so that it for example also works within a Acero query / dataset scan.

rok · 2022-09-13T12:01:47Z

This would indeed be useful in c++ too. Reading scalar_cast_numeric.cc and scalar_cast_test.cc might be a good way to get familiar with casting there.

milesgranger · 2022-09-13T13:02:07Z

Thanks @rok, just saw your comment, I'll take a deeper look at that. Now in b9d8cb6 I loop over id or storage id. This creates the same result as the Python casting in e4db00b which fixes the reported issue (dictionary casting ends up as the ExtensionArray UuidType). However, when casting the test's IntegerType it comes back as Int64Array instead of extension array.. (maybe that's okay?)

rok · 2022-09-13T13:26:21Z

@milesgranger My guess is you will want to instantiate an ExtensionArray from a 'regular' Array by passing the underlaying buffers, kind of like what CastFromExtension does and then add c++ test like this one.

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

milesgranger · 2022-09-19T09:56:42Z

@pitrou (and others already subscribed), this is ready for a look now. Couple things I'm not sure about is if I'm adding the supported types in GetCastToExtension correctly and if I should better the C++ tests to cover more type varieties like the Python tests do.

pitrou

Thanks @milesgranger . Some comments below.

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

cpp/src/arrow/compute/kernels/scalar_cast_internal.h

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

pitrou · 2022-09-19T15:47:32Z

Also cc @lidavidm for potential opinions.

lidavidm

Don't think I have much to add to Antoine's comments here.

It may be good to add an entry to "Generic conversions" in the docs: https://github.com/apache/arrow/blob/master/docs/source/cpp/compute.rst#conversions

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

[skip ci] Co-authored-by: Antoine Pitrou <pitrou@free.fr>

docs/source/cpp/compute.rst

jorisvandenbossche

Looks good!

I think the main unresolved issue is whether we want to allow extension->extension casts: #14106 (comment)

Personally, I think this would be useful, although I certainly agree that the cast will often not make much sense (depending on the extension type that is being used). But, I think that is just as true for any other cast to/from extension type in case it is not casting from/to the exact storage type (for example, we allow casting timestamp to int, but that doesn't mean that a cast from timestamp to extension-type-with-int-storage would make sense).

jorisvandenbossche · 2022-09-27T14:24:26Z

I think the main unresolved issue is whether we want to allow extension->extension casts: #14106 (comment)

Personally, I think this would be useful, although I certainly agree that the cast will often not make much sense (depending on the extension type that is being used). But, I think that is just as true for any other cast to/from extension type in case it is not casting from/to the exact storage type (for example, we allow casting timestamp to int, but that doesn't mean that a cast from timestamp to extension-type-with-int-storage would make sense).

Continuing on this myself: We could also restrict it (by default) to only casts from/to exactly the storage type? And allow the other casts behind a flag (like allow_non_storage_extension_casts ..., not sure I like that myself though)

@pitrou you mentioned(#14106 (comment)):

Being able to register extension-specific casts would be another separate possibility.

In general that sounds interesting. Especially if that would also allow to change the behaviour of a specific cast (and not just allowing a pre-defined cast to/from the storage type). For example casting to string could give some actual string representation instead of just the string representation of the storage.
But at the same time this also sounds quite complicated for a maybe limited use case?

lidavidm · 2022-09-27T14:25:49Z

In general that sounds interesting. Especially if that would also allow to change the behaviour of a specific cast (and not just allowing a pre-defined cast to/from the storage type). For example casting to string could give some actual string representation instead of just the string representation of the storage.
But at the same time this also sounds quite complicated for a maybe limited use case?

Cast-to-string is how things like the CSV writer work, right? So maybe the use case isn't all that limited?

mavam · 2022-09-29T05:59:37Z

Glad to see this being worked on!

I'm currently running in the not-yet-implemented error, per https://issues.apache.org/jira/browse/ARROW-17839, where I have a nested struct with an extension type.

jorisvandenbossche · 2022-09-29T07:31:38Z

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

+  if (array->type()->id() == Type::EXTENSION) {
+    const auto& ext_arr = checked_cast<const ExtensionArray&>(*array);
+    if (out_ty->id() != ext_arr.extension_type()->storage_id()) {


My interpretation was to (for now) disallow any cast unless the input array has exactly a type equal to the target storage type (in which case basically no actual cast is needed, only the creation of the extension array ..)

So something like if (!array->type()->Equals(out_ty)) { ...

(the error message you now wrote would cover that as well)

Ah okay.. then it's updated, and also does not cast unless it's needed.

For future reference, Miles and I chatted about this and were thinking to keep the above, and thus only disallow Extension->Extension casts for now in this PR, while still allow Any->Extension casts based on the storage type casting rules (so if Any can be cast to storage type, then it can also be cast to the Extension type).

This would make it consistent with the existing Extension->Any cast, which also just defers to the casting rules of the storage type at the moment (and does not limit to strictly casting Extension only to the exact storage type). And I think this is also what others were OK with (based on the inline discussion in #14106 (comment))

Personally I still think it's a bit inconsistent to allow casting any unrelated type to ExtensionType, but not allow an unrelated extension array to ExtensionType. But we can see to allow registering such casts in the follow-up https://issues.apache.org/jira/browse/ARROW-17890

Implicit conversions always have to end up inconsistent in practice, so this is not a problem.

For example, in Python you can convert a dict to bool, a bool to float, but not a dict to float.

Yes, but you can use that analogy for the "any -> extension" casts as well (for the case where "any" is not equal to "storage"), that the user needs to do "any -> storage -> extension" explicitly instead .

Yes, but you can use that analogy for the "any -> extension" casts as well (for the case where "any" is not equal to "storage"), that the user needs to do "any -> storage -> extension" explicitly instead .

Well, I wouldn't mind this :-)

[skip ci]

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

jorisvandenbossche · 2022-09-29T11:21:09Z

I opened a follow-up JIRA about allowing more casts / ability to register casts -> https://issues.apache.org/jira/browse/ARROW-17890

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

docs/source/cpp/compute.rst

python/pyarrow/tests/test_extension_type.py

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

milesgranger · 2022-10-07T09:12:23Z

Is there anything else we ought to address?
I don't think the two failing jobs are related.

jorisvandenbossche

Thanks for the last updates @milesgranger !

Planning to merge this later today (the failures seem unrelated)

jorisvandenbossche · 2022-10-11T08:16:23Z

Thanks @milesgranger !

ursabot · 2022-10-11T23:53:52Z

Benchmark runs are scheduled for baseline = 5fb02bd and contender = 06c99f7. 06c99f7 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 ⬇️1.11% ⬆️0.0%] test-mac-arm
[Failed ⬇️0.82% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.32% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 06c99f7a ec2-t3-xlarge-us-east-2
[Failed] 06c99f7a test-mac-arm
[Failed] 06c99f7a ursa-i9-9960x
[Finished] 06c99f7a ursa-thinkcentre-m75q
[Finished] 5fb02bdb ec2-t3-xlarge-us-east-2
[Failed] 5fb02bdb test-mac-arm
[Failed] 5fb02bdb ursa-i9-9960x
[Finished] 5fb02bdb 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 · 2022-10-11T23:54:10Z

['Python', 'R'] benchmarks have high level of regressions.
ursa-i9-9960x

jorisvandenbossche · 2022-10-12T07:15:43Z

FWIW the regressions all seems to be flaky runs (they didn't continue to be slower on the subsequent merged commits)

Support casting to extension type

e4db00b

github-actions bot added the Component: Python label Sep 13, 2022

Move extension casting impl from Python to C++

b9d8cb6

milesgranger changed the title ~~ARROW-15545: [Python] Support casting to extension type~~ ARROW-15545: [Python][C++] Support casting to extension type Sep 13, 2022

github-actions bot added the Component: C++ label Sep 13, 2022

Attempt impl CastToExtension et al. [skip ci]

89aa16c

milesgranger commented Sep 15, 2022

View reviewed changes

cpp/src/arrow/compute/kernels/scalar_cast_numeric.cc Outdated Show resolved Hide resolved

rok reviewed Sep 15, 2022

View reviewed changes

cpp/src/arrow/compute/kernels/scalar_cast_numeric.cc Outdated Show resolved Hide resolved

Works with hard-coded extension type [skip ci]

444fd02

milesgranger commented Sep 16, 2022

View reviewed changes

cpp/src/arrow/compute/kernels/scalar_cast_numeric.cc Outdated Show resolved Hide resolved

milesgranger added 3 commits September 16, 2022 14:37

Static cast to extension type [skip ci]

6c05a1d

Move to scalar_cast_extension, need C++ tests [skip ci]

296b4fb

Impl initial cpp test for casting to extension

120204b

pitrou requested changes Sep 19, 2022

View reviewed changes

lidavidm reviewed Sep 19, 2022

View reviewed changes

cpp/src/arrow/compute/kernels/scalar_cast_extension.cc Outdated Show resolved Hide resolved

milesgranger and others added 6 commits September 21, 2022 08:43

Update cpp/src/arrow/compute/kernels/scalar_cast_test.cc

34c9998

[skip ci] Co-authored-by: Antoine Pitrou <pitrou@free.fr>

Move all but GetExtensionCasts to anonymous namespace [skip ci]

20be1fe

Remove CastToExtension from header file [skip ci]

c7e76c7

Use ARROW_ASSIGN_OR_RAISE during cast [skip ci]

4cd9b65

Updates tests from review comments [skip ci]

2d18904

Add entry to compute.rst docs

7e8d795

lidavidm reviewed Sep 21, 2022

View reviewed changes

docs/source/cpp/compute.rst Outdated Show resolved Hide resolved

Move compute.rst docs entry to generic conversions table

069bfd0

jorisvandenbossche approved these changes Sep 27, 2022

View reviewed changes

mavam mentioned this pull request Sep 29, 2022

Implement VAST extension types in Python tenzir/tenzir#2596

Merged

8 tasks

jorisvandenbossche reviewed Sep 29, 2022

View reviewed changes

milesgranger added 3 commits September 29, 2022 09:33

Fail cast between incompatible storage types [skip ci]

02013e8

Only convert to ext where input type is same as storage type

da66bf4

[skip ci]

ARROW_EXPORT for AllTypeIds

374301c

jorisvandenbossche reviewed Sep 29, 2022

View reviewed changes

cpp/src/arrow/compute/kernels/scalar_cast_extension.cc Show resolved Hide resolved

jorisvandenbossche reviewed Sep 30, 2022

View reviewed changes

cpp/src/arrow/compute/kernels/scalar_cast_extension.cc Outdated Show resolved Hide resolved

Move result assignments inside if branches

44cef44

jorisvandenbossche reviewed Oct 4, 2022

View reviewed changes

milesgranger added 3 commits October 4, 2022 15:35

Update error msg and test casting between same ext types

8e5063f

Update compute.rst doc notes

2bc8a83

Fixup: autopep8 format

2cef2ae

Add test for casting to extension w/ extension storage

135c8a0

jorisvandenbossche approved these changes Oct 11, 2022

View reviewed changes

pitrou approved these changes Oct 11, 2022

View reviewed changes

jorisvandenbossche merged commit 06c99f7 into apache:master Oct 11, 2022

milesgranger deleted the ARROW-15545_cast-of-extension-types branch October 11, 2022 08:49

mavam mentioned this pull request Oct 29, 2022

Finalize extension types after Arrow 10 fix tenzir/tenzir#2676

Merged

1 task

This was referenced Oct 11, 2022

[C++] Support casting from storage type to extension type #30057

Closed

[C++] Cast dictionary of extension type to extension type #31015

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-15545: [Python][C++] Support casting to extension type #14106

ARROW-15545: [Python][C++] Support casting to extension type #14106

milesgranger commented Sep 13, 2022 •

edited by jorisvandenbossche

Loading

github-actions bot commented Sep 13, 2022

jorisvandenbossche commented Sep 13, 2022

rok commented Sep 13, 2022

milesgranger commented Sep 13, 2022 •

edited

Loading

rok commented Sep 13, 2022

milesgranger commented Sep 19, 2022

pitrou left a comment

pitrou commented Sep 19, 2022

lidavidm left a comment

jorisvandenbossche left a comment

jorisvandenbossche commented Sep 27, 2022

lidavidm commented Sep 27, 2022

mavam commented Sep 29, 2022

jorisvandenbossche Sep 29, 2022 •

edited

Loading

milesgranger Sep 29, 2022

jorisvandenbossche Oct 4, 2022

pitrou Oct 4, 2022

jorisvandenbossche Oct 4, 2022

pitrou Oct 4, 2022

jorisvandenbossche commented Sep 29, 2022

milesgranger commented Oct 7, 2022

jorisvandenbossche left a comment

jorisvandenbossche commented Oct 11, 2022

ursabot commented Oct 11, 2022

ursabot commented Oct 11, 2022

jorisvandenbossche commented Oct 12, 2022

ARROW-15545: [Python][C++] Support casting to extension type #14106

ARROW-15545: [Python][C++] Support casting to extension type #14106

Conversation

milesgranger commented Sep 13, 2022 • edited by jorisvandenbossche Loading

github-actions bot commented Sep 13, 2022

jorisvandenbossche commented Sep 13, 2022

rok commented Sep 13, 2022

milesgranger commented Sep 13, 2022 • edited Loading

rok commented Sep 13, 2022

milesgranger commented Sep 19, 2022

pitrou left a comment

Choose a reason for hiding this comment

pitrou commented Sep 19, 2022

lidavidm left a comment

Choose a reason for hiding this comment

jorisvandenbossche left a comment

Choose a reason for hiding this comment

jorisvandenbossche commented Sep 27, 2022

lidavidm commented Sep 27, 2022

mavam commented Sep 29, 2022

jorisvandenbossche Sep 29, 2022 • edited Loading

Choose a reason for hiding this comment

milesgranger Sep 29, 2022

Choose a reason for hiding this comment

jorisvandenbossche Oct 4, 2022

Choose a reason for hiding this comment

pitrou Oct 4, 2022

Choose a reason for hiding this comment

jorisvandenbossche Oct 4, 2022

Choose a reason for hiding this comment

pitrou Oct 4, 2022

Choose a reason for hiding this comment

jorisvandenbossche commented Sep 29, 2022

milesgranger commented Oct 7, 2022

jorisvandenbossche left a comment

Choose a reason for hiding this comment

jorisvandenbossche commented Oct 11, 2022

ursabot commented Oct 11, 2022

ursabot commented Oct 11, 2022

jorisvandenbossche commented Oct 12, 2022

milesgranger commented Sep 13, 2022 •

edited by jorisvandenbossche

Loading

milesgranger commented Sep 13, 2022 •

edited

Loading

jorisvandenbossche Sep 29, 2022 •

edited

Loading