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

[R] Possible undesirable handling of POSIXlt objects #33506

Closed
asfimport opened this issue Nov 16, 2022 · 4 comments
Closed

[R] Possible undesirable handling of POSIXlt objects #33506

asfimport opened this issue Nov 16, 2022 · 4 comments

Comments

@asfimport
Copy link

In the course of updating documentation, I noticed that it is possible to create an Arrow array of POSIXlt objects from R, but not a scalar.

#14514 (comment)

This works:

tm <- as.POSIXlt(c(Sys.time(), Sys.time()))
arrow::Array$create(tm)

This fails:

arrow::Scalar$create(as.POSIXlt(Sys.time()))

It's possible to manually convert a POSIXlt object to a struct scalar like this:

df <- as.data.frame(unclass(as.POSIXlt(Sys.time())))
arrow::Scalar$create(df, 
              type = struct(
                sec = float32(), 
                min = int32(),
                hour = int32(),
                mday = int32(),
                mon = int32(),
                year = int32(),
                wday = int32(),
                yday = int32(),
                isdst = int32(),
                zone = utf8(),
                gmtoff = int32()
              ))

although this does not seem precisely the same as the behaviour of Array$create() which creates an extension type?

It was unclear to us ( @thisisnic and myself) whether the current behaviour was desirable, so it seemed sensible to open an issue!

Related issue:

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

Reporter: Danielle Navarro / @djnavarro
Assignee: Dewey Dunnington / @paleolimbot

PRs and other links:

Note: This issue was originally created as ARROW-18337. Please see the migration documentation for further details.

@asfimport
Copy link
Author

Dewey Dunnington / @paleolimbot:
Oooof...this is particularly bad since it causes a crash (for me):

/Users/dewey/Desktop/rscratch/arrow/cpp/src/arrow/result.cc:28: ValueOrDie called on an error: NotImplemented: construction from scalar of type POSIXlt of length 0

The error seems to be coming from the ValueOrDie here: https://github.com/apache/arrow/blob/master/cpp/src/arrow/scalar.cc#L785-L803

I tried a rudimentary approach of an RExtensionScalar subclass but didn't get anywhere because Scalar::ToString() is not virtual (i.e., I can't provide a more reasonable method without exposing an additional R6 class at the R level).

@asfimport
Copy link
Author

Dewey Dunnington / @paleolimbot:
The backtrace of the ValueOrDie is:

/Users/dewey/Desktop/rscratch/arrow/cpp/src/arrow/result.cc:28: ValueOrDie called on an error: NotImplemented: construction from scalar of type POSIXlt of length 0
/Users/dewey/Desktop/rscratch/arrow/cpp/src/arrow/array/util.cc:545  VisitTypeInline(*scalar_.type, this)
0   libarrow.1100.0.0.dylib             0x000000011b3a9d0c _ZN5arrow4util7CerrLogD2Ev + 236
1   libarrow.1100.0.0.dylib             0x000000011b3a9c18 _ZN5arrow4util7CerrLogD0Ev + 12
2   libarrow.1100.0.0.dylib             0x000000011b3a9bb4 _ZN5arrow4util8ArrowLogD1Ev + 48
3   libarrow.1100.0.0.dylib             0x000000011b2862a0 _ZN5arrow8internal17InvalidValueOrDieERKNS_6StatusE + 244
4   libarrow.1100.0.0.dylib             0x000000011b28a0d8 _ZNK5arrow6Scalar8ToStringEv + 792
5   arrow.so                            0x000000010c064fac _arrow_Scalar__ToString + 108
6   libR.dylib                          0x0000000102f89be4 R_doDotCall + 1572

@asfimport asfimport added this to the 11.0.0 milestone Jan 11, 2023
@raulcd
Copy link
Member

raulcd commented Jan 11, 2023

This has been merged pointing to the original issue. I am closing the issue manually. Should we update the merge script to not allow merging issues pointing to JIRA so we have to update the titles? I already created a PR to update the title check on the PR: #33611

@raulcd raulcd closed this as completed Jan 11, 2023
@assignUser
Copy link
Member

@raulcd yes I think that makes sense

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

No branches or pull requests

4 participants