bkietz commented on a change in pull request #11384:
URL: https://github.com/apache/arrow/pull/11384#discussion_r741225936
##########
File path: cpp/src/arrow/compute/api_vector.h
##########
@@ -98,17 +98,17 @@ enum class NullPlacement {
/// \brief One sort key for PartitionNthIndices (TODO) and SortIndices
class ARROW_EXPORT SortKey : public util::EqualityComparable<SortKey> {
public:
- explicit SortKey(std::string name, SortOrder order = SortOrder::Ascending)
- : name(std::move(name)), order(order) {}
+ explicit SortKey(FieldRef target, SortOrder order = SortOrder::Ascending)
+ : target(std::move(target)), order(order) {}
using util::EqualityComparable<SortKey>::Equals;
using util::EqualityComparable<SortKey>::operator==;
using util::EqualityComparable<SortKey>::operator!=;
bool Equals(const SortKey& other) const;
std::string ToString() const;
- /// The name of the sort column.
- std::string name;
+ /// A FieldRef targetting the sort column.
+ FieldRef target;
Review comment:
This API is explicitly experimental, so I don't think we need be so
conservative about changing it. It *would* be a `struct` (semantically it
functions as one) except that it needs a virtual destructor. This compromise
between `struct` and `class` is used for a number of `class /.*Options` and
*is* a deviation from the style guide, but facilitates a more terse
specification of options structs. I don't think replacing the public fields
with full classy access boilerplate is in scope for this PR, but I can write a
follow up JIRA if you like
--
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]
