You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/10/05 19:39:45 UTC
[GitHub] [arrow] kszucs opened a new pull request #8349: ARROW-3080: [Python] Unify Arrow to Python object conversion paths
kszucs opened a new pull request #8349:
URL: https://github.com/apache/arrow/pull/8349
----------------------------------------------------------------
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
[GitHub] [arrow] kszucs commented on a change in pull request #8349: ARROW-3080: [Python] Unify Arrow to Python object conversion paths
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #8349:
URL: https://github.com/apache/arrow/pull/8349#discussion_r502490505
##########
File path: python/pyarrow/tests/test_convert_builtin.py
##########
@@ -1903,18 +1946,40 @@ def test_dictionary_from_strings():
assert a.dictionary.equals(expected_dictionary)
-def _has_unique_field_names(ty):
- if isinstance(ty, pa.StructType):
- field_names = [field.name for field in ty]
- return len(set(field_names)) == len(field_names)
- else:
- return True
+@pytest.mark.parametrize('unit', ['s', 'ms', 'us', 'ns'])
+def test_duration_array_roundtrip_corner_cases(unit):
+ # Corner case discovered by hypothesis: there were implicit conversions to
+ # unsigned values resulting wrong values with wrong signs.
+ ty = pa.duration(unit)
+ arr = pa.array([-2147483000], type=ty)
+ restored = pa.array(arr.to_pylist(), type=ty)
Review comment:
Updated.
##########
File path: python/pyarrow/tests/test_convert_builtin.py
##########
@@ -1903,18 +1946,40 @@ def test_dictionary_from_strings():
assert a.dictionary.equals(expected_dictionary)
-def _has_unique_field_names(ty):
- if isinstance(ty, pa.StructType):
- field_names = [field.name for field in ty]
- return len(set(field_names)) == len(field_names)
- else:
- return True
+@pytest.mark.parametrize('unit', ['s', 'ms', 'us', 'ns'])
+def test_duration_array_roundtrip_corner_cases(unit):
+ # Corner case discovered by hypothesis: there were implicit conversions to
+ # unsigned values resulting wrong values with wrong signs.
+ ty = pa.duration(unit)
+ arr = pa.array([-2147483000], type=ty)
+ restored = pa.array(arr.to_pylist(), type=ty)
+ assert arr.equals(restored)
+
+
+@pytest.mark.pandas
+def test_roundtrip_nanosecond_resolution_pandas_temporal_objects():
+ # corner case discovered by hypothesis: preserving the nanoseconds on
+ # conversion from a list of Timedelta and Timestamp objects
+ import pandas as pd
+
+ ty = pa.duration('ns')
+ arr = pa.array([9223371273709551616], type=ty)
+ data = arr.to_pylist()
+ assert isinstance(data[0], pd.Timedelta)
+ restored = pa.array(data, type=ty)
Review comment:
Updated.
----------------------------------------------------------------
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
[GitHub] [arrow] pitrou commented on a change in pull request #8349: ARROW-3080: [Python] Unify Arrow to Python object conversion paths
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8349:
URL: https://github.com/apache/arrow/pull/8349#discussion_r501762533
##########
File path: cpp/src/arrow/python/helpers.cc
##########
@@ -258,30 +266,46 @@ bool PyFloat_IsNaN(PyObject* obj) {
namespace {
static std::once_flag pandas_static_initialized;
-static PyTypeObject* pandas_NaTType = nullptr;
+
static PyObject* pandas_NA = nullptr;
+static PyObject* pandas_NaT = nullptr;
+static PyObject* pandas_Timedelta = nullptr;
+static PyObject* pandas_Timestamp = nullptr;
+static PyTypeObject* pandas_NaTType = nullptr;
void GetPandasStaticSymbols() {
OwnedRef pandas;
+
+ // import pandas
Status s = ImportModule("pandas", &pandas);
if (!s.ok()) {
return;
}
OwnedRef ref;
- s = ImportFromModule(pandas.obj(), "NaT", &ref);
- if (!s.ok()) {
- return;
+
+ // set NaT sentinel and its type
+ if (ImportFromModule(pandas.obj(), "NaT", &ref).ok()) {
+ pandas_NaT = ref.obj();
+ // PyObject_Type returns a new reference but we trust that pandas.NaT will
Review comment:
You can just use the `Py_TYPE` macro then: https://docs.python.org/3/c-api/structures.html#c.Py_TYPE
##########
File path: python/pyarrow/tests/test_convert_builtin.py
##########
@@ -1903,18 +1946,40 @@ def test_dictionary_from_strings():
assert a.dictionary.equals(expected_dictionary)
-def _has_unique_field_names(ty):
- if isinstance(ty, pa.StructType):
- field_names = [field.name for field in ty]
- return len(set(field_names)) == len(field_names)
- else:
- return True
+@pytest.mark.parametrize('unit', ['s', 'ms', 'us', 'ns'])
+def test_duration_array_roundtrip_corner_cases(unit):
+ # Corner case discovered by hypothesis: there were implicit conversions to
+ # unsigned values resulting wrong values with wrong signs.
+ ty = pa.duration(unit)
+ arr = pa.array([-2147483000], type=ty)
+ restored = pa.array(arr.to_pylist(), type=ty)
Review comment:
Shouldn't you also test the actual value of `restored`?
##########
File path: python/pyarrow/tests/test_convert_builtin.py
##########
@@ -1903,18 +1946,40 @@ def test_dictionary_from_strings():
assert a.dictionary.equals(expected_dictionary)
-def _has_unique_field_names(ty):
- if isinstance(ty, pa.StructType):
- field_names = [field.name for field in ty]
- return len(set(field_names)) == len(field_names)
- else:
- return True
+@pytest.mark.parametrize('unit', ['s', 'ms', 'us', 'ns'])
+def test_duration_array_roundtrip_corner_cases(unit):
+ # Corner case discovered by hypothesis: there were implicit conversions to
+ # unsigned values resulting wrong values with wrong signs.
+ ty = pa.duration(unit)
+ arr = pa.array([-2147483000], type=ty)
+ restored = pa.array(arr.to_pylist(), type=ty)
+ assert arr.equals(restored)
+
+
+@pytest.mark.pandas
+def test_roundtrip_nanosecond_resolution_pandas_temporal_objects():
+ # corner case discovered by hypothesis: preserving the nanoseconds on
+ # conversion from a list of Timedelta and Timestamp objects
+ import pandas as pd
+
+ ty = pa.duration('ns')
+ arr = pa.array([9223371273709551616], type=ty)
+ data = arr.to_pylist()
+ assert isinstance(data[0], pd.Timedelta)
+ restored = pa.array(data, type=ty)
Review comment:
Same here and below: Shouldn't you also test the actual value of `restored`?
##########
File path: python/pyarrow/tests/test_convert_builtin.py
##########
@@ -1903,18 +1946,40 @@ def test_dictionary_from_strings():
assert a.dictionary.equals(expected_dictionary)
-def _has_unique_field_names(ty):
- if isinstance(ty, pa.StructType):
- field_names = [field.name for field in ty]
- return len(set(field_names)) == len(field_names)
- else:
- return True
+@pytest.mark.parametrize('unit', ['s', 'ms', 'us', 'ns'])
+def test_duration_array_roundtrip_corner_cases(unit):
+ # Corner case discovered by hypothesis: there were implicit conversions to
+ # unsigned values resulting wrong values with wrong signs.
+ ty = pa.duration(unit)
+ arr = pa.array([-2147483000], type=ty)
+ restored = pa.array(arr.to_pylist(), type=ty)
+ assert arr.equals(restored)
+
+
+@pytest.mark.pandas
+def test_roundtrip_nanosecond_resolution_pandas_temporal_objects():
+ # corner case discovered by hypothesis: preserving the nanoseconds on
+ # conversion from a list of Timedelta and Timestamp objects
+ import pandas as pd
+
+ ty = pa.duration('ns')
+ arr = pa.array([9223371273709551616], type=ty)
+ data = arr.to_pylist()
+ assert isinstance(data[0], pd.Timedelta)
+ restored = pa.array(data, type=ty)
+ assert arr.equals(restored)
+
+ ty = pa.timestamp('ns')
+ arr = pa.array([9223371273709551616], type=ty)
+ data = arr.to_pylist()
+ assert isinstance(data[0], pd.Timestamp)
+ restored = pa.array(data, type=ty)
+ assert arr.equals(restored)
@h.given(past.all_arrays)
def test_array_to_pylist_roundtrip(arr):
# TODO(kszucs): ARROW-9997
Review comment:
Shouldn't you remove this TODO?
----------------------------------------------------------------
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
[GitHub] [arrow] kszucs commented on a change in pull request #8349: ARROW-3080: [Python] Unify Arrow to Python object conversion paths
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #8349:
URL: https://github.com/apache/arrow/pull/8349#discussion_r499835603
##########
File path: cpp/src/arrow/python/python_to_arrow.cc
##########
@@ -1004,6 +991,9 @@ Result<std::shared_ptr<ChunkedArray>> ConvertPySequence(PyObject* obj, PyObject*
PyObject* seq;
OwnedRef tmp_seq_nanny;
+ // FIXME(kszucs): shouldn't import pandas unconditionally
Review comment:
We import pandas when converting from arrow to python in case of nanosecond resolution timestamp so to have successful roundtrip we need pandas to convert it back.
----------------------------------------------------------------
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
[GitHub] [arrow] kszucs removed a comment on pull request #8349: ARROW-3080: [Python] Unify Arrow to Python object conversion paths
Posted by GitBox <gi...@apache.org>.
kszucs removed a comment on pull request #8349:
URL: https://github.com/apache/arrow/pull/8349#issuecomment-704914556
@github-actions crossbow submit test-conda-python-3.8-hypothesis
----------------------------------------------------------------
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
[GitHub] [arrow] pitrou commented on a change in pull request #8349: ARROW-3080: [Python] Unify Arrow to Python object conversion paths
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8349:
URL: https://github.com/apache/arrow/pull/8349#discussion_r501762533
##########
File path: cpp/src/arrow/python/helpers.cc
##########
@@ -258,30 +266,46 @@ bool PyFloat_IsNaN(PyObject* obj) {
namespace {
static std::once_flag pandas_static_initialized;
-static PyTypeObject* pandas_NaTType = nullptr;
+
static PyObject* pandas_NA = nullptr;
+static PyObject* pandas_NaT = nullptr;
+static PyObject* pandas_Timedelta = nullptr;
+static PyObject* pandas_Timestamp = nullptr;
+static PyTypeObject* pandas_NaTType = nullptr;
void GetPandasStaticSymbols() {
OwnedRef pandas;
+
+ // import pandas
Status s = ImportModule("pandas", &pandas);
if (!s.ok()) {
return;
}
OwnedRef ref;
- s = ImportFromModule(pandas.obj(), "NaT", &ref);
- if (!s.ok()) {
- return;
+
+ // set NaT sentinel and its type
+ if (ImportFromModule(pandas.obj(), "NaT", &ref).ok()) {
+ pandas_NaT = ref.obj();
+ // PyObject_Type returns a new reference but we trust that pandas.NaT will
Review comment:
You can just use the `Py_TYPE` macro then: https://docs.python.org/3/c-api/structures.html#c.Py_TYPE
##########
File path: python/pyarrow/tests/test_convert_builtin.py
##########
@@ -1903,18 +1946,40 @@ def test_dictionary_from_strings():
assert a.dictionary.equals(expected_dictionary)
-def _has_unique_field_names(ty):
- if isinstance(ty, pa.StructType):
- field_names = [field.name for field in ty]
- return len(set(field_names)) == len(field_names)
- else:
- return True
+@pytest.mark.parametrize('unit', ['s', 'ms', 'us', 'ns'])
+def test_duration_array_roundtrip_corner_cases(unit):
+ # Corner case discovered by hypothesis: there were implicit conversions to
+ # unsigned values resulting wrong values with wrong signs.
+ ty = pa.duration(unit)
+ arr = pa.array([-2147483000], type=ty)
+ restored = pa.array(arr.to_pylist(), type=ty)
Review comment:
Shouldn't you also test the actual value of `restored`?
##########
File path: python/pyarrow/tests/test_convert_builtin.py
##########
@@ -1903,18 +1946,40 @@ def test_dictionary_from_strings():
assert a.dictionary.equals(expected_dictionary)
-def _has_unique_field_names(ty):
- if isinstance(ty, pa.StructType):
- field_names = [field.name for field in ty]
- return len(set(field_names)) == len(field_names)
- else:
- return True
+@pytest.mark.parametrize('unit', ['s', 'ms', 'us', 'ns'])
+def test_duration_array_roundtrip_corner_cases(unit):
+ # Corner case discovered by hypothesis: there were implicit conversions to
+ # unsigned values resulting wrong values with wrong signs.
+ ty = pa.duration(unit)
+ arr = pa.array([-2147483000], type=ty)
+ restored = pa.array(arr.to_pylist(), type=ty)
+ assert arr.equals(restored)
+
+
+@pytest.mark.pandas
+def test_roundtrip_nanosecond_resolution_pandas_temporal_objects():
+ # corner case discovered by hypothesis: preserving the nanoseconds on
+ # conversion from a list of Timedelta and Timestamp objects
+ import pandas as pd
+
+ ty = pa.duration('ns')
+ arr = pa.array([9223371273709551616], type=ty)
+ data = arr.to_pylist()
+ assert isinstance(data[0], pd.Timedelta)
+ restored = pa.array(data, type=ty)
Review comment:
Same here and below: Shouldn't you also test the actual value of `restored`?
##########
File path: python/pyarrow/tests/test_convert_builtin.py
##########
@@ -1903,18 +1946,40 @@ def test_dictionary_from_strings():
assert a.dictionary.equals(expected_dictionary)
-def _has_unique_field_names(ty):
- if isinstance(ty, pa.StructType):
- field_names = [field.name for field in ty]
- return len(set(field_names)) == len(field_names)
- else:
- return True
+@pytest.mark.parametrize('unit', ['s', 'ms', 'us', 'ns'])
+def test_duration_array_roundtrip_corner_cases(unit):
+ # Corner case discovered by hypothesis: there were implicit conversions to
+ # unsigned values resulting wrong values with wrong signs.
+ ty = pa.duration(unit)
+ arr = pa.array([-2147483000], type=ty)
+ restored = pa.array(arr.to_pylist(), type=ty)
+ assert arr.equals(restored)
+
+
+@pytest.mark.pandas
+def test_roundtrip_nanosecond_resolution_pandas_temporal_objects():
+ # corner case discovered by hypothesis: preserving the nanoseconds on
+ # conversion from a list of Timedelta and Timestamp objects
+ import pandas as pd
+
+ ty = pa.duration('ns')
+ arr = pa.array([9223371273709551616], type=ty)
+ data = arr.to_pylist()
+ assert isinstance(data[0], pd.Timedelta)
+ restored = pa.array(data, type=ty)
+ assert arr.equals(restored)
+
+ ty = pa.timestamp('ns')
+ arr = pa.array([9223371273709551616], type=ty)
+ data = arr.to_pylist()
+ assert isinstance(data[0], pd.Timestamp)
+ restored = pa.array(data, type=ty)
+ assert arr.equals(restored)
@h.given(past.all_arrays)
def test_array_to_pylist_roundtrip(arr):
# TODO(kszucs): ARROW-9997
Review comment:
Shouldn't you remove this TODO?
----------------------------------------------------------------
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
[GitHub] [arrow] kszucs commented on a change in pull request #8349: ARROW-3080: [Python] Unify Arrow to Python object conversion paths
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #8349:
URL: https://github.com/apache/arrow/pull/8349#discussion_r500957250
##########
File path: cpp/src/arrow/python/python_to_arrow.cc
##########
@@ -298,7 +298,14 @@ class PyValue {
value = internal::PyDelta_to_us(dt);
break;
case TimeUnit::NANO:
- value = internal::PyDelta_to_ns(dt);
+ if (internal::IsPandasTimedelta(obj)) {
+ OwnedRef nanos(PyObject_GetAttrString(obj, "nanoseconds"));
+ RETURN_IF_PYERROR();
+ RETURN_NOT_OK(internal::CIntFromPython(nanos.obj(), &value));
+ value += internal::PyDelta_to_ns(dt);
Review comment:
Added support for it.
----------------------------------------------------------------
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
[GitHub] [arrow] kszucs commented on a change in pull request #8349: ARROW-3080: [Python] Unify Arrow to Python object conversion paths
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #8349:
URL: https://github.com/apache/arrow/pull/8349#discussion_r500948820
##########
File path: cpp/src/arrow/python/python_to_arrow.cc
##########
@@ -298,7 +298,14 @@ class PyValue {
value = internal::PyDelta_to_us(dt);
break;
case TimeUnit::NANO:
- value = internal::PyDelta_to_ns(dt);
+ if (internal::IsPandasTimedelta(obj)) {
+ OwnedRef nanos(PyObject_GetAttrString(obj, "nanoseconds"));
+ RETURN_IF_PYERROR();
+ RETURN_NOT_OK(internal::CIntFromPython(nanos.obj(), &value));
+ value += internal::PyDelta_to_ns(dt);
Review comment:
Yes, just forget to add because bugs were raining.
----------------------------------------------------------------
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
[GitHub] [arrow] kszucs commented on a change in pull request #8349: ARROW-3080: [Python] Unify Arrow to Python object conversion paths
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #8349:
URL: https://github.com/apache/arrow/pull/8349#discussion_r500988522
##########
File path: python/pyarrow/tests/strategies.py
##########
@@ -124,39 +132,67 @@ def fields(draw, type_strategy=primitive_types):
def list_types(item_strategy=primitive_types):
return (
st.builds(pa.list_, item_strategy) |
- st.builds(pa.large_list, item_strategy)
+ st.builds(pa.large_list, item_strategy) |
+ st.builds(
+ pa.list_,
+ item_strategy,
+ st.integers(min_value=0, max_value=16)
+ )
)
-def struct_types(item_strategy=primitive_types):
- return st.builds(pa.struct, st.lists(fields(item_strategy)))
-
-
-def complex_types(inner_strategy=primitive_types):
- return list_types(inner_strategy) | struct_types(inner_strategy)
-
-
-def nested_list_types(item_strategy=primitive_types, max_leaves=3):
- return st.recursive(item_strategy, list_types, max_leaves=max_leaves)
+@st.composite
+def struct_types(draw, item_strategy=primitive_types):
+ fields_strategy = st.lists(fields(item_strategy))
+ fields_rendered = draw(fields_strategy)
+ field_names = [field.name for field in fields_rendered]
+ # check that field names are unique, see ARROW-9997
+ h.assume(len(set(field_names)) == len(field_names))
+ return pa.struct(fields_rendered)
+
+
+def dictionary_types(key_strategy=None, value_strategy=None):
+ key_strategy = key_strategy or signed_integer_types
+ value_strategy = value_strategy or st.one_of(
+ bool_type,
+ integer_types,
+ st.sampled_from([pa.float32(), pa.float64()]),
+ binary_type,
+ string_type,
+ fixed_size_binary_type,
+ )
+ return st.builds(pa.dictionary, key_strategy, value_strategy)
-def nested_struct_types(item_strategy=primitive_types, max_leaves=3):
- return st.recursive(item_strategy, struct_types, max_leaves=max_leaves)
+@st.composite
+def map_types(draw, key_strategy=primitive_types,
+ item_strategy=primitive_types):
+ key_type = draw(key_strategy)
+ h.assume(not pa.types.is_null(key_type))
+ value_type = draw(item_strategy)
+ return pa.map_(key_type, value_type)
-def nested_complex_types(inner_strategy=primitive_types, max_leaves=3):
- return st.recursive(inner_strategy, complex_types, max_leaves=max_leaves)
+# union type
+# extension type
Review comment:
Created a follow-up jira https://issues.apache.org/jira/browse/ARROW-10212
----------------------------------------------------------------
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
[GitHub] [arrow] kszucs commented on a change in pull request #8349: ARROW-3080: [Python] Unify Arrow to Python object conversion paths
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #8349:
URL: https://github.com/apache/arrow/pull/8349#discussion_r500986644
##########
File path: cpp/src/arrow/python/python_to_arrow.cc
##########
@@ -1004,6 +991,9 @@ Result<std::shared_ptr<ChunkedArray>> ConvertPySequence(PyObject* obj, PyObject*
PyObject* seq;
OwnedRef tmp_seq_nanny;
+ // FIXME(kszucs): shouldn't import pandas unconditionally
Review comment:
Updated according to my previous comment.
----------------------------------------------------------------
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
[GitHub] [arrow] kszucs commented on pull request #8349: ARROW-3080: [Python] Unify Arrow to Python object conversion paths
Posted by GitBox <gi...@apache.org>.
kszucs commented on pull request #8349:
URL: https://github.com/apache/arrow/pull/8349#issuecomment-705177619
@github-actions crossbow submit test-conda-python-3.8-hypothesis
----------------------------------------------------------------
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
[GitHub] [arrow] kszucs commented on pull request #8349: ARROW-3080: [Python] Unify Arrow to Python object conversion paths
Posted by GitBox <gi...@apache.org>.
kszucs commented on pull request #8349:
URL: https://github.com/apache/arrow/pull/8349#issuecomment-704914556
@github-actions crossbow submit test-conda-python-3.8-hypothesis
----------------------------------------------------------------
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
[GitHub] [arrow] kszucs commented on a change in pull request #8349: ARROW-3080: [Python] Unify Arrow to Python object conversion paths
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #8349:
URL: https://github.com/apache/arrow/pull/8349#discussion_r500959959
##########
File path: cpp/src/arrow/python/python_to_arrow.cc
##########
@@ -1004,6 +991,9 @@ Result<std::shared_ptr<ChunkedArray>> ConvertPySequence(PyObject* obj, PyObject*
PyObject* seq;
OwnedRef tmp_seq_nanny;
+ // FIXME(kszucs): shouldn't import pandas unconditionally
Review comment:
We could make it depend on `options.from_pandas`, but currently we use that flag for null checks only.
----------------------------------------------------------------
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
[GitHub] [arrow] kszucs commented on a change in pull request #8349: ARROW-3080: [Python] Unify Arrow to Python object conversion paths
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #8349:
URL: https://github.com/apache/arrow/pull/8349#discussion_r502497329
##########
File path: python/pyarrow/tests/test_convert_builtin.py
##########
@@ -1903,18 +1946,40 @@ def test_dictionary_from_strings():
assert a.dictionary.equals(expected_dictionary)
-def _has_unique_field_names(ty):
- if isinstance(ty, pa.StructType):
- field_names = [field.name for field in ty]
- return len(set(field_names)) == len(field_names)
- else:
- return True
+@pytest.mark.parametrize('unit', ['s', 'ms', 'us', 'ns'])
+def test_duration_array_roundtrip_corner_cases(unit):
+ # Corner case discovered by hypothesis: there were implicit conversions to
+ # unsigned values resulting wrong values with wrong signs.
+ ty = pa.duration(unit)
+ arr = pa.array([-2147483000], type=ty)
+ restored = pa.array(arr.to_pylist(), type=ty)
+ assert arr.equals(restored)
+
+
+@pytest.mark.pandas
+def test_roundtrip_nanosecond_resolution_pandas_temporal_objects():
+ # corner case discovered by hypothesis: preserving the nanoseconds on
+ # conversion from a list of Timedelta and Timestamp objects
+ import pandas as pd
+
+ ty = pa.duration('ns')
+ arr = pa.array([9223371273709551616], type=ty)
+ data = arr.to_pylist()
+ assert isinstance(data[0], pd.Timedelta)
+ restored = pa.array(data, type=ty)
+ assert arr.equals(restored)
+
+ ty = pa.timestamp('ns')
+ arr = pa.array([9223371273709551616], type=ty)
+ data = arr.to_pylist()
+ assert isinstance(data[0], pd.Timestamp)
+ restored = pa.array(data, type=ty)
+ assert arr.equals(restored)
@h.given(past.all_arrays)
def test_array_to_pylist_roundtrip(arr):
# TODO(kszucs): ARROW-9997
Review comment:
Removed.
##########
File path: cpp/src/arrow/python/helpers.cc
##########
@@ -258,30 +266,46 @@ bool PyFloat_IsNaN(PyObject* obj) {
namespace {
static std::once_flag pandas_static_initialized;
-static PyTypeObject* pandas_NaTType = nullptr;
+
static PyObject* pandas_NA = nullptr;
+static PyObject* pandas_NaT = nullptr;
+static PyObject* pandas_Timedelta = nullptr;
+static PyObject* pandas_Timestamp = nullptr;
+static PyTypeObject* pandas_NaTType = nullptr;
void GetPandasStaticSymbols() {
OwnedRef pandas;
+
+ // import pandas
Status s = ImportModule("pandas", &pandas);
if (!s.ok()) {
return;
}
OwnedRef ref;
- s = ImportFromModule(pandas.obj(), "NaT", &ref);
- if (!s.ok()) {
- return;
+
+ // set NaT sentinel and its type
+ if (ImportFromModule(pandas.obj(), "NaT", &ref).ok()) {
+ pandas_NaT = ref.obj();
+ // PyObject_Type returns a new reference but we trust that pandas.NaT will
Review comment:
Updated.
----------------------------------------------------------------
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
[GitHub] [arrow] github-actions[bot] commented on pull request #8349: ARROW-3080: [Python] Unify Arrow to Python object conversion paths
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8349:
URL: https://github.com/apache/arrow/pull/8349#issuecomment-704918186
Revision: f359b3d27a52b705de30e363801cdaa72c2c2b8f
Submitted crossbow builds: [ursa-labs/crossbow @ actions-610](https://github.com/ursa-labs/crossbow/branches/all?query=actions-610)
|Task|Status|
|----|------|
|test-conda-python-3.8-hypothesis|[](https://github.com/ursa-labs/crossbow/actions?query=branch:actions-610-github-test-conda-python-3.8-hypothesis)|
----------------------------------------------------------------
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
[GitHub] [arrow] kszucs commented on pull request #8349: ARROW-3080: [Python] Unify Arrow to Python object conversion paths
Posted by GitBox <gi...@apache.org>.
kszucs commented on pull request #8349:
URL: https://github.com/apache/arrow/pull/8349#issuecomment-704890653
@github-actions crossbow submit test-conda-python-3.8-hypothesis
----------------------------------------------------------------
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
[GitHub] [arrow] kszucs commented on a change in pull request #8349: ARROW-3080: [Python] Unify Arrow to Python object conversion paths
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #8349:
URL: https://github.com/apache/arrow/pull/8349#discussion_r499836082
##########
File path: python/pyarrow/tests/strategies.py
##########
@@ -293,3 +359,32 @@ def tables(draw, type, rows=None, max_fields=None):
all_chunked_arrays = chunked_arrays(all_types)
all_record_batches = record_batches(all_types)
all_tables = tables(all_types)
+
+
+pandas_compatible_primitive_types = st.one_of(
+ null_type,
+ bool_type,
+ integer_types,
+ st.sampled_from([pa.float32(), pa.float64()]),
+ decimal_type,
+ date_types,
+ time_types,
+ # timestamp_types,
+ # duration_types
Review comment:
Enable these.
----------------------------------------------------------------
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
[GitHub] [arrow] kszucs commented on a change in pull request #8349: ARROW-3080: [Python] Unify Arrow to Python object conversion paths
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #8349:
URL: https://github.com/apache/arrow/pull/8349#discussion_r500970961
##########
File path: cpp/src/arrow/python/python_to_arrow.cc
##########
@@ -1004,6 +991,9 @@ Result<std::shared_ptr<ChunkedArray>> ConvertPySequence(PyObject* obj, PyObject*
PyObject* seq;
OwnedRef tmp_seq_nanny;
+ // FIXME(kszucs): shouldn't import pandas unconditionally
Review comment:
Perhaps only initialize pandas static data if pandas has been already imported (in sys.modules)?
----------------------------------------------------------------
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
[GitHub] [arrow] kszucs commented on a change in pull request #8349: ARROW-3080: [Python] Unify Arrow to Python object conversion paths
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #8349:
URL: https://github.com/apache/arrow/pull/8349#discussion_r500988522
##########
File path: python/pyarrow/tests/strategies.py
##########
@@ -124,39 +132,67 @@ def fields(draw, type_strategy=primitive_types):
def list_types(item_strategy=primitive_types):
return (
st.builds(pa.list_, item_strategy) |
- st.builds(pa.large_list, item_strategy)
+ st.builds(pa.large_list, item_strategy) |
+ st.builds(
+ pa.list_,
+ item_strategy,
+ st.integers(min_value=0, max_value=16)
+ )
)
-def struct_types(item_strategy=primitive_types):
- return st.builds(pa.struct, st.lists(fields(item_strategy)))
-
-
-def complex_types(inner_strategy=primitive_types):
- return list_types(inner_strategy) | struct_types(inner_strategy)
-
-
-def nested_list_types(item_strategy=primitive_types, max_leaves=3):
- return st.recursive(item_strategy, list_types, max_leaves=max_leaves)
+@st.composite
+def struct_types(draw, item_strategy=primitive_types):
+ fields_strategy = st.lists(fields(item_strategy))
+ fields_rendered = draw(fields_strategy)
+ field_names = [field.name for field in fields_rendered]
+ # check that field names are unique, see ARROW-9997
+ h.assume(len(set(field_names)) == len(field_names))
+ return pa.struct(fields_rendered)
+
+
+def dictionary_types(key_strategy=None, value_strategy=None):
+ key_strategy = key_strategy or signed_integer_types
+ value_strategy = value_strategy or st.one_of(
+ bool_type,
+ integer_types,
+ st.sampled_from([pa.float32(), pa.float64()]),
+ binary_type,
+ string_type,
+ fixed_size_binary_type,
+ )
+ return st.builds(pa.dictionary, key_strategy, value_strategy)
-def nested_struct_types(item_strategy=primitive_types, max_leaves=3):
- return st.recursive(item_strategy, struct_types, max_leaves=max_leaves)
+@st.composite
+def map_types(draw, key_strategy=primitive_types,
+ item_strategy=primitive_types):
+ key_type = draw(key_strategy)
+ h.assume(not pa.types.is_null(key_type))
+ value_type = draw(item_strategy)
+ return pa.map_(key_type, value_type)
-def nested_complex_types(inner_strategy=primitive_types, max_leaves=3):
- return st.recursive(inner_strategy, complex_types, max_leaves=max_leaves)
+# union type
+# extension type
Review comment:
Created a follow-up jira to generate these types as well https://issues.apache.org/jira/browse/ARROW-10212
----------------------------------------------------------------
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
[GitHub] [arrow] kszucs commented on a change in pull request #8349: ARROW-3080: [Python] Unify Arrow to Python object conversion paths
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #8349:
URL: https://github.com/apache/arrow/pull/8349#discussion_r500967626
##########
File path: cpp/src/arrow/python/python_to_arrow.cc
##########
@@ -1004,6 +991,9 @@ Result<std::shared_ptr<ChunkedArray>> ConvertPySequence(PyObject* obj, PyObject*
PyObject* seq;
OwnedRef tmp_seq_nanny;
+ // FIXME(kszucs): shouldn't import pandas unconditionally
Review comment:
I think we can simply remove this initialization since pandas should be already imported to have pandas objects in the input data.
----------------------------------------------------------------
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
[GitHub] [arrow] jorisvandenbossche commented on a change in pull request #8349: ARROW-3080: [Python] Unify Arrow to Python object conversion paths
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on a change in pull request #8349:
URL: https://github.com/apache/arrow/pull/8349#discussion_r500820685
##########
File path: cpp/src/arrow/python/python_to_arrow.cc
##########
@@ -298,7 +298,14 @@ class PyValue {
value = internal::PyDelta_to_us(dt);
break;
case TimeUnit::NANO:
- value = internal::PyDelta_to_ns(dt);
+ if (internal::IsPandasTimedelta(obj)) {
+ OwnedRef nanos(PyObject_GetAttrString(obj, "nanoseconds"));
+ RETURN_IF_PYERROR();
+ RETURN_NOT_OK(internal::CIntFromPython(nanos.obj(), &value));
+ value += internal::PyDelta_to_ns(dt);
Review comment:
Should something similar be needed for Timestamp (datetime) ?
##########
File path: python/pyarrow/tests/test_convert_builtin.py
##########
@@ -1894,18 +1937,30 @@ def test_dictionary_from_strings():
assert a.dictionary.equals(expected_dictionary)
-def _has_unique_field_names(ty):
- if isinstance(ty, pa.StructType):
- field_names = [field.name for field in ty]
- return len(set(field_names)) == len(field_names)
- else:
- return True
+@pytest.mark.parametrize('unit', ['s', 'ms', 'us', 'ns'])
+def test_duration_array_roundtrip_corner_cases(unit):
+ # corner case discovered by hypothesis
+ ty = pa.duration(unit)
+ arr = pa.array([-2147483000], type=ty)
+ restored = pa.array(arr.to_pylist(), type=ty)
+ assert arr.equals(restored)
Review comment:
Do you know what was going wrong in this case?
##########
File path: python/pyarrow/tests/test_convert_builtin.py
##########
@@ -1894,18 +1937,30 @@ def test_dictionary_from_strings():
assert a.dictionary.equals(expected_dictionary)
-def _has_unique_field_names(ty):
- if isinstance(ty, pa.StructType):
- field_names = [field.name for field in ty]
- return len(set(field_names)) == len(field_names)
- else:
- return True
+@pytest.mark.parametrize('unit', ['s', 'ms', 'us', 'ns'])
+def test_duration_array_roundtrip_corner_cases(unit):
+ # corner case discovered by hypothesis
+ ty = pa.duration(unit)
+ arr = pa.array([-2147483000], type=ty)
+ restored = pa.array(arr.to_pylist(), type=ty)
+ assert arr.equals(restored)
+
+
+@pytest.mark.pandas
+def test_duration_array_roundtrip_nanosecond_resolution_pandas_timedelta():
+ # corner case discovered by hypothesis
Review comment:
Can you also mention the actual bug it is testing? (i.e. preserving the nanoseconds on conversion from a list of Timedelta objects, I think?)
----------------------------------------------------------------
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
[GitHub] [arrow] kszucs commented on pull request #8349: ARROW-3080: [Python] Unify Arrow to Python object conversion paths
Posted by GitBox <gi...@apache.org>.
kszucs commented on pull request #8349:
URL: https://github.com/apache/arrow/pull/8349#issuecomment-703854189
@github-actions crossbow submit test-conda-python-3.8-hypothesis
----------------------------------------------------------------
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
[GitHub] [arrow] github-actions[bot] commented on pull request #8349: ARROW-3080: [Python] Unify Arrow to Python object conversion paths
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8349:
URL: https://github.com/apache/arrow/pull/8349#issuecomment-703849579
https://issues.apache.org/jira/browse/ARROW-3080
----------------------------------------------------------------
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
[GitHub] [arrow] kszucs commented on a change in pull request #8349: ARROW-3080: [Python] Unify Arrow to Python object conversion paths
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #8349:
URL: https://github.com/apache/arrow/pull/8349#discussion_r500961615
##########
File path: cpp/src/arrow/python/python_to_arrow.cc
##########
@@ -1004,6 +991,9 @@ Result<std::shared_ptr<ChunkedArray>> ConvertPySequence(PyObject* obj, PyObject*
PyObject* seq;
OwnedRef tmp_seq_nanny;
+ // FIXME(kszucs): shouldn't import pandas unconditionally
Review comment:
@jorisvandenbossche any suggestions here?
----------------------------------------------------------------
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
[GitHub] [arrow] kszucs commented on a change in pull request #8349: ARROW-3080: [Python] Unify Arrow to Python object conversion paths
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #8349:
URL: https://github.com/apache/arrow/pull/8349#discussion_r500958013
##########
File path: python/pyarrow/tests/strategies.py
##########
@@ -293,3 +359,32 @@ def tables(draw, type, rows=None, max_fields=None):
all_chunked_arrays = chunked_arrays(all_types)
all_record_batches = record_batches(all_types)
all_tables = tables(all_types)
+
+
+pandas_compatible_primitive_types = st.one_of(
+ null_type,
+ bool_type,
+ integer_types,
+ st.sampled_from([pa.float32(), pa.float64()]),
+ decimal_type,
+ date_types,
+ time_types,
+ # timestamp_types,
+ # duration_types
Review comment:
Depends on https://issues.apache.org/jira/browse/ARROW-10210
----------------------------------------------------------------
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
[GitHub] [arrow] github-actions[bot] commented on pull request #8349: ARROW-3080: [Python] Unify Arrow to Python object conversion paths
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8349:
URL: https://github.com/apache/arrow/pull/8349#issuecomment-704895599
Revision: 018e03ef9d6609a7fdc5e8e2d884c958cf38b192
Submitted crossbow builds: [ursa-labs/crossbow @ actions-609](https://github.com/ursa-labs/crossbow/branches/all?query=actions-609)
|Task|Status|
|----|------|
|test-conda-python-3.8-hypothesis|[](https://github.com/ursa-labs/crossbow/actions?query=branch:actions-609-github-test-conda-python-3.8-hypothesis)|
----------------------------------------------------------------
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
[GitHub] [arrow] github-actions[bot] commented on pull request #8349: ARROW-3080: [Python] Unify Arrow to Python object conversion paths
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8349:
URL: https://github.com/apache/arrow/pull/8349#issuecomment-705178406
Revision: f359b3d27a52b705de30e363801cdaa72c2c2b8f
Submitted crossbow builds: [ursa-labs/crossbow @ actions-614](https://github.com/ursa-labs/crossbow/branches/all?query=actions-614)
|Task|Status|
|----|------|
|test-conda-python-3.8-hypothesis|[](https://github.com/ursa-labs/crossbow/actions?query=branch:actions-614-github-test-conda-python-3.8-hypothesis)|
----------------------------------------------------------------
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
[GitHub] [arrow] jorisvandenbossche closed pull request #8349: ARROW-3080: [Python] Unify Arrow to Python object conversion paths
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche closed pull request #8349:
URL: https://github.com/apache/arrow/pull/8349
----------------------------------------------------------------
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
[GitHub] [arrow] github-actions[bot] removed a comment on pull request #8349: ARROW-3080: [Python] Unify Arrow to Python object conversion paths
Posted by GitBox <gi...@apache.org>.
github-actions[bot] removed a comment on pull request #8349:
URL: https://github.com/apache/arrow/pull/8349#issuecomment-704918186
Revision: f359b3d27a52b705de30e363801cdaa72c2c2b8f
Submitted crossbow builds: [ursa-labs/crossbow @ actions-610](https://github.com/ursa-labs/crossbow/branches/all?query=actions-610)
|Task|Status|
|----|------|
|test-conda-python-3.8-hypothesis|[](https://github.com/ursa-labs/crossbow/actions?query=branch:actions-610-github-test-conda-python-3.8-hypothesis)|
----------------------------------------------------------------
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
[GitHub] [arrow] kszucs commented on a change in pull request #8349: ARROW-3080: [Python] Unify Arrow to Python object conversion paths
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #8349:
URL: https://github.com/apache/arrow/pull/8349#discussion_r500945878
##########
File path: python/pyarrow/tests/test_convert_builtin.py
##########
@@ -1894,18 +1937,30 @@ def test_dictionary_from_strings():
assert a.dictionary.equals(expected_dictionary)
-def _has_unique_field_names(ty):
- if isinstance(ty, pa.StructType):
- field_names = [field.name for field in ty]
- return len(set(field_names)) == len(field_names)
- else:
- return True
+@pytest.mark.parametrize('unit', ['s', 'ms', 'us', 'ns'])
+def test_duration_array_roundtrip_corner_cases(unit):
+ # corner case discovered by hypothesis
+ ty = pa.duration(unit)
+ arr = pa.array([-2147483000], type=ty)
+ restored = pa.array(arr.to_pylist(), type=ty)
+ assert arr.equals(restored)
Review comment:
Yes, added comment.
##########
File path: python/pyarrow/tests/test_convert_builtin.py
##########
@@ -1894,18 +1937,30 @@ def test_dictionary_from_strings():
assert a.dictionary.equals(expected_dictionary)
-def _has_unique_field_names(ty):
- if isinstance(ty, pa.StructType):
- field_names = [field.name for field in ty]
- return len(set(field_names)) == len(field_names)
- else:
- return True
+@pytest.mark.parametrize('unit', ['s', 'ms', 'us', 'ns'])
+def test_duration_array_roundtrip_corner_cases(unit):
+ # corner case discovered by hypothesis
+ ty = pa.duration(unit)
+ arr = pa.array([-2147483000], type=ty)
+ restored = pa.array(arr.to_pylist(), type=ty)
+ assert arr.equals(restored)
+
+
+@pytest.mark.pandas
+def test_duration_array_roundtrip_nanosecond_resolution_pandas_timedelta():
+ # corner case discovered by hypothesis
Review comment:
Added comment.
----------------------------------------------------------------
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
[GitHub] [arrow] github-actions[bot] commented on pull request #8349: ARROW-3080: [Python] Unify Arrow to Python object conversion paths
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8349:
URL: https://github.com/apache/arrow/pull/8349#issuecomment-703855507
Revision: f823a17f209e16c9583ee3ad630730d5164ec98b
Submitted crossbow builds: [ursa-labs/crossbow @ actions-602](https://github.com/ursa-labs/crossbow/branches/all?query=actions-602)
|Task|Status|
|----|------|
|test-conda-python-3.8-hypothesis|[](https://github.com/ursa-labs/crossbow/actions?query=branch:actions-602-github-test-conda-python-3.8-hypothesis)|
----------------------------------------------------------------
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