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/11/30 16:44:13 UTC
[GitHub] [arrow] jorisvandenbossche opened a new pull request #8805: ARROW-10725: [Python][Compute] Expose sort options in Python bindings
jorisvandenbossche opened a new pull request #8805:
URL: https://github.com/apache/arrow/pull/8805
----------------------------------------------------------------
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 pull request #8805: ARROW-10725: [Python][Compute] Expose sort options in Python bindings
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on pull request #8805:
URL: https://github.com/apache/arrow/pull/8805#issuecomment-754858007
Rebased, so if it passes, this should be ready for another review / merge.
----------------------------------------------------------------
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 #8805: ARROW-10725: [Python][Compute] Expose sort options in Python bindings
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on a change in pull request #8805:
URL: https://github.com/apache/arrow/pull/8805#discussion_r542503495
##########
File path: python/pyarrow/includes/libarrow.pxd
##########
@@ -1788,6 +1788,25 @@ cdef extern from "arrow/compute/api.h" namespace "arrow::compute" nogil:
CPartitionNthOptions(int64_t pivot)
int64_t pivot
+ enum CSortOrder" arrow::compute::SortOrder":
Review comment:
Ah, I only looked at the 32bit failure, which seemed unrelated. Will push a fix.
----------------------------------------------------------------
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 #8805: ARROW-10725: [Python][Compute] Expose sort options in Python bindings
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8805:
URL: https://github.com/apache/arrow/pull/8805#discussion_r542433177
##########
File path: python/pyarrow/includes/libarrow.pxd
##########
@@ -1788,6 +1788,25 @@ cdef extern from "arrow/compute/api.h" namespace "arrow::compute" nogil:
CPartitionNthOptions(int64_t pivot)
int64_t pivot
+ enum CSortOrder" arrow::compute::SortOrder":
Review comment:
I think this should be `ctypedef enum` because `arrow::compute::SortOrder` is a scoped enum (it's declared as `enum class SortOrder`).
Hopefully that'd fix the compile error on CI.
----------------------------------------------------------------
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 #8805: ARROW-10725: [Python][Compute] Expose sort options in Python bindings
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8805:
URL: https://github.com/apache/arrow/pull/8805#discussion_r542441007
##########
File path: python/pyarrow/tests/test_compute.py
##########
@@ -1017,3 +1017,51 @@ def test_partition_nth():
for i in range(pivot))
assert all(data[indices[i]] >= data[indices[pivot]]
for i in range(pivot, len(data)))
+
+
+def test_array_sort_indices():
+ arr = pa.array([1, 2, None, 0])
+ result = pc.array_sort_indices(arr)
+ assert result.to_pylist() == [3, 0, 1, 2]
+ result = pc.array_sort_indices(arr, order="ascending")
+ assert result.to_pylist() == [3, 0, 1, 2]
+ result = pc.array_sort_indices(arr, order="descending")
+ assert result.to_pylist() == [1, 0, 3, 2]
+
+ with pytest.raises(ValueError, match="not a valid order"):
+ pc.array_sort_indices(arr, order="nonscending")
+
+
+def test_sort_indices_array():
+ arr = pa.array([1, 2, None, 0])
+ result = pc.sort_indices(arr)
+ assert result.to_pylist() == [3, 0, 1, 2]
+ result = pc.sort_indices(arr, sort_keys=[("dummy", "ascending")])
Review comment:
Just thinking out loud, but another possibility would be `sort_keys=["a", "-b"]` for `[("a", "ascending"), ("b", "descending")]`. Not sure how that feels...
cc @xhochy for opinions.
##########
File path: python/pyarrow/includes/libarrow.pxd
##########
@@ -1788,6 +1788,25 @@ cdef extern from "arrow/compute/api.h" namespace "arrow::compute" nogil:
CPartitionNthOptions(int64_t pivot)
int64_t pivot
+ enum CSortOrder" arrow::compute::SortOrder":
Review comment:
I think this should be `ctypedef enum` because `arrow::compute::SortOrder` is a scoped num (it's declared as `enum class SortOrder`.
Hopefully that'd fix the compile error on CI.
##########
File path: python/pyarrow/tests/test_compute.py
##########
@@ -1017,3 +1017,51 @@ def test_partition_nth():
for i in range(pivot))
assert all(data[indices[i]] >= data[indices[pivot]]
for i in range(pivot, len(data)))
+
+
+def test_array_sort_indices():
+ arr = pa.array([1, 2, None, 0])
+ result = pc.array_sort_indices(arr)
+ assert result.to_pylist() == [3, 0, 1, 2]
+ result = pc.array_sort_indices(arr, order="ascending")
+ assert result.to_pylist() == [3, 0, 1, 2]
+ result = pc.array_sort_indices(arr, order="descending")
+ assert result.to_pylist() == [1, 0, 3, 2]
+
+ with pytest.raises(ValueError, match="not a valid order"):
+ pc.array_sort_indices(arr, order="nonscending")
+
+
+def test_sort_indices_array():
+ arr = pa.array([1, 2, None, 0])
+ result = pc.sort_indices(arr)
+ assert result.to_pylist() == [3, 0, 1, 2]
+ result = pc.sort_indices(arr, sort_keys=[("dummy", "ascending")])
Review comment:
It's a bit of a pity to have such a verbose convention (having to type "ascending" for every sort key). Apparently [pandas](https://pandas.pydata.org/pandas-docs/stable/reference/api/pandas.DataFrame.sort_values.html) uses a separate `ascending` argument with is a list of bools, though that's not very pretty either...
----------------------------------------------------------------
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] kou closed pull request #8805: ARROW-10725: [Python][Compute] Expose sort options in Python bindings
Posted by GitBox <gi...@apache.org>.
kou closed pull request #8805:
URL: https://github.com/apache/arrow/pull/8805
----------------------------------------------------------------
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 #8805: ARROW-10725: [Python][Compute] Expose sort options in Python bindings
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8805:
URL: https://github.com/apache/arrow/pull/8805#issuecomment-735916541
https://issues.apache.org/jira/browse/ARROW-10725
----------------------------------------------------------------
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 pull request #8805: ARROW-10725: [Python][Compute] Expose sort options in Python bindings
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on pull request #8805:
URL: https://github.com/apache/arrow/pull/8805#issuecomment-756206333
Opened https://issues.apache.org/jira/browse/ARROW-11167 to track improving the usability for those options
----------------------------------------------------------------
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 pull request #8805: ARROW-10725: [Python][Compute] Expose sort options in Python bindings
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #8805:
URL: https://github.com/apache/arrow/pull/8805#issuecomment-754840680
> We probably want to support the current "verbose" way of writing the options anyway? In which case it might be fine to defer a more ergonomic API to a follow-up JIRA (since this needs to get in this week)
Sounds fine.
----------------------------------------------------------------
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 pull request #8805: ARROW-10725: [Python][Compute] Expose sort options in Python bindings
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #8805:
URL: https://github.com/apache/arrow/pull/8805#issuecomment-754816122
@jorisvandenbossche Is this ready for review again?
----------------------------------------------------------------
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 #8805: ARROW-10725: [Python][Compute] Expose sort options in Python bindings
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8805:
URL: https://github.com/apache/arrow/pull/8805#discussion_r544344212
##########
File path: python/pyarrow/tests/test_compute.py
##########
@@ -1017,3 +1017,51 @@ def test_partition_nth():
for i in range(pivot))
assert all(data[indices[i]] >= data[indices[pivot]]
for i in range(pivot, len(data)))
+
+
+def test_array_sort_indices():
+ arr = pa.array([1, 2, None, 0])
+ result = pc.array_sort_indices(arr)
+ assert result.to_pylist() == [3, 0, 1, 2]
+ result = pc.array_sort_indices(arr, order="ascending")
+ assert result.to_pylist() == [3, 0, 1, 2]
+ result = pc.array_sort_indices(arr, order="descending")
+ assert result.to_pylist() == [1, 0, 3, 2]
+
+ with pytest.raises(ValueError, match="not a valid order"):
+ pc.array_sort_indices(arr, order="nonscending")
+
+
+def test_sort_indices_array():
+ arr = pa.array([1, 2, None, 0])
+ result = pc.sort_indices(arr)
+ assert result.to_pylist() == [3, 0, 1, 2]
+ result = pc.sort_indices(arr, sort_keys=[("dummy", "ascending")])
Review comment:
Well, we do need some kind of shortcut notation, whatever it is, IMHO. Having to repeat "ascending" or "descending" is much too cumbersome.
----------------------------------------------------------------
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 #8805: ARROW-10725: [Python][Compute] Expose sort options in Python bindings
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on a change in pull request #8805:
URL: https://github.com/apache/arrow/pull/8805#discussion_r532839071
##########
File path: python/pyarrow/compute.py
##########
@@ -155,7 +157,7 @@ def _handle_options(name, option_class, options, kwargs):
"Function {!r} expected a {} parameter, got {}"
.format(name, option_class, type(options)))
- return options
+ return option_class()
Review comment:
(you basically get here when `options` is None, and there are also no `**kwargs`)
This seems a sensible change to me (use the default constructor of the options class when no options are specified), but I think it might also be an indication that on the C++ side the sort_indices kernel has not a default options set, which is something we might want to fix on the C++ side? (since all other classes seem to have this, and can be called without options class specified)
Now, this change also causes some failure in another test (one that tests the error message with wrong number of arguments, because it now errors here first). So if we keep it like this, I need to update that test
----------------------------------------------------------------
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 #8805: ARROW-10725: [Python][Compute] Expose sort options in Python bindings
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on a change in pull request #8805:
URL: https://github.com/apache/arrow/pull/8805#discussion_r544417190
##########
File path: python/pyarrow/tests/test_compute.py
##########
@@ -1017,3 +1017,51 @@ def test_partition_nth():
for i in range(pivot))
assert all(data[indices[i]] >= data[indices[pivot]]
for i in range(pivot, len(data)))
+
+
+def test_array_sort_indices():
+ arr = pa.array([1, 2, None, 0])
+ result = pc.array_sort_indices(arr)
+ assert result.to_pylist() == [3, 0, 1, 2]
+ result = pc.array_sort_indices(arr, order="ascending")
+ assert result.to_pylist() == [3, 0, 1, 2]
+ result = pc.array_sort_indices(arr, order="descending")
+ assert result.to_pylist() == [1, 0, 3, 2]
+
+ with pytest.raises(ValueError, match="not a valid order"):
+ pc.array_sort_indices(arr, order="nonscending")
+
+
+def test_sort_indices_array():
+ arr = pa.array([1, 2, None, 0])
+ result = pc.sort_indices(arr)
+ assert result.to_pylist() == [3, 0, 1, 2]
+ result = pc.sort_indices(arr, sort_keys=[("dummy", "ascending")])
Review comment:
> Having to repeat "ascending" or "descending" is much too cumbersome.
Also if you have different order for different columns? (assuming that the all-ascending or all-descending cases would be covered by a shortcut like an `order` keyword mentioned above)
(I would personally be fine with leaving that case as cumbersome to write, 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] jorisvandenbossche commented on a change in pull request #8805: ARROW-10725: [Python][Compute] Expose sort options in Python bindings
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on a change in pull request #8805:
URL: https://github.com/apache/arrow/pull/8805#discussion_r544417190
##########
File path: python/pyarrow/tests/test_compute.py
##########
@@ -1017,3 +1017,51 @@ def test_partition_nth():
for i in range(pivot))
assert all(data[indices[i]] >= data[indices[pivot]]
for i in range(pivot, len(data)))
+
+
+def test_array_sort_indices():
+ arr = pa.array([1, 2, None, 0])
+ result = pc.array_sort_indices(arr)
+ assert result.to_pylist() == [3, 0, 1, 2]
+ result = pc.array_sort_indices(arr, order="ascending")
+ assert result.to_pylist() == [3, 0, 1, 2]
+ result = pc.array_sort_indices(arr, order="descending")
+ assert result.to_pylist() == [1, 0, 3, 2]
+
+ with pytest.raises(ValueError, match="not a valid order"):
+ pc.array_sort_indices(arr, order="nonscending")
+
+
+def test_sort_indices_array():
+ arr = pa.array([1, 2, None, 0])
+ result = pc.sort_indices(arr)
+ assert result.to_pylist() == [3, 0, 1, 2]
+ result = pc.sort_indices(arr, sort_keys=[("dummy", "ascending")])
Review comment:
> Having to repeat "ascending" or "descending" is much too cumbersome.
Also if you have different order for different columns? (assuming that the all-ascending or all-descending cases would be covered by a shortcut like an `order` keyword mentioned above)
----------------------------------------------------------------
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 #8805: ARROW-10725: [Python][Compute] Expose sort options in Python bindings
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on a change in pull request #8805:
URL: https://github.com/apache/arrow/pull/8805#discussion_r542510825
##########
File path: python/pyarrow/tests/test_compute.py
##########
@@ -1017,3 +1017,51 @@ def test_partition_nth():
for i in range(pivot))
assert all(data[indices[i]] >= data[indices[pivot]]
for i in range(pivot, len(data)))
+
+
+def test_array_sort_indices():
+ arr = pa.array([1, 2, None, 0])
+ result = pc.array_sort_indices(arr)
+ assert result.to_pylist() == [3, 0, 1, 2]
+ result = pc.array_sort_indices(arr, order="ascending")
+ assert result.to_pylist() == [3, 0, 1, 2]
+ result = pc.array_sort_indices(arr, order="descending")
+ assert result.to_pylist() == [1, 0, 3, 2]
+
+ with pytest.raises(ValueError, match="not a valid order"):
+ pc.array_sort_indices(arr, order="nonscending")
+
+
+def test_sort_indices_array():
+ arr = pa.array([1, 2, None, 0])
+ result = pc.sort_indices(arr)
+ assert result.to_pylist() == [3, 0, 1, 2]
+ result = pc.sort_indices(arr, sort_keys=[("dummy", "ascending")])
Review comment:
Yes, it's certainly not the nicest API ..
Note that for this specific case, the additional "dummy" is needed because it's using the generic `sort_indices` kernel. With `pc.sort_indices_array` it would be simpler as `order="ascending"`.
> It's a bit of a pity to have such a verbose convention (having to type "ascending" for every sort key).
We could add a "short-cut" to write the default a bit more succinctly in case all columns are sorted ascending. For example, we could allow `sort_keys=["col1", "col2"]` and translate it to `sort_keys=[("col1", "ascending"), ("col2", "ascending")]` on the python side.
However, of course once you want to sort (one of the columns) descending, you still need the more verbose option.
Another idea could be to have a `order` keyword on the python side, but that only works to set the order of *all* sort keys to the same value (so doing something like `sort_keys=["col1", "col2"], order="descending"` would be simpler).
Pandas indeed splits the sort key and the sort order in two different keywords, but as you mention, that has drawbacks as well, specifically when sorting by multiple columns.
I am personally not a big fan of adding any kind of "string syntax".
----------------------------------------------------------------
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 #8805: ARROW-10725: [Python][Compute] Expose sort options in Python bindings
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on a change in pull request #8805:
URL: https://github.com/apache/arrow/pull/8805#discussion_r540235330
##########
File path: python/pyarrow/compute.py
##########
@@ -155,7 +157,7 @@ def _handle_options(name, option_class, options, kwargs):
"Function {!r} expected a {} parameter, got {}"
.format(name, option_class, type(options)))
- return options
+ return option_class()
Review comment:
Indeed, and for RecordBatch/Table, you already get a nice error message when providing it empty sort keys.
----------------------------------------------------------------
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] kou commented on a change in pull request #8805: ARROW-10725: [Python][Compute] Expose sort options in Python bindings
Posted by GitBox <gi...@apache.org>.
kou commented on a change in pull request #8805:
URL: https://github.com/apache/arrow/pull/8805#discussion_r533032135
##########
File path: python/pyarrow/compute.py
##########
@@ -155,7 +157,7 @@ def _handle_options(name, option_class, options, kwargs):
"Function {!r} expected a {} parameter, got {}"
.format(name, option_class, type(options)))
- return options
+ return option_class()
Review comment:
I'm OK that we add `SortOptins::Defaults()` with empty `sort_keys` and use it for the default option of `sort_indices`.
It doesn't work for `RecordBatch` and `Table` but works for `Array` and `ChunkedArray`.
----------------------------------------------------------------
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