[GitHub] [arrow] bkietz commented on pull request #10511: ARROW-13025: [C++][Python] Add FunctionOptions::Equals/ToString/Serialize

[GitBox <gi...@apache.org>]

bkietz commented on pull request #10511:
URL: https://github.com/apache/arrow/pull/10511#issuecomment-860852122


   @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:
   ```c++
   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.


-- 
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:
users@infra.apache.org