andishgar commented on PR #49290: URL: https://github.com/apache/arrow/pull/49290#issuecomment-4677824473
@pitrou A few notes: 1. Regarding [this](https://github.com/apache/arrow/pull/49290#discussion_r2851626609), since NaN is handled before it reaches `UlpDistance`, handling NaN inside this method is just a waste of CPU cycles, IMHO. 2. Regarding [this](https://github.com/apache/arrow/pull/49290#discussion_r2851639856), I found it's common in float comparison to specify several methods (atol, rtol, ulp) and perform a logical OR between them to get the final answer. 3. Regarding [this](https://github.com/apache/arrow/pull/49290#discussion_r2851661165) and [this](https://github.com/apache/arrow/pull/49290#discussion_r2851687977), my opinion is that the current builder like API is okay as is. However, I tried the APIs below and none of them are appropriate for `arrow::EqualOptions`. (If you have any other opinion, I'd be glad to hear it.) ```c++ EqualOptions float_comparison( std::optional<std::optional<double>> atol = std::nullopt, std::optional<std::optional<int32_t>> ulp_distance = std::nullopt, std::optional<bool> nans_equal = std::nullopt, std::optional<bool> signed_zero_equal = std::nullopt) const; ``` ```c++ template <typename T> using Update = std::variant<std::monostate, T>; EqualOptions float_comparison( Update<std::optional<double>> atol = {}, Update<std::optional<int32_t>> ulp_distance = {}, Update<bool> nans_equal = {}, Update<bool> signed_zeros_equal = {}) const; ``` ```c++ struct FloatComparisonPatch { std::optional<std::optional<double>> atol; std::optional<std::optional<int32_t>> ulp_distance; std::optional<bool> nans_equal; std::optional<bool> signed_zeros_equal; }; EqualOptions float_comparison( const FloatComparisonPatch& patch) const; //usage opts.float_comparison({ .atol = 1e-6, .nans_equal = true }); ``` ```c++ template <typename Fn> EqualOptions modify_float_comparison(Fn&& fn) const { auto res = *this; fn(res); return res; } // usage opts = opts.modify_float_comparison([](auto& o) { o.atol(1e-6); o.nans_equal(true); }); ``` ```c++ template <typename T> struct SetOrKeep { enum class State { Keep, SetNull, SetValue }; State state = State::Keep; T value{}; static SetOrKeep keep() { return {State::Keep, {}}; } static SetOrKeep set_null() { return {State::SetNull, {}}; } static SetOrKeep set(T v) { return {State::SetValue, std::move(v)}; } }; EqualOptions float_comparison( SetOrKeep<double> atol = SetOrKeep<double>::keep(), SetOrKeep<int32_t> ulp_distance = SetOrKeep<int32_t>::keep(), std::optional<bool> nans_equal = std::nullopt, std::optional<bool> signed_zero_equal = std::nullopt) const; ``` -- 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]
