Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

Proposal: Remove the code related to the Operator enumeration #527

Closed
yjhmelody opened this issue Oct 13, 2021 · 6 comments · Fixed by #607
Closed

Proposal: Remove the code related to the Operator enumeration #527

yjhmelody opened this issue Oct 13, 2021 · 6 comments · Fixed by #607
Labels
backwards-incompatible no-changelog Issues whose changes are covered by a PR and thus should not be shown in the changelog

Comments

@yjhmelody
Copy link
Contributor

yjhmelody commented Oct 13, 2021

I recommend removing the Operator enumeration.
If arrow users build their own computing systems, they will generally not use the operator enumeration values defined by arrow, because there are often differences in the number of operators defined, or differences in classification, such as the lack of bitwise operators, and there may be more in the future for arithmetic operator.
Or lack some traits implementation, such as serde.

It is very boilerplate code to distribute various operations based on Operator. If it can be removed, the amount of arrow code can be greatly reduced.

@jorgecarleitao
Copy link
Owner

I agree. The other issue is that some native types only implement some of the traits (e.g. Eq but not PartialOrd), making it difficult to even declare the function.

@sundy-li @ritchie46 @houqp @yjshen , thoughts?

@ritchie46
Copy link
Collaborator

I agree. 👍

@houqp
Copy link
Collaborator

houqp commented Oct 13, 2021

I agree it's a good idea to not make them part of the core so downstream won't be forced to pull them in. But I think there is value in keeping the code and convert them into a feature or self-contained crate for the following two reasons:

  • To be feature parity with the cpp implementation
  • For use-case where downstream just want an out of the box simple compute module to get stuff done without having to write these boilerplate code

@jorgecarleitao
Copy link
Owner

@houqp , note that the kernels themselves are kept. I think the proposal is:

use add_primitive(lhs, rhs) instead of arithmetics_primitive<T: NativeType>(lhs, Operator::Add, rhs).

The issue with arithmetics_primitive<T: NativeType>(lhs, Operator::Add, rhs) is that it requires T: Add + Sub + Mul + ..., which may not be fulfiled by all native types (e.g. months_days_ns is probably Add and Sub but not necessarily Div?).

An equivalent problem exists in comparison compare_primitive<T: NativeType>(lhs, Operator, rhs) requires T: PartialEq + PartialOrd but months_days_ns has a similar issue.

@yjhmelody
Copy link
Contributor Author

Maybe We can add #[deprecated] first, and then completely delete these APIs after a few versions.

@houqp
Copy link
Collaborator

houqp commented Oct 14, 2021

Yeah, I only took a quick scan on the CPP code and noticed they have these comparison operators defined. But on a second thought, I think doing something just because the CPP code is doing it is not a good technical argument.

Personally, I think writing add_primitive(lhs, rhs) is cleaner and more readable than arithmetics_primitive<T>(lhs, Operator::Add, rhs). Similarily, I would prefer compare_lt(a, b) over compare(a, b, Operator::Lt).

That said, the current operator related code does more than just operator based function dispatching though. It also handles array type matching to provide high level APIs centered around Array trait object like other compute modules. The latter is the what I was suggesting us to keep to make the interface more consistent across compute modules and make the library a little bit easier for new users to adopt for less complicated applications.

In short, I am 100% on board with removing all the operators.

@jorgecarleitao jorgecarleitao added the no-changelog Issues whose changes are covered by a PR and thus should not be shown in the changelog label Nov 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backwards-incompatible no-changelog Issues whose changes are covered by a PR and thus should not be shown in the changelog
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants