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 2021/07/08 09:00:57 UTC
[GitHub] [arrow] edponce opened a new pull request #10683: ARROW-13288: [Python] Missing default values of kernel options in PyArrow
edponce opened a new pull request #10683:
URL: https://github.com/apache/arrow/pull/10683
This PR ensures that default values for kernel options in PyArrow are consistent with the C++ defaults.
--
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 pull request #10683: ARROW-13288: [Python] Missing default values of kernel options in PyArrow
Posted by GitBox <gi...@apache.org>.
edponce commented on pull request #10683:
URL: https://github.com/apache/arrow/pull/10683#issuecomment-879172919
@amol- The issue I find with `None` is that it doesn't provide any useful info w.r.t. to what value it is representing. Consider the case where a default C++ option is `std::numeric_limits<int64_t>::min()` then mapping `None` to also mean `-sys.maxsize-1` is inconsistent. Maybe another approach is to use `None` in Python for all options with a default value in C++, and let the C++ layer establish the actual default value.
--
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 edited a comment on pull request #10683: ARROW-13288: [Python] Missing default values of kernel options in PyArrow
Posted by GitBox <gi...@apache.org>.
edponce edited a comment on pull request #10683:
URL: https://github.com/apache/arrow/pull/10683#issuecomment-879185656
Related to this PR and discussion, some default values in Cython are explicitly typed but others do not (e.g., `int64_t pivot` in [`PartitionNthOptions`](https://github.com/apache/arrow/blob/master/python/pyarrow/_compute.pyx#L858). Why are not all options in Cython explicitly typed?
--
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 edited a comment on pull request #10683: ARROW-13288: [Python] Missing default values of kernel options in PyArrow
Posted by GitBox <gi...@apache.org>.
edponce edited a comment on pull request #10683:
URL: https://github.com/apache/arrow/pull/10683#issuecomment-879172919
@amol- The issue I find with `None` is that it doesn't provide any useful info w.r.t. to what value it is representing. Consider the case where a default C++ option is `std::numeric_limits<int64_t>::min()` then mapping `None` to also mean `-sys.maxsize-1` is inconsistent. Maybe another approach is to use `None` in Python for all options with a default value in C++, and let the C++ layer establish the actual default value. But using `None` will not allow viewing the default values in the exposed options signatures (e.g., when using Python's `help` command).
For this PR, I added explicit type `int64_t` to the range limits of `SliceOptions`.
cc @bkietz
--
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] bkietz closed pull request #10683: ARROW-13288: [Python] Missing default values of kernel options in PyArrow
Posted by GitBox <gi...@apache.org>.
bkietz closed pull request #10683:
URL: https://github.com/apache/arrow/pull/10683
--
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 edited a comment on pull request #10683: ARROW-13288: [Python] Missing default values of kernel options in PyArrow
Posted by GitBox <gi...@apache.org>.
edponce edited a comment on pull request #10683:
URL: https://github.com/apache/arrow/pull/10683#issuecomment-879185656
Related to this PR and discussion, some default values in Cython are explicitly typed but others do not (e.g., `int64_t pivot` in [`PartitionNthOptions`](https://github.com/apache/arrow/blob/master/python/pyarrow/_compute.pyx#L858). Why are not all options in Cython explicitly typed?
My recommendation is to provide explicit types to the function options in both class levels: the C extension and the Python derived class.
--
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 edited a comment on pull request #10683: ARROW-13288: [Python] Missing default values of kernel options in PyArrow
Posted by GitBox <gi...@apache.org>.
edponce edited a comment on pull request #10683:
URL: https://github.com/apache/arrow/pull/10683#issuecomment-879172919
@amol- The issue I find with `None` is that it doesn't provide any useful info w.r.t. to what value it is representing. Consider the case where a default C++ option is `std::numeric_limits<int64_t>::min()` then mapping `None` to also mean `-sys.maxsize-1` is inconsistent. Maybe another approach is to use `None` in Python for all options with a default value in C++, and let the C++ layer establish the actual default value. But using `None` only will not allow viewing the default values with Python's `help` command.
cc @bkietz
--
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 edited a comment on pull request #10683: ARROW-13288: [Python] Missing default values of kernel options in PyArrow
Posted by GitBox <gi...@apache.org>.
edponce edited a comment on pull request #10683:
URL: https://github.com/apache/arrow/pull/10683#issuecomment-879172919
@amol- The issue I find with `None` is that it doesn't provide any useful info w.r.t. to what value it is representing. Consider the case where a default C++ option is `std::numeric_limits<int64_t>::min()` then mapping `None` to also mean `-sys.maxsize-1` is inconsistent. Maybe another approach is to use `None` in Python for all options with a default value in C++, and let the C++ layer establish the actual default value.
cc @bkietz
--
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 edited a comment on pull request #10683: ARROW-13288: [Python] Missing default values of kernel options in PyArrow
Posted by GitBox <gi...@apache.org>.
edponce edited a comment on pull request #10683:
URL: https://github.com/apache/arrow/pull/10683#issuecomment-879172919
@amol- The issue I find with `None` is that it doesn't provide any useful info w.r.t. to what value it is representing. Consider the case where a default C++ option is `std::numeric_limits<int64_t>::min()` then mapping `None` to also mean `-sys.maxsize-1` is inconsistent. Maybe another approach is to use `None` in Python for all options with a default value in C++, and let the C++ layer establish the actual default value. But using `None` will not allow viewing the default values in the exposed options signatures (e.g., when using Python's `help` command).
cc @bkietz
--
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 #10683: ARROW-13288: [Python] Missing default values of kernel options in PyArrow
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10683:
URL: https://github.com/apache/arrow/pull/10683#issuecomment-876264532
https://issues.apache.org/jira/browse/ARROW-13288
--
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 edited a comment on pull request #10683: ARROW-13288: [Python] Missing default values of kernel options in PyArrow
Posted by GitBox <gi...@apache.org>.
edponce edited a comment on pull request #10683:
URL: https://github.com/apache/arrow/pull/10683#issuecomment-879185656
Related to this PR and discussion, some default values in CPython are explicitly typed but others do not (e.g., `int64_t pivot` in [`PartitionNthOptions`](https://github.com/apache/arrow/blob/master/python/pyarrow/_compute.pyx#L858). Why are not all options in CPython explicitly typed?
--
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 edited a comment on pull request #10683: ARROW-13288: [Python] Missing default values of kernel options in PyArrow
Posted by GitBox <gi...@apache.org>.
edponce edited a comment on pull request #10683:
URL: https://github.com/apache/arrow/pull/10683#issuecomment-879185656
Related to this PR and discussion, some default values in Cython are explicitly typed but others do not (e.g., `int64_t pivot` in [`PartitionNthOptions`](https://github.com/apache/arrow/blob/master/python/pyarrow/_compute.pyx#L858). Why are not all options in Cython explicitly typed?
My recommendation is to provide explicit types to the function options in both class levels: the C extension and the Python derived class. I created [ARROW-13327](https://issues.apache.org/jira/browse/ARROW-13327) to help address strict typing in PyArrow bindings.
--
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 pull request #10683: ARROW-13288: [Python] Missing default values of kernel options in PyArrow
Posted by GitBox <gi...@apache.org>.
edponce commented on pull request #10683:
URL: https://github.com/apache/arrow/pull/10683#issuecomment-879185656
Related to this PR and discussion, some default values in CPython are typed with `int64_t` see [`PartitionNthOptions`](https://github.com/apache/arrow/blob/master/python/pyarrow/_compute.pyx#L858). Also, why are not all options in CPython explicitly typed?
--
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