ARROW-13025: [C++][Python] Add FunctionOptions::Equals/ToString/Serialize #10511

lidavidm · 2021-06-10T21:28:17Z

This is a draft of adding more utility methods to FunctionOptions. It's not fully implemented (it needs rebasing + serialization isn't implemented for most options, plus there are various TODOs scattered). But before I proceed further, I wanted to get some feedback.

Some concerns I have:

I don't like adding protected methods to a struct, and it's inconsistent with how equality is implemented for other structs (via a visitor or otherwise centralized in a single location). However ARROW-8891 will require that we be able to define kernels - and presumably their options - in a separate shared library, so I don't think we can do much better than this.
But for (de)serialization, we'll still need some way to dynamically register the mapping between a type_name and the actual struct, so maybe this is a moot point.
I've exposed the fact that serialization uses StructScalars to support Expression - but maybe this is too much to commit to in the API?

lidavidm · 2021-06-10T21:28:34Z

@bkietz I'd appreciate if you had any feedback on the approach here/if this was in line with what you were thinking.

github-actions · 2021-06-10T21:28:34Z

https://issues.apache.org/jira/browse/ARROW-13025

bkietz · 2021-06-14T17:16:11Z

@lidavidm thanks for working on this!

I don't like adding protected methods to a struct, and it's inconsistent with how equality is implemented for other structs (via a visitor or otherwise centralized in a single location). However ARROW-8891 will require that we be able to define kernels - and presumably their options - in a separate shared library, so I don't think we can do much better than this.

I agree that it's unfortunately necessary in this case: FunctionOptions is an extension point (unlike Array or Scalar, where we own the full enumeration of subclasses). In light of that: FunctionOptions should be promoted from struct to class (same for its subclasses).

But for (de)serialization, we'll still need some way to dynamically register the mapping between a type_name and the actual struct, so maybe this is a moot point.

We could add this to FunctionRegistry, I think:

class FunctionOptionsType {
 public:
  virtual std::string name() const = 0;

  virtual bool Compare(const FunctionOptions&, const FunctionOptions&) const = 0;
  virtual std::string Stringify(const FunctionOptions&) const = 0;
  virtual Result<std::unique_ptr<Buffer>> Serialize(const FunctionOptions&) const = 0;
  virtual Result<std::unique_ptr<FunctionOptions>> Deserialize(const Buffer&) const = 0;
};

class FunctionOptions {
 public:
  virtual ~FunctionOptions() = default;
  const FunctionOptionsType* options_type() const { return type_; }

  bool Equals(const FunctionOptions& other) const {
    return type_ = other.type_ && type_->Compare(*this, other);
  }
  // forward other methods to type_ as well

 protected:
  explicit FunctionOptions(const FunctionOptionsType* type) : type_(type) {}
  const FunctionOptionsType* type_;
};

class FunctionRegistry {
 public:
  // Called alongside AddFunction
  // will raise if another options type with the same name exists
  Status AddFunctionOptionsType(const FunctionOptionsType*);
};

class Function {
 public:
  // nullptr if the function does not take options.
  // This is also nice since we can assert correctly typed FunctionOptions
  const FunctionOptionsType* options_type() const;
};

I've exposed the fact that serialization uses StructScalars to support Expression - but maybe this is too much to commit to in the API?

Yes, I'd prefer that implementation detail be kept internal. Please ensure ToScalar/FromScalar can't be accessed without including an _internal header.

pitrou · 2021-06-16T09:35:52Z

Can we explain the objectives and design constraints here?

pitrou · 2021-06-16T09:36:30Z

Also, if the goal is to serialize function options as a struct scalar using the IPC format, it would be good to check performance, because I fear there may be a lot of overhead doing so.

pitrou · 2021-06-16T09:37:52Z

One more thought: we should try to minimize the hand-writing of boilerplate. Is it possible to somehow define a subclass of std::tuple<Args...> that would automatically generate at least some of the methods (such as comparison and serialization)?

pitrou · 2021-06-16T09:51:54Z

There's an intriguing thing in Boost which seems to allow to treat a struct as a tuple, but the doc is obnoxious:
https://www.boost.org/doc/libs/1_76_0/libs/fusion/doc/html/fusion/adapted/adapt_struct.html

Another intriguing thing when looking for "C++ named tuple":
https://stackoverflow.com/a/17072195/10194

lidavidm · 2021-06-16T11:37:02Z

Can we explain the objectives and design constraints here?

The goal is to just add basic utility methods on FunctionOptions. Then we could make them serializable/pickleable (and by extension, we could then support pickling/unpickling things like Expressions, and properly compare them too). The main constraint is not assuming that all the subclasses are statically known, so that we can move the function implementations into a separate library eventually with ARROW-8891.

