Skip to content
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

[Python] Repr of Timestamp scalars error if they are outside range of stdlib datetime.datetime #36323

Closed
jorisvandenbossche opened this issue Jun 27, 2023 · 4 comments · Fixed by #36942

Comments

@jorisvandenbossche
Copy link
Member

Describe the bug, including details regarding any error messages, version, and platform.

Currently, the repr of the TimestampScalar (well, of scalars in general) essentially uses the repr of the Python object. For TimestampScalar, this thus fails if this conversion fails, such as for timestamps that are outside of the range of datetime.datetime ([1-9999]):

>>> res = pa.scalar("0000-01-01").cast(pa.timestamp("s"))
>>>type(res)
pyarrow.lib.TimestampScalar
>>> repr(res)
...
File ~/miniconda3/envs/dev39/lib/python3.9/site-packages/pyarrow/scalar.pxi:117, in pyarrow.lib.Scalar.__repr__()

File ~/miniconda3/envs/dev39/lib/python3.9/site-packages/pyarrow/scalar.pxi:521, in pyarrow.lib.TimestampScalar.as_py()

File ~/miniconda3/envs/dev39/lib/python3.9/site-packages/pyarrow/scalar.pxi:437, in pyarrow.lib._datetime_from_int()

OverflowError: date value out of range

>>> pc.strftime(res)
<pyarrow.StringScalar: '0000-01-01T00:00:00'>

Our generic Scalar __repr__ implementation:

def __repr__(self):
return '<pyarrow.{}: {!r}>'.format(
self.__class__.__name__, self.as_py()
)

I think for Timestamps we should just convert to a string repr ourselves (strftime, or our own pretty printer as is used for the Array repr)

Component(s)

Python

@aboss123
Copy link
Contributor

Hey, I'd like to work on this issue.

@anjakefala
Copy link
Collaborator

You can write 'take' into a comment body to assign yourself =)

I will add this to the documentation: https://arrow.apache.org/docs/dev/developers/guide/step_by_step/finding_issues.html

@aboss123
Copy link
Contributor

take

@raulcd
Copy link
Member

raulcd commented Jul 7, 2023

@jorisvandenbossche should this be a blocker for 13.0.0 or can I move it to 14.0.0?

@jorisvandenbossche jorisvandenbossche modified the milestones: 13.0.0, 14.0.0 Jul 7, 2023
kou pushed a commit that referenced this issue Jul 31, 2023
…datetime range (#36942)

### Rationale for this change

#36323

### What changes are included in this PR?

Changed the way repr is handled for TimestampScalar

### Are these changes tested?

I have added a very basic test for this change to see whether it will error or not if outside the range.

### Are there any user-facing changes?

The functionality of TimestampScalar's repr now uses the `strftime` function.

* Closes: #36323

Lead-authored-by: Ashish Bailkeri <ashishbailkeri123@gmail.com>
Co-authored-by: Ashish Bailkeri <47304318+aboss123@users.noreply.github.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
R-JunmingChen pushed a commit to R-JunmingChen/arrow that referenced this issue Aug 20, 2023
…tside datetime range (apache#36942)

### Rationale for this change

apache#36323

### What changes are included in this PR?

Changed the way repr is handled for TimestampScalar

### Are these changes tested?

I have added a very basic test for this change to see whether it will error or not if outside the range.

### Are there any user-facing changes?

The functionality of TimestampScalar's repr now uses the `strftime` function.

* Closes: apache#36323

Lead-authored-by: Ashish Bailkeri <ashishbailkeri123@gmail.com>
Co-authored-by: Ashish Bailkeri <47304318+aboss123@users.noreply.github.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
loicalleyne pushed a commit to loicalleyne/arrow that referenced this issue Nov 13, 2023
…tside datetime range (apache#36942)

### Rationale for this change

apache#36323

### What changes are included in this PR?

Changed the way repr is handled for TimestampScalar

### Are these changes tested?

I have added a very basic test for this change to see whether it will error or not if outside the range.

### Are there any user-facing changes?

The functionality of TimestampScalar's repr now uses the `strftime` function.

* Closes: apache#36323

Lead-authored-by: Ashish Bailkeri <ashishbailkeri123@gmail.com>
Co-authored-by: Ashish Bailkeri <47304318+aboss123@users.noreply.github.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment