[BUG] GpuAdd and GpuMultiply do not include failOnError

[revans2]

Describe the bug
Add and Multiply added in failOnError fields.

This is similar to #2078 but for overflow. We either need to fall back to the GPU in these cases or implement the feature. Note that this impacts Canonicalization too.

[firestarman]

... We either need to fall back to the GPU in ...

fall back to the CPU ?

[revans2]

Yes that was a typo. We should fall back to the CPU unless we have a good way to detect overflow and we want to add it in.

[firestarman]

We can leverage the same solution with Java to detect overflow for “Add” op.
addExact(int x, int y) and addExact(long x, long y) are using the same algorithm to check if overflow occurs, which is likely to apply to Byte and Short.

Concerning the "Multiply" op, unfortunately it is much more complitcated, so we can fall back to CPU now.

Any idea ?

[revans2]

Yes we could detect a sign change like add exact is doing. This will not work for the AST version of add so we still will need to fall back in that case. We also will have to be careful with Decimal vs non-Decimal add.

[revans2]

I just saw that UnaryMinus also has a fail on error added in...

[revans2]

multiplyExact is more complicated, but it should work here too. We support everything that is a part of it. I would file a follow on issue though, for us to implement our own kernel to be able to do the checking ourselves.

[firestarman]

I just saw that UnaryMinus also has a fail on error added in...

So do almost all the expressions in file arithmetic.scala, e.g. Subtract, Abs...

[firestarman]

Yes we could detect a sign change like add exact is doing. This will not work for the AST version of add so we still will need to fall back in that case. We also will have to be careful with Decimal vs non-Decimal add.

Yes seems we need to always fall back to CPU when ansi is enabled. Since there is no reliable way to know whether the AST version will be used.
We can add failOnError support after implementing overflow checks in cuDF kernels.

[jlowe]

Since there is no reliable way to know whether the AST version will be used.

There's a separate tagForAst which controls the logic of tagging whether this can be converted to AST, so if you want to only allow conversion in the non-AST case that should be possible to do. tagForAst can check for failOnError and preclude use in the AST.

[firestarman]

There's a separate tagForAst which controls the logic of tagging whether this can be converted to AST, so if you want to only allow conversion in the non-AST case that should be possible to do. tagForAst can check for failOnError and preclude use in the AST.

Thanks Jason ,will check it.