Light-City commented on code in PR #38584:
URL: https://github.com/apache/arrow/pull/38584#discussion_r1388805188
##########
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:
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.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
