Skip to content

Conversation

@edponce
Copy link

@edponce edponce commented Dec 31, 2021

Adds Between ternary kernel with internal dispatching of inclusive options.

@edponce
Copy link
Author

edponce commented Dec 31, 2021

@bkmgit This PR is incomplete (only supports input BooleanTypes) but it shows how to do dispatch at runtime based on input type and BetweenOptions. I plan to share a full version in the next 2 days.

This structure will allow implementing not_between as follows:

template <BetweenOptions::Inclusive Inclusive>
struct NotBetween {
  template <typename T, typename Arg0, typename Arg1, typename Arg2>
  static T Call(KernelContext*, const Arg0& middle, const Arg1& left, const Arg2& right, Status*) {
    static_assert(std::is_same<T, bool>::value && std::is_same<Arg0, Arg1>::value && std::is_same<Arg1, Arg2>::value, "");
    return !BetweenImpl<Inclusive>::Between(middle, left, right);
  }
};

@bkmgit
Copy link
Owner

bkmgit commented Jan 1, 2022

Thanks for the example.

@bkmgit
Copy link
Owner

bkmgit commented Jan 3, 2022

@edponce Am happy to merge this and continue developing it later today.

@edponce
Copy link
Author

edponce commented Jan 3, 2022

@bkmgit I have locally reworked this PR a bit, so I will update it in the next 2 hours and you can continue completing it. Does this works for you?

@bkmgit
Copy link
Owner

bkmgit commented Jan 3, 2022

That is fine. Can start in about 10 hours.

@edponce edponce marked this pull request as ready for review January 3, 2022 15:13
@edponce
Copy link
Author

edponce commented Jan 3, 2022

@bkmgit I pushed the most recent code in this PR. Feel free to continue development on top of this PR.

Two things to note:

  • Currently, the Between kernels registration duplicates the switch-case for BetweenOptions. This is the only way to do this at the moment, but can be simplified in the future by placing generator dispatchers in classes (I have a local example using this approach). I am working on a proposal for refactoring parts of the compute layer to simplify and make the codebase more consistent.
  • Tests are commented bc I did not update them, need to invoke CallFunction("between", inputs, options).

@bkmgit
Copy link
Owner

bkmgit commented Jan 3, 2022

@edponce Thanks.

@bkmgit bkmgit merged this pull request into bkmgit:ARROW-9843b Jan 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants