[GitHub] [arrow] js8544 commented on issue #32190: [C++] Implement cumulative product, max, and min compute functions

["js8544 (via GitHub)" <gi...@apache.org>]

js8544 commented on issue #32190:
URL: https://github.com/apache/arrow/issues/32190#issuecomment-1572136450

   I noticed that the functions cumprod, cummin and cummax would require the same option as `CumulativeSumOptions` (min and max won't need to `check_overflow` but I plan to remove it in #35790). 
   
   I want to avoid having four different option types with identical structure, so I intend to refactor `CumulativeSumOptions` to `CumulativeOptions` and reuse it for all cumulative functions.
   
   The current definition is
   ```cpp
   /// \brief Options for cumulative sum function
   class ARROW_EXPORT CumulativeSumOptions : public FunctionOptions {
    public:
     explicit CumulativeSumOptions(double start = 0, bool skip_nulls = false,
                                   bool check_overflow = false);
     explicit CumulativeSumOptions(std::shared_ptr<Scalar> start, bool skip_nulls = false,
                                   bool check_overflow = false);
     static constexpr char const kTypeName[] = "CumulativeSumOptions";
     static CumulativeSumOptions Defaults() { return CumulativeSumOptions(); }
   
     /// Optional starting value for cumulative operation computation
     std::shared_ptr<Scalar> start;
   
     /// If true, nulls in the input are ignored and produce a corresponding null output.
     /// When false, the first null encountered is propagated through the remaining output.
     bool skip_nulls = false;
   
     /// When true, returns an Invalid Status when overflow is detected
     bool check_overflow = false;
   };
   ```
   
   I plan to do this:
   ```cpp
   /// \brief Options for cumulative functions
   class ARROW_EXPORT CumulativeOptions : public FunctionOptions {
     // ... (same as above)
   };
   
   using CumulativeSumOptions = CumulativeOptions; // for backward compatibility
   ```
   
   I checked that with this change all compute tests still passed, both in cpp and python. But I want to make sure there are no better ways to solve this. Do you consider this as the best approach? cc @pitrou @westonpace 


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org