bkietz commented on a change in pull request #9294: URL: https://github.com/apache/arrow/pull/9294#discussion_r573793483
########## File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc ########## @@ -637,5 +637,77 @@ TYPED_TEST(TestBinaryArithmeticFloating, Mul) { this->AssertBinop(Multiply, "[null, 2.0]", this->MakeNullScalar(), "[null, null]"); } +TEST(TestBinaryArithmetic, DispatchBest) { + for (std::string name : {"add", "subtract", "multiply", "divide"}) { + for (std::string suffix : {"", "_checked"}) { + name += suffix; + + CheckDispatchBest(name, {int32(), int32()}, {int32(), int32()}); + CheckDispatchBest(name, {int32(), null()}, {int32(), int32()}); + CheckDispatchBest(name, {null(), int32()}, {int32(), int32()}); + + CheckDispatchBest(name, {int32(), int8()}, {int32(), int32()}); + CheckDispatchBest(name, {int32(), int16()}, {int32(), int32()}); + CheckDispatchBest(name, {int32(), int32()}, {int32(), int32()}); + CheckDispatchBest(name, {int32(), int64()}, {int64(), int64()}); + + CheckDispatchBest(name, {int32(), uint8()}, {int32(), int32()}); + CheckDispatchBest(name, {int32(), uint16()}, {int32(), int32()}); + CheckDispatchBest(name, {int32(), uint32()}, {int64(), int64()}); + CheckDispatchBest(name, {int32(), uint64()}, {int64(), int64()}); + + CheckDispatchBest(name, {uint8(), uint8()}, {uint8(), uint8()}); + CheckDispatchBest(name, {uint8(), uint16()}, {uint16(), uint16()}); + + CheckDispatchBest(name, {int32(), float32()}, {float32(), float32()}); + CheckDispatchBest(name, {float32(), int64()}, {float32(), float32()}); + CheckDispatchBest(name, {float64(), int32()}, {float64(), float64()}); + + CheckDispatchBest(name, {dictionary(int8(), float64()), float64()}, + {float64(), float64()}); + CheckDispatchBest(name, {dictionary(int8(), float64()), int16()}, + {float64(), float64()}); + } + } +} + +TEST(TestBinaryArithmetic, AddWithImplicitCasts) { + CheckScalarBinary("add", ArrayFromJSON(int32(), "[0, 1, 2, null]"), + ArrayFromJSON(float64(), "[0.25, 0.5, 0.75, 1.0]"), + ArrayFromJSON(float64(), "[0.25, 1.5, 2.75, null]")); + + CheckScalarBinary("add", ArrayFromJSON(int8(), "[-16, 0, 16, null]"), + ArrayFromJSON(uint32(), "[3, 4, 5, 7]"), + ArrayFromJSON(int64(), "[-13, 4, 21, null]")); + + CheckScalarBinary("add", + ArrayFromJSON(dictionary(int32(), int32()), "[8, 6, 3, null, 2]"), + ArrayFromJSON(uint32(), "[3, 4, 5, 7, 0]"), + ArrayFromJSON(int64(), "[11, 10, 8, null, 2]")); + + CheckScalarBinary("add", ArrayFromJSON(int32(), "[0, 1, 2, null]"), + std::make_shared<NullArray>(4), + ArrayFromJSON(int32(), "[null, null, null, null]")); + + CheckScalarBinary("add", ArrayFromJSON(dictionary(int32(), int8()), "[0, 1, 2, null]"), + ArrayFromJSON(uint32(), "[3, 4, 5, 7]"), + ArrayFromJSON(int64(), "[3, 5, 7, null]")); +} + +TEST(TestBinaryArithmetic, AddWithImplicitCastsUint64EdgeCase) { + // int64 is as wide as we can promote + CheckDispatchBest("add", {int8(), uint64()}, {int64(), int64()}); + + // this works sometimes + CheckScalarBinary("add", ArrayFromJSON(int8(), "[-1]"), ArrayFromJSON(uint64(), "[0]"), + ArrayFromJSON(int64(), "[-1]")); + + // ... but it can result in impossible implicit casts in the presence of uint64, since + // some uint64 values cannot be cast to int64: Review comment: Yes, on balance I think that for now at least it's fine to mark `uint64` with "I hope you know what you're doing" ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org