ARROW-2145/ARROW-2157: [Python] Decimal conversion not working for NaN values

[cpcloud]

No description provided.

[cpcloud]

Restarted the travis jobs, they seemed to be failing during apt-get

[pitrou]

Is this actually required? You are only using the decimal type below.

[cpcloud]

I might be misunderstanding the use case for OwnedRefNoGIL.

My understanding is that if we're Py_XDECREFing something and we do not hold then GIL then we need to acquire it.

The GIL is released in Cython before the function that instantiates this class is called (and isn't subsequently acquired in that function). Assuming my understanding is correct then we need to acquire the GIL before calling Py_XDECREF on this.

Are you suggesting that because the order of destruction here is guaranteed to be the reverse of initialization order that we don't need to care that decimal_module_ holds the GIL because we'll never decref decimal_module_ before we decref decimal_type_?

[pitrou]

What I mean is that you don't care about keeping a reference to the decimal module if you only need to use the decimal type:

>>> D = __import__('decimal').Decimal
>>> sys.modules['decimal'] = None
>>> D(100)
Decimal('100')

[cpcloud]

Oh, awesome. There are a few places that we have unnecessary refs to the decimal module. I'll open a JIRA to clean those up (and remove this one). Thanks for the review.

[pitrou]

This won't work with Python Decimal nans, right? Try decimal.Decimal('nan').

[pitrou]

What is the policy for adding tests here rather than in pyarrow/tests? It seems writing tests in pure Python is generally easier :-)

[cpcloud]

It's funny you mention that. I agree, but I wanted to be able to step through C++ code in the CLion IDE so I wrote the test here.

[cpcloud]

Let me add Decimal('nan') to this list.

[cpcloud]

Done

[cpcloud]

@wesm@pitrou is there any reason we shouldn't make these static global variables, so we don't have to call these APIs all over the place and inside of loops?

[pitrou]

IMO it's fine to make it a static global (or a singleton, etc.), as long as we don't want to support subinterpreters perhaps.

[pitrou]

By the way the nested block above doesn't seem needed.

[cpcloud]

Yep, okay I'll refactor and make those static

[cpcloud]

Hm so it appears that string interning of module names doesn't play well with C++ globals. I'll leave these as they are for now.

[pitrou]

Right, you probably can't use OwnedRef here since the destructor would trigger too late. But you could have a PyObject*. The only downside is that it would make the object eternal.

[pitrou]

Why the consts? I don't think it makes sense, and you're bound to do a lot of casts as soon as you call the Python C API.

[pitrou]

Note PyObject is non-const pretty much by construction, as it has a reference count that can be mutated by any operation.

[cpcloud]

Ok, I was following convention in the file. I can adjust.

[wesm]

I've been removing all the instances of const PyObject* after having a compiler warning from an internal Py-API, so happy to see them all go

[pitrou]

The last argument can simply be the empty string AFAIR.

[cpcloud]

Cool

[pitrou]

You should call PyDecimal_Checkbefore calling the method. Also you should check if the method raises. And it's better to use PyObject_IsTrue rather than compare against Py_True.

[cpcloud]

Yes, not sure how I missed that :)

[cpcloud]

@pitrou any more comments here?

[pitrou]

See comments below. Also it seems that the AppVeyor build has failed, though it may be unrelated?

[pitrou]

This member doesn't seem used actually. Were you planning to use it with PyObject_IsInstance instead of the costlier call to internal::PyDecimal_Check?

[cpcloud]

Yes, thanks. Will fix.

[pitrou]

Might be nice to add a comment motivating the algorithm here (why compare the absolute values but then memorize the original value)?

[cpcloud]

Actually, reconsidering this based on your comment this really should be just the max scale. Negative scale should contribute to precision only if it would increase precision. The goal here is to "cast the widest net", ie the max precision and max scale. Negative scale complicates things a tiny bit. I'll add some commentary.

[pitrou]

I'm not a C++ expert, so I'm curious why this is necessary?

[cpcloud]

Constructors are not inherited by default. I'm not actually using this, so it should cost nothing at runtime. If we ever wanted to construct one of these with a pointer as it's first argument we'd have to define it anyway. I can remove it if you'd like.

[pitrou]

No problem with me. I had forgotten about non-inheritance of constructors.

[pitrou]

Can PandasObjectIsNull return true on a Decimal instance?

[cpcloud]

No, I'll fix that.

[pitrou]

Also check that the Arrow type was inferred correctly?

[cpcloud]

Yep will do.

[wesm]

needs rebase

[cpcloud]

Closed in favor of #1651

[cpcloud]

No description provided.

[cpcloud]

Restarted the travis jobs, they seemed to be failing during apt-get

[pitrou]

Is this actually required? You are only using the decimal type below.

[cpcloud]

I might be misunderstanding the use case for OwnedRefNoGIL.

My understanding is that if we're Py_XDECREFing something and we do not hold then GIL then we need to acquire it.

The GIL is released in Cython before the function that instantiates this class is called (and isn't subsequently acquired in that function). Assuming my understanding is correct then we need to acquire the GIL before calling Py_XDECREF on this.

Are you suggesting that because the order of destruction here is guaranteed to be the reverse of initialization order that we don't need to care that decimal_module_ holds the GIL because we'll never decref decimal_module_ before we decref decimal_type_?

[pitrou]

What I mean is that you don't care about keeping a reference to the decimal module if you only need to use the decimal type:

>>> D = __import__('decimal').Decimal
>>> sys.modules['decimal'] = None
>>> D(100)
Decimal('100')

[cpcloud]

Oh, awesome. There are a few places that we have unnecessary refs to the decimal module. I'll open a JIRA to clean those up (and remove this one). Thanks for the review.

[pitrou]

This won't work with Python Decimal nans, right? Try decimal.Decimal('nan').

[pitrou]

What is the policy for adding tests here rather than in pyarrow/tests? It seems writing tests in pure Python is generally easier :-)

[cpcloud]

It's funny you mention that. I agree, but I wanted to be able to step through C++ code in the CLion IDE so I wrote the test here.

[cpcloud]

Let me add Decimal('nan') to this list.

[cpcloud]

Done

[cpcloud]

@wesm@pitrou is there any reason we shouldn't make these static global variables, so we don't have to call these APIs all over the place and inside of loops?

[pitrou]

IMO it's fine to make it a static global (or a singleton, etc.), as long as we don't want to support subinterpreters perhaps.

[pitrou]

By the way the nested block above doesn't seem needed.

[cpcloud]

Yep, okay I'll refactor and make those static

[cpcloud]

Hm so it appears that string interning of module names doesn't play well with C++ globals. I'll leave these as they are for now.

[pitrou]

Right, you probably can't use OwnedRef here since the destructor would trigger too late. But you could have a PyObject*. The only downside is that it would make the object eternal.

[pitrou]

Why the consts? I don't think it makes sense, and you're bound to do a lot of casts as soon as you call the Python C API.

[pitrou]

Note PyObject is non-const pretty much by construction, as it has a reference count that can be mutated by any operation.

[cpcloud]

Ok, I was following convention in the file. I can adjust.

[wesm]

I've been removing all the instances of const PyObject* after having a compiler warning from an internal Py-API, so happy to see them all go

[pitrou]

The last argument can simply be the empty string AFAIR.

[cpcloud]

Cool

[pitrou]

You should call PyDecimal_Checkbefore calling the method. Also you should check if the method raises. And it's better to use PyObject_IsTrue rather than compare against Py_True.

[cpcloud]

Yes, not sure how I missed that :)

[cpcloud]

@pitrou any more comments here?

[pitrou]

See comments below. Also it seems that the AppVeyor build has failed, though it may be unrelated?

[pitrou]

This member doesn't seem used actually. Were you planning to use it with PyObject_IsInstance instead of the costlier call to internal::PyDecimal_Check?

[cpcloud]

Yes, thanks. Will fix.

[pitrou]

Might be nice to add a comment motivating the algorithm here (why compare the absolute values but then memorize the original value)?

[cpcloud]

Actually, reconsidering this based on your comment this really should be just the max scale. Negative scale should contribute to precision only if it would increase precision. The goal here is to "cast the widest net", ie the max precision and max scale. Negative scale complicates things a tiny bit. I'll add some commentary.

[pitrou]

I'm not a C++ expert, so I'm curious why this is necessary?

[cpcloud]

Constructors are not inherited by default. I'm not actually using this, so it should cost nothing at runtime. If we ever wanted to construct one of these with a pointer as it's first argument we'd have to define it anyway. I can remove it if you'd like.

[pitrou]

No problem with me. I had forgotten about non-inheritance of constructors.

[pitrou]

Can PandasObjectIsNull return true on a Decimal instance?

[cpcloud]

No, I'll fix that.

[pitrou]

Also check that the Arrow type was inferred correctly?

[cpcloud]

Yep will do.

[wesm]

needs rebase

[cpcloud]

Closed in favor of #1651

[cpcloud]

No description provided.

[cpcloud]

Restarted the travis jobs, they seemed to be failing during apt-get

[pitrou]

Is this actually required? You are only using the decimal type below.

[cpcloud]

I might be misunderstanding the use case for OwnedRefNoGIL.

My understanding is that if we're Py_XDECREFing something and we do not hold then GIL then we need to acquire it.

The GIL is released in Cython before the function that instantiates this class is called (and isn't subsequently acquired in that function). Assuming my understanding is correct then we need to acquire the GIL before calling Py_XDECREF on this.

Are you suggesting that because the order of destruction here is guaranteed to be the reverse of initialization order that we don't need to care that decimal_module_ holds the GIL because we'll never decref decimal_module_ before we decref decimal_type_?

[pitrou]

What I mean is that you don't care about keeping a reference to the decimal module if you only need to use the decimal type:

>>> D = __import__('decimal').Decimal
>>> sys.modules['decimal'] = None
>>> D(100)
Decimal('100')

[cpcloud]

Oh, awesome. There are a few places that we have unnecessary refs to the decimal module. I'll open a JIRA to clean those up (and remove this one). Thanks for the review.

[pitrou]

This won't work with Python Decimal nans, right? Try decimal.Decimal('nan').

[pitrou]

What is the policy for adding tests here rather than in pyarrow/tests? It seems writing tests in pure Python is generally easier :-)

[cpcloud]

It's funny you mention that. I agree, but I wanted to be able to step through C++ code in the CLion IDE so I wrote the test here.

[cpcloud]

Let me add Decimal('nan') to this list.

[cpcloud]

Done

[cpcloud]

@wesm@pitrou is there any reason we shouldn't make these static global variables, so we don't have to call these APIs all over the place and inside of loops?

[pitrou]

IMO it's fine to make it a static global (or a singleton, etc.), as long as we don't want to support subinterpreters perhaps.

[pitrou]

By the way the nested block above doesn't seem needed.

[cpcloud]

Yep, okay I'll refactor and make those static

[cpcloud]

Hm so it appears that string interning of module names doesn't play well with C++ globals. I'll leave these as they are for now.

[pitrou]

Right, you probably can't use OwnedRef here since the destructor would trigger too late. But you could have a PyObject*. The only downside is that it would make the object eternal.

[pitrou]

Why the consts? I don't think it makes sense, and you're bound to do a lot of casts as soon as you call the Python C API.

[pitrou]

Note PyObject is non-const pretty much by construction, as it has a reference count that can be mutated by any operation.

[cpcloud]

Ok, I was following convention in the file. I can adjust.

[wesm]

I've been removing all the instances of const PyObject* after having a compiler warning from an internal Py-API, so happy to see them all go

[pitrou]

The last argument can simply be the empty string AFAIR.

[cpcloud]

Cool

[pitrou]

You should call PyDecimal_Checkbefore calling the method. Also you should check if the method raises. And it's better to use PyObject_IsTrue rather than compare against Py_True.

[cpcloud]

Yes, not sure how I missed that :)

[cpcloud]

@pitrou any more comments here?

[pitrou]

See comments below. Also it seems that the AppVeyor build has failed, though it may be unrelated?

[pitrou]

This member doesn't seem used actually. Were you planning to use it with PyObject_IsInstance instead of the costlier call to internal::PyDecimal_Check?

[cpcloud]

Yes, thanks. Will fix.

[pitrou]

Might be nice to add a comment motivating the algorithm here (why compare the absolute values but then memorize the original value)?

[cpcloud]

Actually, reconsidering this based on your comment this really should be just the max scale. Negative scale should contribute to precision only if it would increase precision. The goal here is to "cast the widest net", ie the max precision and max scale. Negative scale complicates things a tiny bit. I'll add some commentary.

[pitrou]

I'm not a C++ expert, so I'm curious why this is necessary?

[cpcloud]

Constructors are not inherited by default. I'm not actually using this, so it should cost nothing at runtime. If we ever wanted to construct one of these with a pointer as it's first argument we'd have to define it anyway. I can remove it if you'd like.

[pitrou]

No problem with me. I had forgotten about non-inheritance of constructors.

[pitrou]

Can PandasObjectIsNull return true on a Decimal instance?

[cpcloud]

No, I'll fix that.

[pitrou]

Also check that the Arrow type was inferred correctly?

[cpcloud]

Yep will do.

[wesm]

