Skip to content

GH-38558: [C++] Add support for null sort option per sort key - #38584

Closed
Light-City wants to merge 4 commits into
apache:mainfrom
Light-City:GH-38558
Closed

GH-38558: [C++] Add support for null sort option per sort key#38584
Light-City wants to merge 4 commits into
apache:mainfrom
Light-City:GH-38558

Conversation

@Light-City

@Light-CityLight-City commented Nov 4, 2023

Copy link
Copy Markdown
Contributor

Rationale for this change

support multi sortkey nulls first.

order by i nulls first, j, k nulls first;

The current null sorting only supports all sortkeys, not a certain sortkey, so NullPlacement is extended to the SortKey field. Since the underlying framework is very well written, when modifying this function, you only need to pass the null_placement of each SortKey in. That’s it.

What changes are included in this PR?

1.SortKey structure, NullPlacemnt transfer logic, sorting logic and Ording related, test related
2.Substriait related.
3.c_glib related.
4.SelectK related.
5.RankOptions related.

Are these changes tested?

yes, I changed the code inside vector_sort_test.cc and performed additional tests.

Are there any user-facing changes?

yes, pg database include null sorting of multiple sort keys.

@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #38558has been automatically assigned in GitHub to PR creator.

@koukou changed the title GH-38558: [C++] Fix: null sorting of multiple sort keys.GH-38558: [C++] Add support for null sort option per sort keyNov 4, 2023
@Light-City
Light-City requested a review from kou as a code ownerNovember 5, 2023 05:49
@Light-City
Light-Cityforce-pushed the GH-38558 branch 2 times, most recently from a8d62df to 2948f03CompareNovember 5, 2023 05:54
Comment threadcpp/src/arrow/compute/api_vector.h Outdated
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc Outdated
Comment threadc_glib/arrow-glib/compute.cpp Outdated
Comment threadc_glib/arrow-glib/compute.cpp Outdated
Comment threadc_glib/arrow-glib/compute.cpp Outdated
Comment threadc_glib/arrow-glib/compute.cpp Outdated
Comment threadc_glib/arrow-glib/compute.cpp Outdated
Comment threadc_glib/arrow-glib/compute.cpp Outdated
Comment threadc_glib/arrow-glib/compute.cpp Outdated
@github-actionsgithub-actionsBot added awaiting changes Awaiting changes Component: Ruby awaiting change review Awaiting change review and removed awaiting review Awaiting review awaiting changes Awaiting changes labels Nov 6, 2023
@Light-City

Copy link
Copy Markdown
ContributorAuthor

Fixes #38633,
Fixes #38558

1.Reconstruct the SortKey structure and add NullPlacement.
2.Remove NullPlacement from SortOptions
3.Fix selectk not displaying non-empty results in null AtEnd scenario.
When limit k is greater than the actual table data and the table contains Null/NaN, the data cannot be obtained and only non-empty results are available.
Therefore, we support returning non-null and supporting the order of setting Null for each SortKey.
4.Add relevant unit tests and change the interface implemented by multiple versions
@Light-City

Light-City commented Nov 9, 2023

Copy link
Copy Markdown
ContributorAuthor

This PR is ready, all tests have passed, and four of them failed seem to have nothing to do with this submission.
@kou@westonpace

@bkietz
bkietz self-requested a review November 9, 2023 16:09
Comment threadpython/pyarrow/tests/test_compute.py Outdated
Comment on lines 2575 to +2576
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")])

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that the default value cannot be set at present, CSortKey.null_placement=NullPlacement::AtEnd

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean that a tuple of name, order without null placement should still be accepted as a sort key. Currently IIUC passing this to _SortOptions._set_options would raise an exception.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I understand your purpose. we hope that the original tuple can be processed correctly without throwing an exception.

@Light-CityLight-CityNov 14, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

 cdef cppclass CSortKey" arrow::compute::SortKey":
CSortKey(CFieldRef target, CSortOrder order, CNullPlacement null_placement)
CFieldRef target
CSortOrder order
CNullPlacement null_placement

What I mean is that when we add a new fileld(here is null_placement) end to sortkey, this tuple is incompatible with the original one, that is, it seems that we cannot pass a default value here so that the original tuple can also be used.

our expected:

 cdef cppclass CSortKey" arrow::compute::SortKey":
CSortKey(CFieldRef target, CSortOrder order, CNullPlacement null_placement = AtEnd)
CFieldRef target
CSortOrder order
CNullPlacement null_placement

Then the following two situations can be supported

arr, k=k, sort_keys=[("dummy", order)])
arr, k=k, sort_keys=[("dummy", order, null_placement)])

Comment threadcpp/src/arrow/engine/substrait/serde_test.cc
/// 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;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 )

@Light-CityLight-CityNov 10, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I wanted to set the deprecated field to a std::optional member, I found some conflicts while retaining the original construction, for example:

If the user calls SortOptions, use the following method:

auto null_placement = {SortKey{NullPlacement::AtEnd}, SortKey{}};
SortOptions(sort_keys, NullPlacement::AtStart)
SortOptions(std::vector<SortKey> sort_keys = {}, NullPlacement null_placement = NullPlacement::AtEnd) {
// Should I use SortKey's NullPlacement or NullPlacement::AtStart in the constructor?
}

Therefore, here I added the interface modification annotation and did not use std::optional.

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting change review Awaiting change review awaiting changes Awaiting changes labels Nov 9, 2023
@github-actionsgithub-actionsBot added the awaiting changes Awaiting changes label Nov 13, 2023
@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Nov 14, 2023
@taepper

Copy link
Copy Markdown
Contributor

What is the status on this?

@kou

kou commented Jan 30, 2026

Copy link
Copy Markdown
Member

I close this in favor of #46926.

@koukou closed this Jan 30, 2026
pitrou added a commit that referenced this pull request Jun 22, 2026
See #38584 for original PR. Will be quoted for this PR description.
### Rationale for this change
> support multi sortkey nulls first.
```
order by i nulls first, j, k nulls first;
```
> The current null sorting only supports all sortkeys, not a certain sortkey, so NullPlacement is extended to the SortKey field. Since the underlying framework is very well written, when modifying this function, you only need to pass the null_placement of each SortKey in. That’s it.
### What changes are included in this PR?
> 1.SortKey structure, NullPlacemnt transfer logic, sorting logic and Ording related, test related
> 2.Substriait related.
> 3.c_glib related.
> 4.SelectK related.
> 5.RankOptions related.
### Are these changes tested?
> yes, I changed the code inside vector_sort_test.cc and performed additional tests.
### Are there any user-facing changes?
> yes, pg database include null sorting of multiple sort keys.
**This PR includes breaking changes to public APIs.** (If there are any breaking changes to public APIs, please explain which changes are breaking. If not, you can remove this.)
I amended the original PR to be less breaking in public APIs.
Still Ordering, SortOptions, RankOptions, and RankQuantileOptions now accept a `std::optional<NullPlacement>` instead of NullPlacement, which did lead to some changes in downstream APIs and bindings. ~I also need some help with fixing the `c_glib` bindings.~
* GitHub Issue: #38558
Lead-authored-by: Alexander Taepper <alexander.taepper@gmail.com>
Co-authored-by: light-city <455954986@qq.com>
Co-authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[C++][Compute] Fix null sorting of multiple sort keys

4 participants

@Light-City@taepper@kou@bkietz