Skip to content

Conversation

@danepitkin
Copy link
Member

@danepitkin danepitkin commented May 2, 2023

Rationale for this change

Scalar cast should use the computer kernel just like Arrays, instead of its own custom implementation.

Are these changes tested?

Added test cases for GH-35370, GH-34901, and GH-35040

Are there any user-facing changes?

The Scalar.cast() API is enhanced and backwards compatible.

@danepitkin danepitkin requested a review from AlenkaF as a code owner May 2, 2023 21:13
@github-actions
Copy link

github-actions bot commented May 2, 2023

@github-actions
Copy link

github-actions bot commented May 2, 2023

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

@danepitkin
Copy link
Member Author

Build error is unrelated. The error exists on main (and in other PRs).

Copy link
Member

@AlenkaF AlenkaF left a comment

Choose a reason for hiding this comment

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

LGTM +1

Just a small nit though: I would also add a test for casting float to int, using default safe option and an unsafe casting option. Not sure about the memory pool though.

Yes, the failing tests are not related, I have created an issue for it: #35490

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels May 8, 2023
@danepitkin
Copy link
Member Author

LGTM +1

Just a small nit though: I would also add a test for casting float to int, using default safe option and an unsafe casting option. Not sure about the memory pool though.

Yes, the failing tests are not related, I have created an issue for it: #35490

Great idea! I will add this test.

@danepitkin danepitkin force-pushed the danepitkin/gh-35040 branch from 23f54a8 to 689fe59 Compare May 10, 2023 20:46
@AlenkaF
Copy link
Member

AlenkaF commented May 11, 2023

The failure in the MacOS build could now be corrected with a rebase.

@danepitkin danepitkin force-pushed the danepitkin/gh-35040 branch from 689fe59 to 8de3749 Compare May 11, 2023 16:04
@AlenkaF
Copy link
Member

AlenkaF commented May 11, 2023

Thank you for the update Dane!
I think this is ready, will merge 👍

@AlenkaF AlenkaF changed the title GH-35040: [Python] Pyarrow scalar cast uses compute kernel GH-35040: [Python] Pyarrow scalar cast should use compute kernel May 11, 2023
@AlenkaF AlenkaF merged commit 053b5ee into apache:main May 11, 2023
@danepitkin
Copy link
Member Author

danepitkin commented May 11, 2023

We can also close #34901 and #35370 now!

I'll take care of this.

@ursabot
Copy link

ursabot commented May 13, 2023

Benchmark runs are scheduled for baseline = e7a885d and contender = 053b5ee. 053b5ee 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
[Finished ⬇️2.15% ⬆️0.0%] test-mac-arm
[Finished ⬇️1.27% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️1.72% ⬆️0.27%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 053b5eec ec2-t3-xlarge-us-east-2
[Finished] 053b5eec test-mac-arm
[Finished] 053b5eec ursa-i9-9960x
[Finished] 053b5eec ursa-thinkcentre-m75q
[Finished] e7a885d8 ec2-t3-xlarge-us-east-2
[Finished] e7a885d8 test-mac-arm
[Finished] e7a885d8 ursa-i9-9960x
[Finished] e7a885d8 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

ArgusLi pushed a commit to Bit-Quill/arrow that referenced this pull request May 15, 2023
apache#35395)

### Rationale for this change

Scalar cast should use the computer kernel just like Arrays, instead of its own custom implementation.

### Are these changes tested?

Added test cases for apacheGH-35370, apacheGH-34901, and apacheGH-35040

### Are there any user-facing changes?

The Scalar.cast() API is enhanced and backwards compatible. 
* Closes: apache#35040

Authored-by: Dane Pitkin <[email protected]>
Signed-off-by: Alenka Frim <[email protected]>
jorisvandenbossche added a commit that referenced this pull request Jun 6, 2023
…use it requires tz database (#35735)

### Rationale for this change

Fix up of #35395, skipping one of the tests added in that PR on Windows, because the test requires access to a tz database.

Authored-by: Joris Van den Bossche <[email protected]>
Signed-off-by: Joris Van den Bossche <[email protected]>
@mroeschke
Copy link
Contributor

xref #36677

I don't think the new safe keyword fully covers past cases where numeric scalars could be cast to temporal scalars

>>> import pyarrow as pa
>>> pa.scalar(1.1).cast(pa.time32("s"))
<pyarrow.Time32Scalar: datetime.time(0, 0, 1)>
>>> pa.__version__
'12.0.1'

>>> import pyarrow as pa
>>> pa.scalar(1.1).cast(pa.time32("s"), safe=False)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "pyarrow/scalar.pxi", line 91, in pyarrow.lib.Scalar.cast
  File "/opt/miniconda3/envs/pyarrow-pandas-dev/lib/python3.11/site-packages/pyarrow/compute.py", line 403, in cast
    return call_function("cast", [arr], options, memory_pool)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "pyarrow/_compute.pyx", line 572, in pyarrow._compute.call_function
  File "pyarrow/_compute.pyx", line 367, in pyarrow._compute.Function.call
  File "pyarrow/error.pxi", line 144, in pyarrow.lib.pyarrow_internal_check_status
  File "pyarrow/error.pxi", line 121, in pyarrow.lib.check_status
pyarrow.lib.ArrowNotImplementedError: Unsupported cast from double to time32 using function cast_time32
>>> pa.__version__
'13.0.0.dev497'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[C++] Change Scalar::CastTo to do safe cast by default and allow to specify cast options ?

4 participants