needs rebase

[cpcloud]

Closed in favor of #1651

[cpcloud]

No description provided.

[cpcloud]

Restarted the travis jobs, they seemed to be failing during apt-get

[pitrou]

Is this actually required? You are only using the decimal type below.

[cpcloud]

I might be misunderstanding the use case for OwnedRefNoGIL.

My understanding is that if we're Py_XDECREFing something and we do not hold then GIL then we need to acquire it.

The GIL is released in Cython before the function that instantiates this class is called (and isn't subsequently acquired in that function). Assuming my understanding is correct then we need to acquire the GIL before calling Py_XDECREF on this.

Are you suggesting that because the order of destruction here is guaranteed to be the reverse of initialization order that we don't need to care that decimal_module_ holds the GIL because we'll never decref decimal_module_ before we decref decimal_type_?

[pitrou]

What I mean is that you don't care about keeping a reference to the decimal module if you only need to use the decimal type:

>>> D = __import__('decimal').Decimal
>>> sys.modules['decimal'] = None
>>> D(100)
Decimal('100')

[cpcloud]

Oh, awesome. There are a few places that we have unnecessary refs to the decimal module. I'll open a JIRA to clean those up (and remove this one). Thanks for the review.

[pitrou]

This won't work with Python Decimal nans, right? Try decimal.Decimal('nan').

[pitrou]

What is the policy for adding tests here rather than in pyarrow/tests? It seems writing tests in pure Python is generally easier :-)

[cpcloud]

It's funny you mention that. I agree, but I wanted to be able to step through C++ code in the CLion IDE so I wrote the test here.

[cpcloud]

Let me add Decimal('nan') to this list.

[cpcloud]

Done

[cpcloud]

@wesm@pitrou is there any reason we shouldn't make these static global variables, so we don't have to call these APIs all over the place and inside of loops?

[pitrou]

IMO it's fine to make it a static global (or a singleton, etc.), as long as we don't want to support subinterpreters perhaps.

[pitrou]

By the way the nested block above doesn't seem needed.

[cpcloud]

Yep, okay I'll refactor and make those static

[cpcloud]

Hm so it appears that string interning of module names doesn't play well with C++ globals. I'll leave these as they are for now.

[pitrou]

Right, you probably can't use OwnedRef here since the destructor would trigger too late. But you could have a PyObject*. The only downside is that it would make the object eternal.

[pitrou]

Why the consts? I don't think it makes sense, and you're bound to do a lot of casts as soon as you call the Python C API.

[pitrou]

Note PyObject is non-const pretty much by construction, as it has a reference count that can be mutated by any operation.

[cpcloud]

Ok, I was following convention in the file. I can adjust.

[wesm]

I've been removing all the instances of const PyObject* after having a compiler warning from an internal Py-API, so happy to see them all go

[pitrou]

The last argument can simply be the empty string AFAIR.

[cpcloud]

Cool

[pitrou]

You should call PyDecimal_Checkbefore calling the method. Also you should check if the method raises. And it's better to use PyObject_IsTrue rather than compare against Py_True.

[cpcloud]

Yes, not sure how I missed that :)

[cpcloud]

@pitrou any more comments here?

[pitrou]

See comments below. Also it seems that the AppVeyor build has failed, though it may be unrelated?

[pitrou]

This member doesn't seem used actually. Were you planning to use it with PyObject_IsInstance instead of the costlier call to internal::PyDecimal_Check?

[cpcloud]

Yes, thanks. Will fix.

[pitrou]

Might be nice to add a comment motivating the algorithm here (why compare the absolute values but then memorize the original value)?

[cpcloud]

Actually, reconsidering this based on your comment this really should be just the max scale. Negative scale should contribute to precision only if it would increase precision. The goal here is to "cast the widest net", ie the max precision and max scale. Negative scale complicates things a tiny bit. I'll add some commentary.

[pitrou]

I'm not a C++ expert, so I'm curious why this is necessary?

[cpcloud]

Constructors are not inherited by default. I'm not actually using this, so it should cost nothing at runtime. If we ever wanted to construct one of these with a pointer as it's first argument we'd have to define it anyway. I can remove it if you'd like.

[pitrou]

No problem with me. I had forgotten about non-inheritance of constructors.

[pitrou]

Can PandasObjectIsNull return true on a Decimal instance?

[cpcloud]

No, I'll fix that.

[pitrou]

Also check that the Arrow type was inferred correctly?

[cpcloud]

Yep will do.

[wesm]

needs rebase

