[GitHub] [arrow] jorisvandenbossche commented on a change in pull request #8559: ARROW-10446: [C++][Python] Roundtrip Timestamp ns with TzInfo correctly

[GitBox <gi...@apache.org>]

jorisvandenbossche commented on a change in pull request #8559:
URL: https://github.com/apache/arrow/pull/8559#discussion_r516069013



##########
File path: python/pyarrow/tests/test_convert_builtin.py
##########
@@ -2000,6 +2000,17 @@ def test_roundtrip_nanosecond_resolution_pandas_temporal_objects():
         pd.Timestamp(9223371273709551616, unit='ns')
     ]
 
+    ty = pa.timestamp('ns', tz='US/Eastern')
+    value = 1604119893000000000
+    arr = pa.array([value], type=ty)
+    data = arr.to_pylist()
+    assert isinstance(data[0], pd.Timestamp)
+    restored = pa.array(data, type=ty)
+    assert arr.equals(restored)
+    assert restored.to_pylist() == [
+        pd.Timestamp(value, unit='ns', tz='US/Eastern')

Review comment:
       ```suggestion
           pd.Timestamp(value, unit='ns').tz_localize("UTC").tz_convert('US/Eastern')
   ```
   
   (it's the same, but I find it a bit more explicit, otherwise you need to know if pd.Timestamp interprets the integer as wall time or UTC time)

##########
File path: cpp/src/arrow/python/python_to_arrow.cc
##########
@@ -272,11 +272,13 @@ class PyValue {
               return internal::InvalidValue(obj,

Review comment:
       Can you add a comment in the pandas timestamp case a few lines above about that case not needing to apply to subtract the offset, because pandas timestamps `value` is already nanoseconds in UTC (in contrast to datetime.datetime's microseconds)




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org