Skip to content

ARROW-1562: [C++] Numeric kernel implementations for add - #5021

Closed
fexolm wants to merge 5 commits into
apache:masterfrom
fexolm:ARROW-1562
Closed

ARROW-1562: [C++] Numeric kernel implementations for add#5021
fexolm wants to merge 5 commits into
apache:masterfrom
fexolm:ARROW-1562

Conversation

@fexolm

Copy link
Copy Markdown
Contributor

It's just an initial AddKernel implementation (work in progress). I could not come up with a better implementation of type inference, so I want to hear your thoughts about implementation.

@fsaintjacquesfsaintjacques changed the title ARROW-1562: Numeric kernel implementations for addARROW-1562: [C++] Numeric kernel implementations for addAug 6, 2019
@fsaintjacques

fsaintjacques commented Aug 6, 2019

Copy link
Copy Markdown
Contributor

Multiple notes:

  1. The Compare kernels only support comparing the same type, it expects the caller to decide (via Cast kernel) how to deal with mis-matching types. I think this would be the most simple and sane behavior.
  2. The Sum kernels upcast to the widest type. This might be less of an issue here, but I think we should take a decision.
  3. I wouldn't merge this without supporting null, this is too frequent to avoid.
  4. This code is regular enough to support other basic arithmetic operations, i.e Addition, Substraction, Multiplication, Division.
  5. The code is also regular enough to support (array, scalar) operation. See how this is implemented for Compare kernels.

@fsaintjacquesfsaintjacques changed the title ARROW-1562: [C++] Numeric kernel implementations for addARROW-1562: [WIP][C++] Numeric kernel implementations for addAug 6, 2019
@emkornfield

Copy link
Copy Markdown
Contributor

@fexolm it looks like @fsaintjacques gave some feedback on implementation do you need more guidance?

@fexolm

Copy link
Copy Markdown
ContributorAuthor

@emkornfield no, I just can not find the time to finish this. I will return to this as soon as I get time.

@fexolm
fexolm marked this pull request as ready for review October 18, 2019 11:50
@fexolm

Copy link
Copy Markdown
ContributorAuthor

@fsaintjacques, @emkornfield
I've removed type inference from the code, also add nulls support.
BTW I haven't decided yet how it can be architecturally generalized to support other arithmetic operations. I see, that almost all the code could be reused in other operations, but not sure how to implement it.
Maybe it would be better to do this in other pr, because I'm not sure I have enough time for it now.

@fexolmfexolm changed the title ARROW-1562: [WIP][C++] Numeric kernel implementations for addARROW-1562: [C++] Numeric kernel implementations for addOct 18, 2019
@fexolm
fexolmforce-pushed the ARROW-1562 branch 2 times, most recently from 7a6f7e1 to a60b42fCompareOctober 18, 2019 12:42
@emkornfield

Copy link
Copy Markdown
Contributor

Thanks @fexolm@fsaintjacques do you have time to review?

@fsaintjacques

Copy link
Copy Markdown
Contributor

Will review again.

@fsaintjacques

Copy link
Copy Markdown
Contributor

I created https://issues.apache.org/jira/browse/ARROW-7017 as a followup.

Sign up for freeto 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.

3 participants

@fexolm@fsaintjacques@emkornfield