[cpcloud]

Closed in favor of #1651

[cpcloud]

No description provided.

[cpcloud]

Restarted the travis jobs, they seemed to be failing during apt-get

[pitrou]

Is this actually required? You are only using the decimal type below.

[cpcloud]

I might be misunderstanding the use case for OwnedRefNoGIL.

My understanding is that if we're Py_XDECREFing something and we do not hold then GIL then we need to acquire it.

The GIL is released in Cython before the function that instantiates this class is called (and isn't subsequently acquired in that function). Assuming my understanding is correct then we need to acquire the GIL before calling Py_XDECREF on this.

Are you suggesting that because the order of destruction here is guaranteed to be the reverse of initialization order that we don't need to care that decimal_module_ holds the GIL because we'll never decref decimal_module_ before we decref decimal_type_?

[pitrou]

What I mean is that you don't care about keeping a reference to the decimal module if you only need to use the decimal type:

>>> D = __import__('decimal').Decimal
>>> sys.modules['decimal'] = None
>>> D(100)
Decimal('100')

[cpcloud]

Oh, awesome. There are a few places that we have unnecessary refs to the decimal module. I'll open a JIRA to clean those up (and remove this one). Thanks for the review.

[pitrou]

This won't work with Python Decimal nans, right? Try decimal.Decimal('nan').

[pitrou]

What is the policy for adding tests here rather than in pyarrow/tests? It seems writing tests in pure Python is generally easier :-)

[cpcloud]

It's funny you mention that. I agree, but I wanted to be able to step through C++ code in the CLion IDE so I wrote the test here.

[cpcloud]

Let me add Decimal('nan') to this list.

[cpcloud]

Done

[cpcloud]

@wesm@pitrou is there any reason we shouldn't make these static global variables, so we don't have to call these APIs all over the place and inside of loops?

[pitrou]

IMO it's fine to make it a static global (or a singleton, etc.), as long as we don't want to support subinterpreters perhaps.

[pitrou]

By the way the nested block above doesn't seem needed.

[cpcloud]

Yep, okay I'll refactor and make those static

[cpcloud]

Hm so it appears that string interning of module names doesn't play well with C++ globals. I'll leave these as they are for now.

[pitrou]

Right, you probably can't use OwnedRef here since the destructor would trigger too late. But you could have a PyObject*. The only downside is that it would make the object eternal.

[pitrou]

Why the consts? I don't think it makes sense, and you're bound to do a lot of casts as soon as you call the Python C API.

[pitrou]

Note PyObject is non-const pretty much by construction, as it has a reference count that can be mutated by any operation.

[cpcloud]

Ok, I was following convention in the file. I can adjust.

[wesm]

I've been removing all the instances of const PyObject* after having a compiler warning from an internal Py-API, so happy to see them all go

[pitrou]

The last argument can simply be the empty string AFAIR.

[cpcloud]

Cool

[pitrou]

You should call PyDecimal_Checkbefore calling the method. Also you should check if the method raises. And it's better to use PyObject_IsTrue rather than compare against Py_True.

[cpcloud]

Yes, not sure how I missed that :)

[cpcloud]

@pitrou any more comments here?

[pitrou]

See comments below. Also it seems that the AppVeyor build has failed, though it may be unrelated?

[pitrou]

This member doesn't seem used actually. Were you planning to use it with PyObject_IsInstance instead of the costlier call to internal::PyDecimal_Check?

[cpcloud]

Yes, thanks. Will fix.

[pitrou]

Might be nice to add a comment motivating the algorithm here (why compare the absolute values but then memorize the original value)?

[cpcloud]

Actually, reconsidering this based on your comment this really should be just the max scale. Negative scale should contribute to precision only if it would increase precision. The goal here is to "cast the widest net", ie the max precision and max scale. Negative scale complicates things a tiny bit. I'll add some commentary.

[pitrou]

I'm not a C++ expert, so I'm curious why this is necessary?

[cpcloud]

Constructors are not inherited by default. I'm not actually using this, so it should cost nothing at runtime. If we ever wanted to construct one of these with a pointer as it's first argument we'd have to define it anyway. I can remove it if you'd like.

[pitrou]

No problem with me. I had forgotten about non-inheritance of constructors.

[pitrou]

Can PandasObjectIsNull return true on a Decimal instance?

[cpcloud]

No, I'll fix that.

[pitrou]

Also check that the Arrow type was inferred correctly?

[cpcloud]

Yep will do.

[wesm]

needs rebase

[cpcloud]

Closed in favor of #1651

[cpcloud]

No description provided.

[cpcloud]

Restarted the travis jobs, they seemed to be failing during apt-get

[pitrou]

Is this actually required? You are only using the decimal type below.

[cpcloud]

I might be misunderstanding the use case for OwnedRefNoGIL.

My understanding is that if we're Py_XDECREFing something and we do not hold then GIL then we need to acquire it.

The GIL is released in Cython before the function that instantiates this class is called (and isn't subsequently acquired in that function). Assuming my understanding is correct then we need to acquire the GIL before calling Py_XDECREF on this.

Are you suggesting that because the order of destruction here is guaranteed to be the reverse of initialization order that we don't need to care that decimal_module_ holds the GIL because we'll never decref decimal_module_ before we decref decimal_type_?

[pitrou]

What I mean is that you don't care about keeping a reference to the decimal module if you only need to use the decimal type:

>>> D = __import__('decimal').Decimal
>>> sys.modules['decimal'] = None
>>> D(100)
Decimal('100')

[cpcloud]

Oh, awesome. There are a few places that we have unnecessary refs to the decimal module. I'll open a JIRA to clean those up (and remove this one). Thanks for the review.

[pitrou]

This won't work with Python Decimal nans, right? Try decimal.Decimal('nan').

[pitrou]

What is the policy for adding tests here rather than in pyarrow/tests? It seems writing tests in pure Python is generally easier :-)

[cpcloud]

It's funny you mention that. I agree, but I wanted to be able to step through C++ code in the CLion IDE so I wrote the test here.

[cpcloud]

Let me add Decimal('nan') to this list.

[cpcloud]

Done

[cpcloud]

@wesm@pitrou is there any reason we shouldn't make these static global variables, so we don't have to call these APIs all over the place and inside of loops?

[pitrou]

IMO it's fine to make it a static global (or a singleton, etc.), as long as we don't want to support subinterpreters perhaps.

[pitrou]

By the way the nested block above doesn't seem needed.

[cpcloud]

Yep, okay I'll refactor and make those static

[cpcloud]

Hm so it appears that string interning of module names doesn't play well with C++ globals. I'll leave these as they are for now.

[pitrou]

Right, you probably can't use OwnedRef here since the destructor would trigger too late. But you could have a PyObject*. The only downside is that it would make the object eternal.

[pitrou]

Why the consts? I don't think it makes sense, and you're bound to do a lot of casts as soon as you call the Python C API.

[pitrou]

Note PyObject is non-const pretty much by construction, as it has a reference count that can be mutated by any operation.

[cpcloud]

Ok, I was following convention in the file. I can adjust.

[wesm]

I've been removing all the instances of const PyObject* after having a compiler warning from an internal Py-API, so happy to see them all go

[pitrou]

The last argument can simply be the empty string AFAIR.

[cpcloud]

Cool

[pitrou]

You should call PyDecimal_Checkbefore calling the method. Also you should check if the method raises. And it's better to use PyObject_IsTrue rather than compare against Py_True.

[cpcloud]

Yes, not sure how I missed that :)

[cpcloud]

@pitrou any more comments here?

[pitrou]

See comments below. Also it seems that the AppVeyor build has failed, though it may be unrelated?

[pitrou]

This member doesn't seem used actually. Were you planning to use it with PyObject_IsInstance instead of the costlier call to internal::PyDecimal_Check?

[cpcloud]

Yes, thanks. Will fix.

[pitrou]

Might be nice to add a comment motivating the algorithm here (why compare the absolute values but then memorize the original value)?

[cpcloud]

Actually, reconsidering this based on your comment this really should be just the max scale. Negative scale should contribute to precision only if it would increase precision. The goal here is to "cast the widest net", ie the max precision and max scale. Negative scale complicates things a tiny bit. I'll add some commentary.

[pitrou]

I'm not a C++ expert, so I'm curious why this is necessary?

[cpcloud]

Constructors are not inherited by default. I'm not actually using this, so it should cost nothing at runtime. If we ever wanted to construct one of these with a pointer as it's first argument we'd have to define it anyway. I can remove it if you'd like.

[pitrou]

No problem with me. I had forgotten about non-inheritance of constructors.

[pitrou]

Can PandasObjectIsNull return true on a Decimal instance?

[cpcloud]

No, I'll fix that.

[pitrou]

Also check that the Arrow type was inferred correctly?

[cpcloud]

Yep will do.

[wesm]

needs rebase

[cpcloud]

Closed in favor of #1651

[cpcloud]

No description provided.

[cpcloud]

Restarted the travis jobs, they seemed to be failing during apt-get

[pitrou]

Is this actually required? You are only using the decimal type below.

[cpcloud]

I might be misunderstanding the use case for OwnedRefNoGIL.

My understanding is that if we're Py_XDECREFing something and we do not hold then GIL then we need to acquire it.

The GIL is released in Cython before the function that instantiates this class is called (and isn't subsequently acquired in that function). Assuming my understanding is correct then we need to acquire the GIL before calling Py_XDECREF on this.

Are you suggesting that because the order of destruction here is guaranteed to be the reverse of initialization order that we don't need to care that decimal_module_ holds the GIL because we'll never decref decimal_module_ before we decref decimal_type_?

[pitrou]

What I mean is that you don't care about keeping a reference to the decimal module if you only need to use the decimal type:

>>> D = __import__('decimal').Decimal
>>> sys.modules['decimal'] = None
>>> D(100)
Decimal('100')

[cpcloud]

Oh, awesome. There are a few places that we have unnecessary refs to the decimal module. I'll open a JIRA to clean those up (and remove this one). Thanks for the review.

[pitrou]

This won't work with Python Decimal nans, right? Try decimal.Decimal('nan').

[pitrou]

What is the policy for adding tests here rather than in pyarrow/tests? It seems writing tests in pure Python is generally easier :-)

[cpcloud]

It's funny you mention that. I agree, but I wanted to be able to step through C++ code in the CLion IDE so I wrote the test here.

[cpcloud]

Let me add Decimal('nan') to this list.

[cpcloud]

Done

[cpcloud]

@wesm@pitrou is there any reason we shouldn't make these static global variables, so we don't have to call these APIs all over the place and inside of loops?

[pitrou]

IMO it's fine to make it a static global (or a singleton, etc.), as long as we don't want to support subinterpreters perhaps.

[pitrou]

By the way the nested block above doesn't seem needed.

[cpcloud]

Yep, okay I'll refactor and make those static

[cpcloud]

Hm so it appears that string interning of module names doesn't play well with C++ globals. I'll leave these as they are for now.

[pitrou]

Right, you probably can't use OwnedRef here since the destructor would trigger too late. But you could have a PyObject*. The only downside is that it would make the object eternal.

[pitrou]

Why the consts? I don't think it makes sense, and you're bound to do a lot of casts as soon as you call the Python C API.

[pitrou]

Note PyObject is non-const pretty much by construction, as it has a reference count that can be mutated by any operation.

[cpcloud]

Ok, I was following convention in the file. I can adjust.

[wesm]

I've been removing all the instances of const PyObject* after having a compiler warning from an internal Py-API, so happy to see them all go

[pitrou]

The last argument can simply be the empty string AFAIR.

[cpcloud]

Cool

[pitrou]

You should call PyDecimal_Checkbefore calling the method. Also you should check if the method raises. And it's better to use PyObject_IsTrue rather than compare against Py_True.

[cpcloud]

Yes, not sure how I missed that :)

[cpcloud]

@pitrou any more comments here?

[pitrou]

See comments below. Also it seems that the AppVeyor build has failed, though it may be unrelated?

[pitrou]

This member doesn't seem used actually. Were you planning to use it with PyObject_IsInstance instead of the costlier call to internal::PyDecimal_Check?

[cpcloud]

Yes, thanks. Will fix.

[pitrou]

Might be nice to add a comment motivating the algorithm here (why compare the absolute values but then memorize the original value)?

[cpcloud]

Actually, reconsidering this based on your comment this really should be just the max scale. Negative scale should contribute to precision only if it would increase precision. The goal here is to "cast the widest net", ie the max precision and max scale. Negative scale complicates things a tiny bit. I'll add some commentary.

[pitrou]

I'm not a C++ expert, so I'm curious why this is necessary?

[cpcloud]

Constructors are not inherited by default. I'm not actually using this, so it should cost nothing at runtime. If we ever wanted to construct one of these with a pointer as it's first argument we'd have to define it anyway. I can remove it if you'd like.

[pitrou]

No problem with me. I had forgotten about non-inheritance of constructors.

[pitrou]

Can PandasObjectIsNull return true on a Decimal instance?

[cpcloud]

No, I'll fix that.

[pitrou]

Also check that the Arrow type was inferred correctly?

[cpcloud]

Yep will do.

[wesm]

needs rebase

[cpcloud]

Closed in favor of #1651

[cpcloud]

No description provided.

[cpcloud]

Restarted the travis jobs, they seemed to be failing during apt-get

[pitrou]

Is this actually required? You are only using the decimal type below.

[cpcloud]

I might be misunderstanding the use case for OwnedRefNoGIL.

My understanding is that if we're Py_XDECREFing something and we do not hold then GIL then we need to acquire it.

The GIL is released in Cython before the function that instantiates this class is called (and isn't subsequently acquired in that function). Assuming my understanding is correct then we need to acquire the GIL before calling Py_XDECREF on this.

Are you suggesting that because the order of destruction here is guaranteed to be the reverse of initialization order that we don't need to care that decimal_module_ holds the GIL because we'll never decref decimal_module_ before we decref decimal_type_?

[pitrou]

What I mean is that you don't care about keeping a reference to the decimal module if you only need to use the decimal type:

>>> D = __import__('decimal').Decimal
>>> sys.modules['decimal'] = None
>>> D(100)
Decimal('100')

[cpcloud]

Oh, awesome. There are a few places that we have unnecessary refs to the decimal module. I'll open a JIRA to clean those up (and remove this one). Thanks for the review.

[pitrou]

This won't work with Python Decimal nans, right? Try decimal.Decimal('nan').

[pitrou]

What is the policy for adding tests here rather than in pyarrow/tests? It seems writing tests in pure Python is generally easier :-)

[cpcloud]

It's funny you mention that. I agree, but I wanted to be able to step through C++ code in the CLion IDE so I wrote the test here.

[cpcloud]

Let me add Decimal('nan') to this list.

[cpcloud]

Done

[cpcloud]

@wesm@pitrou is there any reason we shouldn't make these static global variables, so we don't have to call these APIs all over the place and inside of loops?

[pitrou]

IMO it's fine to make it a static global (or a singleton, etc.), as long as we don't want to support subinterpreters perhaps.

[pitrou]

By the way the nested block above doesn't seem needed.

[cpcloud]

Yep, okay I'll refactor and make those static

[cpcloud]

Hm so it appears that string interning of module names doesn't play well with C++ globals. I'll leave these as they are for now.

[pitrou]

Right, you probably can't use OwnedRef here since the destructor would trigger too late. But you could have a PyObject*. The only downside is that it would make the object eternal.

[pitrou]

Why the consts? I don't think it makes sense, and you're bound to do a lot of casts as soon as you call the Python C API.

[pitrou]

Note PyObject is non-const pretty much by construction, as it has a reference count that can be mutated by any operation.

[cpcloud]

Ok, I was following convention in the file. I can adjust.

[wesm]

I've been removing all the instances of const PyObject* after having a compiler warning from an internal Py-API, so happy to see them all go

[pitrou]

The last argument can simply be the empty string AFAIR.

[cpcloud]

Cool

[pitrou]

You should call PyDecimal_Checkbefore calling the method. Also you should check if the method raises. And it's better to use PyObject_IsTrue rather than compare against Py_True.

[cpcloud]

Yes, not sure how I missed that :)

[cpcloud]

@pitrou any more comments here?

[pitrou]

See comments below. Also it seems that the AppVeyor build has failed, though it may be unrelated?

[pitrou]

This member doesn't seem used actually. Were you planning to use it with PyObject_IsInstance instead of the costlier call to internal::PyDecimal_Check?

[cpcloud]

Yes, thanks. Will fix.

[pitrou]

Might be nice to add a comment motivating the algorithm here (why compare the absolute values but then memorize the original value)?

[cpcloud]

Actually, reconsidering this based on your comment this really should be just the max scale. Negative scale should contribute to precision only if it would increase precision. The goal here is to "cast the widest net", ie the max precision and max scale. Negative scale complicates things a tiny bit. I'll add some commentary.

[pitrou]

I'm not a C++ expert, so I'm curious why this is necessary?

[cpcloud]

Constructors are not inherited by default. I'm not actually using this, so it should cost nothing at runtime. If we ever wanted to construct one of these with a pointer as it's first argument we'd have to define it anyway. I can remove it if you'd like.

[pitrou]

No problem with me. I had forgotten about non-inheritance of constructors.

[pitrou]

Can PandasObjectIsNull return true on a Decimal instance?

[cpcloud]

No, I'll fix that.

[pitrou]

Also check that the Arrow type was inferred correctly?

[cpcloud]

Yep will do.

[wesm]

needs rebase

[cpcloud]

Closed in favor of #1651