Also, if the goal is to serialize function options as a struct scalar using the IPC format, it would be good to check performance, because I fear there may be a lot of overhead doing so.

I'll add a benchmark.

One more thought: we should try to minimize the hand-writing of boilerplate.

Yes…I'll look into this, this initial refactor was rather tedious.

bkietz · 2021-06-17T20:07:48Z

cpp/src/arrow/compute/function_test.cc

Could you add operator== and PrintTo so these can just be written

Suggested change

ASSERT_TRUE(cur.Equals(cur)) << cur.ToString();

ASSERT_EQ(cur, cur);

bkietz · 2021-06-17T20:10:05Z

cpp/src/arrow/compute/registry.h

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

bkietz · 2021-06-17T20:12:14Z

cpp/src/arrow/compute/cast.cc

Suggested change

static std::unique_ptr<internal::FunctionOptionsType> instance(new CastOptionsType());

return instance.get();

static const CastOptionsType instance;

return &instance;

bkietz · 2021-06-21T19:09:51Z

@lidavidm 676c902

lidavidm · 2021-06-21T19:10:34Z

Thanks! I'll circle back to this soon

lidavidm · 2021-06-22T19:24:29Z

Alright, I've pushed half a stab at this - I'm going to pick up ARROW-11932 so I can write a generic serialization for vector<T> (where T itself is serializable to Scalar). We'll also probably want some way to cut down on the boilerplate for enums - right now I have a set of partially specialized traits to handle enums, which are a little tedious to implement; a macro would probably help there.

cpp/src/arrow/array/builder_base.cc

cpp/src/arrow/compute/api_aggregate.cc

cpp/src/arrow/compute/function_test.cc

cpp/src/arrow/compute/util_internal.h

cpp/src/arrow/compute/function.h

cpp/src/arrow/compute/api_scalar.h

cpp/src/arrow/compute/function_internal.h

lidavidm · 2021-06-28T19:18:59Z

Thanks. Some of this applies to #10579 so I'll address that there and rebase.

…4_t)

Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com>

bkietz

Only small nits remain, this is looking great

cpp/src/arrow/compute/function_internal.h

cpp/src/arrow/compute/function_test.cc

cpp/src/arrow/util/reflection_internal.h

python/pyarrow/_compute.pyx

cpp/src/arrow/compute/function.h

cpp/src/arrow/compute/function_internal.h

bkietz

LGTM, thanks for doing this!

bkietz · 2021-06-30T14:35:43Z

cpp/src/arrow/compute/api_scalar.h

-
+class ARROW_EXPORT CompareOptions : public FunctionOptions {
+ public:
+  explicit CompareOptions(CompareOperator op);


I see this is necessary; that's unfortunate. Could you open a follow up JIRA to remove the binding to CompareOptions and demote this to a deprecated struct?

Filed ARROW-13219

bkietz · 2021-06-30T14:41:14Z

cpp/src/arrow/compute/function_internal.h

+
+namespace internal {
+template <>
+struct EnumTraits<compute::SortOrder>


Is there a reason to put this here instead of api_vector.cc?

I found it had to be here to be considered by the overloads of the Generic* functions for SortKey, which in turn I had to place in this header or they wouldn't be considered by the generated class in GetFunctionOptionsType. If I move all the overloads for SortKey/SortOrder into api_vector.cc, then I get errors like this:

In file included from /home/lidavidm/Code/upstream/arrow-13025/cpp/src/arrow/compute/api_vector.cc:28: /home/lidavidm/Code/upstream/arrow-13025/cpp/src/arrow/compute/function_internal.h: In instantiation of 'arrow::Result<std::shared_ptr<arrow::Scalar> > arrow::compute::internal::GenericToScalar(const std::vector<T>&) [with T = arrow::compute::SortKey]': /home/lidavidm/Code/upstream/arrow-13025/cpp/src/arrow/compute/function_internal.h:462:34: required from 'void arrow::compute::internal::ToStructScalarImpl<Options>::operator()(const Property&, size_t) [with Property = arrow::internal::DataMemberProperty<arrow::compute::SortOptions, std::vector<arrow::compute::SortKey> >; Options = arrow::compute::SortOptions; size_t = long unsigned int]' /home/lidavidm/Code/upstream/arrow-13025/cpp/src/arrow/util/reflection_internal.h:67:28: required from 'void arrow::internal::ForEachTupleMemberImpl(const std::tuple<_Elements ...>&, Fn&&, arrow::internal::index_sequence<I ...>) [with long unsigned int ...I = {0}; T = {arrow::internal::DataMemberProperty<arrow::compute::SortOptions, std::vector<arrow::compute::SortKey, std::allocator<arrow::compute::SortKey> > >}; Fn = arrow::compute::internal::ToStructScalarImpl<arrow::compute::SortOptions>&]' /home/lidavidm/Code/upstream/arrow-13025/cpp/src/arrow/util/reflection_internal.h:72:25: required from 'void arrow::internal::ForEachTupleMember(const std::tuple<_Tps ...>&, Fn&&) [with T = {arrow::internal::DataMemberProperty<arrow::compute::SortOptions, std::vector<arrow::compute::SortKey, std::allocator<arrow::compute::SortKey> > >}; Fn = arrow::compute::internal::ToStructScalarImpl<arrow::compute::SortOptions>&]' /home/lidavidm/Code/upstream/arrow-13025/cpp/src/arrow/util/reflection_internal.h:100:23: required from 'void arrow::internal::PropertyTuple<Properties>::ForEach(Fn&&) const [with Fn = arrow::compute::internal::ToStructScalarImpl<arrow::compute::SortOptions>&; Properties = {arrow::internal::DataMemberProperty<arrow::compute::SortOptions, std::vector<arrow::compute::SortKey, std::allocator<arrow::compute::SortKey> > >}]' /home/lidavidm/Code/upstream/arrow-13025/cpp/src/arrow/compute/function_internal.h:456:5: required from 'arrow::compute::internal::ToStructScalarImpl<Options>::ToStructScalarImpl(const Options&, const Tuple&, std::vector<std::__cxx11::basic_string<char> >*, std::vector<std::shared_ptr<arrow::Scalar> >*) [with Tuple = arrow::internal::PropertyTuple<arrow::internal::DataMemberProperty<arrow::compute::SortOptions, std::vector<arrow::compute::SortKey, std::allocator<arrow::compute::SortKey> > > >; Options = arrow::compute::SortOptions]' /home/lidavidm/Code/upstream/arrow-13025/cpp/src/arrow/compute/function_internal.h:536:7: required from 'arrow::Status arrow::compute::internal::GetFunctionOptionsType(const Properties& ...)::OptionsType::ToStructScalar(const arrow::compute::FunctionOptions&, std::vector<std::__cxx11::basic_string<char> >*, std::vector<std::shared_ptr<arrow::Scalar> >*) const [with Options = arrow::compute::SortOptions; Properties = {arrow::internal::DataMemberProperty<arrow::compute::SortOptions, std::vector<arrow::compute::SortKey, std::allocator<arrow::compute::SortKey> > >}]' /home/lidavidm/Code/upstream/arrow-13025/cpp/src/arrow/compute/function_internal.h:550:3: required from 'const arrow::compute::FunctionOptionsType* arrow::compute::internal::GetFunctionOptionsType(const Properties& ...) [with Options = arrow::compute::SortOptions; Properties = {arrow::internal::DataMemberProperty<arrow::compute::SortOptions, std::vector<arrow::compute::SortKey, std::allocator<arrow::compute::SortKey> > >}]' /home/lidavidm/Code/upstream/arrow-13025/cpp/src/arrow/compute/api_vector.cc:182:89: required from here /home/lidavidm/Code/upstream/arrow-13025/cpp/src/arrow/compute/function_internal.h:264:59: error: no matching function for call to 'GenericTypeSingleton<arrow::compute::SortKey>()' 264 | std::shared_ptr<DataType> type = GenericTypeSingleton<T>(); | ~~~~~~~~~~~~~~~~~~~~~~~^~

I see, that makes sense

pitrou · 2021-07-01T10:54:22Z

python/pyarrow/_compute.pyx

+        c_options = move(GetResultValue(move(maybe_options)))
+        type_name = frombytes(c_options.get().options_type().type_name())
+        mapping = {
+            "array_sort": ArraySortOptions,


Is their a reason the type_name is different from the C++ class name (which is the same as the Python class name)? If it was the same, we probably wouldn't need to hand-maintain this mapping.

No good reason, thanks for the idea. I filed ARROW-13235.

jorisvandenbossche · 2021-07-01T12:40:37Z

python/pyarrow/_compute.pyx

+        return options
+
+    def __repr__(self):
+        return frombytes(self.get_options().ToString())


We probably still want to include the name of the options in the output (now it's only the arguments).

In [9]: pc.TrimOptions("a") Out[9]: {characters="a"}

Opened https://issues.apache.org/jira/browse/ARROW-13236 for this for the Python side (or is that something we also want to add on the C++ side? (so the output of {{ToString}} includes it))

I was following the output of the existing serialization for Expressions which didn't directly include the name. But maybe that's worth including in both C++ and Python.

github-actions bot added Component: C++ Component: Python labels Jun 10, 2021

bkietz self-requested a review June 14, 2021 16:18

lidavidm force-pushed the arrow-13025 branch from ddf5e1b to b59a0c4 Compare June 15, 2021 21:16

bkietz requested changes Jun 18, 2021

View reviewed changes

lidavidm force-pushed the arrow-13025 branch from b59a0c4 to c918d8e Compare June 22, 2021 19:22

lidavidm force-pushed the arrow-13025 branch 6 times, most recently from 4967267 to a635db7 Compare June 25, 2021 15:26

lidavidm marked this pull request as ready for review June 25, 2021 15:57

bkietz requested changes Jun 28, 2021

View reviewed changes

lidavidm mentioned this pull request Jun 28, 2021

ARROW-11932: [C++] Implement ArrayBuilder::AppendScalar #10579

Closed

lidavidm force-pushed the arrow-13025 branch from a635db7 to ca6ab99 Compare June 29, 2021 18:59

lidavidm added 2 commits June 29, 2021 15:28

ARROW-11932: [C++] Implement ArrayBuilder::AppendScalar

d9d6bb4

ARROW-11932: [C++] Add ArrayBuilder::AppendScalar(const Scalar&, int6…

bd6ac13

…4_t)

lidavidm and others added 8 commits June 29, 2021 15:28

ARROW-11932: [C++] Reserve() in AppendScalar

87d66de

ARROW-11932: [C++] Don't require an allocation in AppendScalar

0da2e62

ARROW-11932: [C++] Fix test on MSVC

0a76cb7

Update cpp/src/arrow/array/builder_base.cc

f273db1

Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com>

ARROW-11932: [C++] Reserve in more cases

6d966ea

ARROW-11932: [C++] Handle fixed size binary with other fixed size types

c64f6a7

ARROW-11932: [C++] Clean up implementation for lists

9a48b4a

ARROW-13025: [C++] Add FunctionOptions::Equals/ToString

ab60ef1

lidavidm force-pushed the arrow-13025 branch from ca6ab99 to ab60ef1 Compare June 29, 2021 19:33

bkietz requested changes Jun 29, 2021

View reviewed changes

lidavidm added 4 commits June 29, 2021 17:07

ARROW-13025: [C++] Fix Python bindings

ddfcf2e

ARROW-13025: [C++] Move and extend EnumTraits

7acc312

ARROW-13025: [C++] CompareOptions needs to be a FunctionOptions for GLib

e4f6057

ARROW-13025: [C++] Try to fix Clang/libc++ build

c64a14f

bkietz approved these changes Jun 30, 2021

View reviewed changes

bkietz closed this in 1430c93 Jun 30, 2021

pitrou reviewed Jul 1, 2021

View reviewed changes

jorisvandenbossche reviewed Jul 1, 2021

View reviewed changes

jorisvandenbossche mentioned this pull request Jan 17, 2022

ARROW-12060: [Python] Enable calling compute functions on Expressions #11918

Closed

asfimport mentioned this pull request Jul 1, 2021

[C++][Compute] Enhance FunctionOptions with equality, debug representability, and serializability #28738

Closed

	ASSERT_TRUE(cur.Equals(cur)) << cur.ToString();
	ASSERT_EQ(cur, cur);

-    static std::unique_ptr<internal::FunctionOptionsType> instance(new CastOptionsType());
-    return instance.get();
+    static const CastOptionsType instance;
+    return &instance;

ARROW-13025: [C++][Python] Add FunctionOptions::Equals/ToString/Serialize #10511

ARROW-13025: [C++][Python] Add FunctionOptions::Equals/ToString/Serialize #10511

Uh oh!

Conversation

lidavidm commented Jun 10, 2021

Uh oh!

lidavidm commented Jun 10, 2021

Uh oh!

github-actions bot commented Jun 10, 2021

Uh oh!

bkietz commented Jun 14, 2021

Uh oh!

pitrou commented Jun 16, 2021

Uh oh!

pitrou commented Jun 16, 2021

Uh oh!

pitrou commented Jun 16, 2021

Uh oh!

pitrou commented Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lidavidm commented Jun 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkietz commented Jun 21, 2021

Uh oh!

lidavidm commented Jun 21, 2021

Uh oh!

lidavidm commented Jun 22, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lidavidm commented Jun 28, 2021

Uh oh!

bkietz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bkietz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorisvandenbossche Jul 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

pitrou commented Jun 16, 2021 •

edited

Loading

jorisvandenbossche Jul 1, 2021 •

edited

Loading