bkietz commented on a change in pull request #10511:
URL: https://github.com/apache/arrow/pull/10511#discussion_r653899327
##########
File path: cpp/src/arrow/compute/function_test.cc
##########
@@ -31,6 +35,89 @@
namespace arrow {
namespace compute {
+TEST(FunctionOptions, Equality) {
+ std::vector<std::shared_ptr<FunctionOptions>> options;
+ options.emplace_back(new ScalarAggregateOptions());
+ options.emplace_back(new ScalarAggregateOptions(/*skip_nulls=*/false,
/*min_count=*/1));
+ options.emplace_back(new ModeOptions());
+ options.emplace_back(new ModeOptions(/*n=*/2));
+ options.emplace_back(new VarianceOptions());
+ options.emplace_back(new VarianceOptions(/*ddof=*/2));
+ options.emplace_back(new QuantileOptions());
+ options.emplace_back(
+ new QuantileOptions(/*q=*/0.75,
QuantileOptions::Interpolation::MIDPOINT));
+ options.emplace_back(new TDigestOptions());
+ options.emplace_back(
+ new TDigestOptions(/*q=*/0.75, /*delta=*/50, /*buffer_size=*/1024));
+ options.emplace_back(new IndexOptions(ScalarFromJSON(int64(), "16")));
+ options.emplace_back(new IndexOptions(ScalarFromJSON(boolean(), "true")));
+ options.emplace_back(new IndexOptions(ScalarFromJSON(boolean(), "null")));
+ options.emplace_back(new ArithmeticOptions());
+ options.emplace_back(new ArithmeticOptions(/*check_overflow=*/true));
+ options.emplace_back(new ElementWiseAggregateOptions());
+ options.emplace_back(new ElementWiseAggregateOptions(/*skip_nulls=*/false));
+ options.emplace_back(new MatchSubstringOptions("pattern"));
+ options.emplace_back(new MatchSubstringOptions("pattern",
/*ignore_case=*/true));
+ options.emplace_back(new SplitOptions());
+ options.emplace_back(new SplitOptions(/*max_splits=*/2, /*reverse=*/true));
+ options.emplace_back(new SplitPatternOptions("pattern"));
+ options.emplace_back(
+ new SplitPatternOptions("pattern", /*max_splits=*/2, /*reverse=*/true));
+ options.emplace_back(new ReplaceSubstringOptions("pattern", "replacement"));
+ options.emplace_back(
+ new ReplaceSubstringOptions("pattern", "replacement",
/*max_replacements=*/2));
+ options.emplace_back(new ExtractRegexOptions("pattern"));
+ options.emplace_back(new ExtractRegexOptions("pattern2"));
+ options.emplace_back(new SetLookupOptions(ArrayFromJSON(int64(), "[1, 2, 3,
4]")));
+ options.emplace_back(new SetLookupOptions(ArrayFromJSON(boolean(), "[true,
false]")));
+ options.emplace_back(new StrptimeOptions("%Y", TimeUnit::type::MILLI));
+ options.emplace_back(new StrptimeOptions("%Y", TimeUnit::type::NANO));
+ options.emplace_back(new TrimOptions(" "));
+ options.emplace_back(new TrimOptions("abc"));
+ options.emplace_back(new SliceOptions(/*start=*/1));
+ options.emplace_back(new SliceOptions(/*start=*/1, /*stop=*/-5,
/*step=*/-2));
+ options.emplace_back(new CompareOptions(CompareOperator::EQUAL));
+ options.emplace_back(new CompareOptions(CompareOperator::NOT_EQUAL));
+ // N.B. we never actually use field_nullability or field_metadata in Arrow
+ options.emplace_back(new ProjectOptions({"col1"}, {true}, {}));
+ options.emplace_back(new ProjectOptions({"col1"}, {false}, {}));
+ options.emplace_back(new CastOptions(CastOptions::Safe(boolean())));
+ options.emplace_back(new CastOptions(CastOptions::Unsafe(int64())));
+ options.emplace_back(new FilterOptions());
+ options.emplace_back(
+ new FilterOptions(FilterOptions::NullSelectionBehavior::EMIT_NULL));
+ options.emplace_back(new TakeOptions());
+ options.emplace_back(new TakeOptions(/*boundscheck=*/false));
+ options.emplace_back(new DictionaryEncodeOptions());
+ options.emplace_back(
+ new
DictionaryEncodeOptions(DictionaryEncodeOptions::NullEncodingBehavior::ENCODE));
+ options.emplace_back(new ArraySortOptions());
+ options.emplace_back(new ArraySortOptions(SortOrder::Descending));
+ options.emplace_back(new SortOptions());
+ options.emplace_back(new SortOptions({SortKey("key",
SortOrder::Ascending)}));
+ options.emplace_back(new SortOptions({SortKey("key",
SortOrder::Descending)}));
+ options.emplace_back(new PartitionNthOptions(/*pivot=*/0));
+ options.emplace_back(new PartitionNthOptions(/*pivot=*/42));
+
+ for (size_t i = 0; i < options.size(); i++) {
+ const size_t prev_i = i == 0 ? options.size() - 1 : i - 1;
+ const FunctionOptions& cur = *options[i];
+ const FunctionOptions& prev = *options[prev_i];
+ ASSERT_TRUE(cur.Equals(cur)) << cur.ToString();
Review comment:
Could you add `operator==` and `PrintTo` so these can just be written
```suggestion
ASSERT_EQ(cur, cur);
```
##########
File path: cpp/src/arrow/compute/registry.h
##########
@@ -32,6 +32,9 @@ namespace arrow {
namespace compute {
class Function;
+namespace internal {
+class FunctionOptionsType;
Review comment:
I don't think FunctionOptionsType itself needs to be internal/opaque.
Instead we can have the public methods be Serialize/Deserialize and let
To/FromStructScalar be the internal details
##########
File path: cpp/src/arrow/compute/cast.cc
##########
@@ -38,6 +40,93 @@ using internal::ToTypeName;
namespace compute {
namespace internal {
+// ----------------------------------------------------------------------
+// Function options
+
+namespace {
+class CastOptionsType : public internal::FunctionOptionsType {
+ public:
+ static const internal::FunctionOptionsType* GetInstance() {
+ static std::unique_ptr<internal::FunctionOptionsType> instance(new
CastOptionsType());
+ return instance.get();
Review comment:
```suggestion
static const CastOptionsType instance;
return &instance;
```
--
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:
[email protected]
