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

ARROW-736: [Python] Mixed-type object DataFrame columns should not silently co… #465

Closed
wants to merge 6 commits into from

Conversation

cpcloud
Copy link
Contributor

@cpcloud cpcloud commented Mar 30, 2017

…erce to an Arrow type by default

@cpcloud cpcloud force-pushed the ARROW-736 branch 3 times, most recently from da635ce to 2702cc6 Compare March 30, 2017 22:32
@wesm
Copy link
Member

wesm commented Mar 30, 2017

needs rebase and cpplint fix

@cpcloud cpcloud changed the title ARROW-736: Mixed-type object DataFrame columns should not silently co… ARROW-736: [Python] Mixed-type object DataFrame columns should not silently co… Mar 31, 2017
Py_DECREF(type_name);
Py_DECREF(type);
DCHECK_NE(PyErr_Occurred(), nullptr) << "Call to PyObject_AsUTF8String(...) returned "
"NULL but PyErr_Occurred() was NULL";
Copy link
Member

Choose a reason for hiding this comment

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

Yikes. Perhaps we should develop some tools to help with the Python C API in C++. I have tried to use the OwnedRef to handle RAII-based DECREF

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice, I'll do that.


Py_DECREF(bytes_obj);
Py_DECREF(type_name);
Py_DECREF(type);
Copy link
Member

Choose a reason for hiding this comment

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

You could try using the OwnedRef for these, save you a bunch of DECREFs per above

namespace arrow {
namespace py {

TEST(BuiltinConversionTest, TestMixedTypeFails) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you put this in pandas-test and rename that test file to python-test.cc instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep.

@cpcloud
Copy link
Contributor Author

cpcloud commented Apr 1, 2017

@wesm good to merge? green builds :)

Copy link
Member

@wesm wesm left a comment

Choose a reason for hiding this comment

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

+1, thank you!

@asfgit asfgit closed this in d75d7a9 Apr 1, 2017
leifwalsh pushed a commit to leifwalsh/arrow that referenced this pull request Apr 1, 2017
…lently co…

…erce to an Arrow type by default

Author: Phillip Cloud <cpcloud@gmail.com>

Closes apache#465 from cpcloud/ARROW-736 and squashes the following commits:

fd09def [Phillip Cloud] Update cmake
bcf6236 [Phillip Cloud] Rename and move
4a18014 [Phillip Cloud] Move test
e80efe1 [Phillip Cloud] Use OwnedRef instead of horror
b2df3e9 [Phillip Cloud] Fix python error handling and make compatible with python27
84d33b4 [Phillip Cloud] ARROW-736: Mixed-type object DataFrame columns should not silently coerce to an Arrow type by default
wesm pushed a commit to wesm/arrow that referenced this pull request Sep 2, 2018
…types

Author: Deepak Majeti <deepak.majeti@hpe.com>

Closes apache#465 from majetideepak/PARQUET-979 and squashes the following commits:

3b18173 [Deepak Majeti] improve naming and ColumnProperties class
a888aa4 [Deepak Majeti] Add an option to specify max stats size
c103c4f [Deepak Majeti] make format
cf0260c [Deepak Majeti] PARQUET-979: [C++] Limit size of min, max or disable stats for long binary types
wesm pushed a commit to wesm/arrow that referenced this pull request Sep 4, 2018
…types

Author: Deepak Majeti <deepak.majeti@hpe.com>

Closes apache#465 from majetideepak/PARQUET-979 and squashes the following commits:

3b18173 [Deepak Majeti] improve naming and ColumnProperties class
a888aa4 [Deepak Majeti] Add an option to specify max stats size
c103c4f [Deepak Majeti] make format
cf0260c [Deepak Majeti] PARQUET-979: [C++] Limit size of min, max or disable stats for long binary types

Change-Id: I2843608b1848b6d85ac226064dd4c5ec93e40f47
wesm pushed a commit to wesm/arrow that referenced this pull request Sep 6, 2018
…types

Author: Deepak Majeti <deepak.majeti@hpe.com>

Closes apache#465 from majetideepak/PARQUET-979 and squashes the following commits:

3b18173 [Deepak Majeti] improve naming and ColumnProperties class
a888aa4 [Deepak Majeti] Add an option to specify max stats size
c103c4f [Deepak Majeti] make format
cf0260c [Deepak Majeti] PARQUET-979: [C++] Limit size of min, max or disable stats for long binary types

Change-Id: I2843608b1848b6d85ac226064dd4c5ec93e40f47
wesm pushed a commit to wesm/arrow that referenced this pull request Sep 7, 2018
…types

Author: Deepak Majeti <deepak.majeti@hpe.com>

Closes apache#465 from majetideepak/PARQUET-979 and squashes the following commits:

3b18173 [Deepak Majeti] improve naming and ColumnProperties class
a888aa4 [Deepak Majeti] Add an option to specify max stats size
c103c4f [Deepak Majeti] make format
cf0260c [Deepak Majeti] PARQUET-979: [C++] Limit size of min, max or disable stats for long binary types

Change-Id: I2843608b1848b6d85ac226064dd4c5ec93e40f47
wesm pushed a commit to wesm/arrow that referenced this pull request Sep 8, 2018
…types

Author: Deepak Majeti <deepak.majeti@hpe.com>

Closes apache#465 from majetideepak/PARQUET-979 and squashes the following commits:

3b18173 [Deepak Majeti] improve naming and ColumnProperties class
a888aa4 [Deepak Majeti] Add an option to specify max stats size
c103c4f [Deepak Majeti] make format
cf0260c [Deepak Majeti] PARQUET-979: [C++] Limit size of min, max or disable stats for long binary types

Change-Id: I2843608b1848b6d85ac226064dd4c5ec93e40f47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants