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 2022/03/17 21:36:22 UTC
[GitHub] [arrow] edponce opened a new pull request #12660: ARROW-15965: [Python] Add Scalar constructor of RoundToMultipleOptions to Python
edponce opened a new pull request #12660:
URL: https://github.com/apache/arrow/pull/12660
Expose in the Python bindings constructor for RoundToMultipleOptions with a shared_ptr<Scalar>.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a change in pull request #12660: ARROW-15965: [Python] Add Scalar constructor of RoundToMultipleOptions to Python
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #12660:
URL: https://github.com/apache/arrow/pull/12660#discussion_r829975339
##########
File path: python/pyarrow/_compute.pyx
##########
@@ -898,10 +898,16 @@ class RoundTemporalOptions(_RoundTemporalOptions):
cdef class _RoundToMultipleOptions(FunctionOptions):
def _set_options(self, multiple, round_mode):
- self.wrapped.reset(
- new CRoundToMultipleOptions(multiple,
- unwrap_round_mode(round_mode))
- )
+ if isinstance(multiple, Scalar):
+ self.wrapped.reset(
+ new CRoundToMultipleOptions(pyarrow_unwrap_scalar(multiple),
+ unwrap_round_mode(round_mode))
+ )
+ else:
+ self.wrapped.reset(
+ new CRoundToMultipleOptions((<double> multiple),
Review comment:
Hmm, do we need to do more type checking here? What happens if `multiple` is neither scalar nor double?
##########
File path: python/pyarrow/_compute.pyx
##########
@@ -898,10 +898,16 @@ class RoundTemporalOptions(_RoundTemporalOptions):
cdef class _RoundToMultipleOptions(FunctionOptions):
def _set_options(self, multiple, round_mode):
- self.wrapped.reset(
- new CRoundToMultipleOptions(multiple,
- unwrap_round_mode(round_mode))
- )
+ if isinstance(multiple, Scalar):
+ self.wrapped.reset(
+ new CRoundToMultipleOptions(pyarrow_unwrap_scalar(multiple),
+ unwrap_round_mode(round_mode))
+ )
+ else:
+ self.wrapped.reset(
+ new CRoundToMultipleOptions((<double> multiple),
Review comment:
Should we add a unit test too?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #12660: ARROW-15965: [Python] Add Scalar constructor of RoundToMultipleOptions to Python
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12660:
URL: https://github.com/apache/arrow/pull/12660#issuecomment-1071528853
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] edponce commented on a change in pull request #12660: ARROW-15965: [Python] Add Scalar constructor of RoundToMultipleOptions to Python
Posted by GitBox <gi...@apache.org>.
edponce commented on a change in pull request #12660:
URL: https://github.com/apache/arrow/pull/12660#discussion_r830022660
##########
File path: python/pyarrow/_compute.pyx
##########
@@ -898,10 +898,16 @@ class RoundTemporalOptions(_RoundTemporalOptions):
cdef class _RoundToMultipleOptions(FunctionOptions):
def _set_options(self, multiple, round_mode):
- self.wrapped.reset(
- new CRoundToMultipleOptions(multiple,
- unwrap_round_mode(round_mode))
- )
+ if isinstance(multiple, Scalar):
+ self.wrapped.reset(
+ new CRoundToMultipleOptions(pyarrow_unwrap_scalar(multiple),
+ unwrap_round_mode(round_mode))
+ )
+ else:
+ self.wrapped.reset(
+ new CRoundToMultipleOptions((<double> multiple),
Review comment:
Agree, needs improved type support. Thanks!
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] edponce commented on a change in pull request #12660: ARROW-15965: [Python] Add Scalar constructor of RoundToMultipleOptions to Python
Posted by GitBox <gi...@apache.org>.
edponce commented on a change in pull request #12660:
URL: https://github.com/apache/arrow/pull/12660#discussion_r830022660
##########
File path: python/pyarrow/_compute.pyx
##########
@@ -898,10 +898,16 @@ class RoundTemporalOptions(_RoundTemporalOptions):
cdef class _RoundToMultipleOptions(FunctionOptions):
def _set_options(self, multiple, round_mode):
- self.wrapped.reset(
- new CRoundToMultipleOptions(multiple,
- unwrap_round_mode(round_mode))
- )
+ if isinstance(multiple, Scalar):
+ self.wrapped.reset(
+ new CRoundToMultipleOptions(pyarrow_unwrap_scalar(multiple),
+ unwrap_round_mode(round_mode))
+ )
+ else:
+ self.wrapped.reset(
+ new CRoundToMultipleOptions((<double> multiple),
Review comment:
Agree, needs improved type support. Thanks!
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #12660: ARROW-15965: [Python] Add Scalar constructor of RoundToMultipleOptions to Python
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12660:
URL: https://github.com/apache/arrow/pull/12660#issuecomment-1071528853
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a change in pull request #12660: ARROW-15965: [Python] Add Scalar constructor of RoundToMultipleOptions to Python
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #12660:
URL: https://github.com/apache/arrow/pull/12660#discussion_r829975339
##########
File path: python/pyarrow/_compute.pyx
##########
@@ -898,10 +898,16 @@ class RoundTemporalOptions(_RoundTemporalOptions):
cdef class _RoundToMultipleOptions(FunctionOptions):
def _set_options(self, multiple, round_mode):
- self.wrapped.reset(
- new CRoundToMultipleOptions(multiple,
- unwrap_round_mode(round_mode))
- )
+ if isinstance(multiple, Scalar):
+ self.wrapped.reset(
+ new CRoundToMultipleOptions(pyarrow_unwrap_scalar(multiple),
+ unwrap_round_mode(round_mode))
+ )
+ else:
+ self.wrapped.reset(
+ new CRoundToMultipleOptions((<double> multiple),
Review comment:
Hmm, do we need to do more type checking here? What happens if `multiple` is neither scalar nor double?
##########
File path: python/pyarrow/_compute.pyx
##########
@@ -898,10 +898,16 @@ class RoundTemporalOptions(_RoundTemporalOptions):
cdef class _RoundToMultipleOptions(FunctionOptions):
def _set_options(self, multiple, round_mode):
- self.wrapped.reset(
- new CRoundToMultipleOptions(multiple,
- unwrap_round_mode(round_mode))
- )
+ if isinstance(multiple, Scalar):
+ self.wrapped.reset(
+ new CRoundToMultipleOptions(pyarrow_unwrap_scalar(multiple),
+ unwrap_round_mode(round_mode))
+ )
+ else:
+ self.wrapped.reset(
+ new CRoundToMultipleOptions((<double> multiple),
Review comment:
Should we add a unit test too?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] edponce commented on a change in pull request #12660: ARROW-15965: [Python] Add Scalar constructor of RoundToMultipleOptions to Python
Posted by GitBox <gi...@apache.org>.
edponce commented on a change in pull request #12660:
URL: https://github.com/apache/arrow/pull/12660#discussion_r831627984
##########
File path: python/pyarrow/_compute.pyx
##########
@@ -898,10 +898,16 @@ class RoundTemporalOptions(_RoundTemporalOptions):
cdef class _RoundToMultipleOptions(FunctionOptions):
def _set_options(self, multiple, round_mode):
- self.wrapped.reset(
- new CRoundToMultipleOptions(multiple,
- unwrap_round_mode(round_mode))
- )
+ if isinstance(multiple, Scalar):
+ self.wrapped.reset(
+ new CRoundToMultipleOptions(pyarrow_unwrap_scalar(multiple),
+ unwrap_round_mode(round_mode))
+ )
+ else:
+ self.wrapped.reset(
+ new CRoundToMultipleOptions(<double> multiple,
+ unwrap_round_mode(round_mode))
+ )
Review comment:
After testing and investigating further, I consider the above code is sufficient and is consistent with the other `FunctionOptions` definitions. In the current state, any non-scalar `multiple` that can be casted to `<double>` is valid (e.g., integers, floats, bools). Those values that cannot be coerced trigger the following Python error:
```python
>>> pc.round_to_multiple(2.1, options=pc.RoundToMultipleOptions('one'))
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "pyarrow/_compute.pyx", line 930, in pyarrow._compute.RoundToMultipleOptions.__init__
File "pyarrow/_compute.pyx", line 908, in pyarrow._compute._RoundToMultipleOptions._set_options
TypeError: must be real number, not str
>>> asin('one')
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: must be real number, not str
```
This is the same behavior as the C++ API which has a constructor with a `double` parameter. Python, C++, and R allow casting boolean to floating-point.
Also, the above `_set_options()` follows the same pattern as all other `FunctionOptions` (and C++) where no type checking in addition to the language's is performed. I do not think we need tests for this one case only.
cc @lidavidm @jorisvandenbossche
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] edponce commented on a change in pull request #12660: ARROW-15965: [Python] Add Scalar constructor of RoundToMultipleOptions to Python
Posted by GitBox <gi...@apache.org>.
edponce commented on a change in pull request #12660:
URL: https://github.com/apache/arrow/pull/12660#discussion_r831875969
##########
File path: python/pyarrow/_compute.pyx
##########
@@ -898,10 +898,16 @@ class RoundTemporalOptions(_RoundTemporalOptions):
cdef class _RoundToMultipleOptions(FunctionOptions):
def _set_options(self, multiple, round_mode):
- self.wrapped.reset(
- new CRoundToMultipleOptions(multiple,
- unwrap_round_mode(round_mode))
- )
+ if isinstance(multiple, Scalar):
+ self.wrapped.reset(
+ new CRoundToMultipleOptions(pyarrow_unwrap_scalar(multiple),
+ unwrap_round_mode(round_mode))
+ )
+ else:
+ self.wrapped.reset(
+ new CRoundToMultipleOptions(<double> multiple,
+ unwrap_round_mode(round_mode))
+ )
Review comment:
Oh, I see. I will add some tests. Thanks!
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorisvandenbossche commented on a change in pull request #12660: ARROW-15965: [Python] Add Scalar constructor of RoundToMultipleOptions to Python
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on a change in pull request #12660:
URL: https://github.com/apache/arrow/pull/12660#discussion_r831836585
##########
File path: python/pyarrow/_compute.pyx
##########
@@ -898,10 +898,16 @@ class RoundTemporalOptions(_RoundTemporalOptions):
cdef class _RoundToMultipleOptions(FunctionOptions):
def _set_options(self, multiple, round_mode):
- self.wrapped.reset(
- new CRoundToMultipleOptions(multiple,
- unwrap_round_mode(round_mode))
- )
+ if isinstance(multiple, Scalar):
+ self.wrapped.reset(
+ new CRoundToMultipleOptions(pyarrow_unwrap_scalar(multiple),
+ unwrap_round_mode(round_mode))
+ )
+ else:
+ self.wrapped.reset(
+ new CRoundToMultipleOptions(<double> multiple,
+ unwrap_round_mode(round_mode))
+ )
Review comment:
It might not be strictly needed to test that a string errors (this already happens on master as well), but you needed to add additional code to enable a new feature: being able to pass a pyarrow scalar. So to ensure this actually works and keeps working, this requires a python test (note that being able to pass a pyarrow scalar is not something that other option classes actually support in general).
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org