bkietz commented on code in PR #38584:
URL: https://github.com/apache/arrow/pull/38584#discussion_r1388243968
##########
cpp/src/arrow/engine/substrait/serde_test.cc:
##########
@@ -5474,10 +5472,9 @@ TEST(Substrait, MixedSort) {
ConversionOptions conversion_options;
conversion_options.named_table_provider = std::move(table_provider);
- ASSERT_THAT(
- DeserializePlan(*buf, /*registry=*/nullptr, /*ext_set_out=*/nullptr,
- conversion_options),
- Raises(StatusCode::NotImplemented, testing::HasSubstr("mixed null
placement")));
+ ASSERT_OK_AND_ASSIGN(
+ auto plan_info, DeserializePlan(*buf, /*registry=*/nullptr,
/*ext_set_out=*/nullptr,
+ conversion_options));
Review Comment:
Please also assert that the SortKeys have correct null_placement
##########
python/pyarrow/tests/test_compute.py:
##########
@@ -2612,20 +2614,22 @@ def validate_select_k(select_k_indices, tbl, sort_keys,
stable_sort=False):
table = pa.table({"a": [1, 2, 0], "b": [1, 0, 1]})
for k in [0, 2, 4]:
result = pc.select_k_unstable(
- table, k=k, sort_keys=[("a", "ascending")])
- validate_select_k(result, table, sort_keys=[("a", "ascending")])
+ table, k=k, sort_keys=[("a", "ascending", "at_end")])
+ validate_select_k(result, table, sort_keys=[("a", "ascending",
"at_end")])
result = pc.select_k_unstable(
- table, k=k, sort_keys=[(pc.field("a"), "ascending"), ("b",
"ascending")])
+ table, k=k, sort_keys=[(pc.field("a"), "ascending", "at_end"),
("b", "ascending", "at_end")])
Review Comment:
Please ensure that python code is correctly formatted
https://github.com/apache/arrow/actions/runs/6806460519/job/18507732686?pr=38584#step:5:799
##########
python/pyarrow/tests/test_compute.py:
##########
@@ -2572,8 +2573,8 @@ def validate_select_k(select_k_indices, arr, order,
stable_sort=False):
for k in [0, 2, 4]:
for order in ["descending", "ascending"]:
result = pc.select_k_unstable(
- arr, k=k, sort_keys=[("dummy", order)])
- validate_select_k(result, arr, order)
+ arr, k=k, sort_keys=[("dummy", order, "at_end")])
Review Comment:
Please ensure that user code which still uses `pc.select_k_unstable(arr,
k=k, sort_keys=[("dummy", order)])` will not be broken by this change; IE most
of these tests should not need to change.
##########
cpp/src/arrow/compute/api_vector.h:
##########
@@ -115,13 +114,11 @@ class ARROW_EXPORT SortOptions : public FunctionOptions {
/// Note: Both classes contain the exact same information. However,
/// sort_options should only be used in a "function options" context while
Ordering
/// is used more generally.
- Ordering AsOrdering() && { return Ordering(std::move(sort_keys),
null_placement); }
- Ordering AsOrdering() const& { return Ordering(sort_keys, null_placement); }
+ Ordering AsOrdering() && { return Ordering(std::move(sort_keys)); }
+ Ordering AsOrdering() const& { return Ordering(sort_keys); }
/// Column key(s) to order by and how to order by these sort keys.
std::vector<SortKey> sort_keys;
- /// Whether nulls and NaNs are placed at the start or at the end
- NullPlacement null_placement;
Review Comment:
Removing this member is a breaking change, which should be explicitly called
out in the PR description. Note that instead, we could keep this member (maybe
deprecating it) as an optional default (see for example the same demotion
applied to SetLookupOptions::skip_nulls in
https://github.com/apache/arrow/pull/36739/files#diff-6bc7ecec6a4f7bcefc2511cde3bd809340ad0d94bb8f7cc5f4994063c798f2faR313
)
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
