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

Simplified arithmetics compute #607

Merged
merged 3 commits into from
Nov 24, 2021
Merged

Simplified arithmetics compute #607

merged 3 commits into from
Nov 24, 2021

Conversation

jorgecarleitao
Copy link
Owner

Closes #527.

The functions are now offered based on the name instead of Operator (which was removed).

Also, the functions no longer error, and instead panic. As usual, can_* can be used to check if the operation is supported, and checking the len of the arrays is required.

@codecov
Copy link

codecov bot commented Nov 15, 2021

Codecov Report

Merging #607 (800b1f7) into main (b3ed162) will increase coverage by 0.22%.
The diff coverage is 91.03%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #607      +/-   ##
==========================================
+ Coverage   69.70%   69.93%   +0.22%     
==========================================
  Files         299      300       +1     
  Lines       16599    16835     +236     
==========================================
+ Hits        11571    11773     +202     
- Misses       5028     5062      +34     
Impacted Files Coverage Δ
benches/arithmetic_kernels.rs 0.00% <0.00%> (ø)
src/compute/arithmetics/basic/mod.rs 0.00% <0.00%> (ø)
src/compute/arithmetics/decimal/mod.rs 100.00% <ø> (ø)
src/compute/arithmetics/time.rs 44.23% <50.00%> (-0.67%) ⬇️
src/compute/arithmetics/basic/div.rs 77.96% <80.00%> (-0.73%) ⬇️
src/compute/arithmetics/basic/add.rs 89.36% <93.75%> (+0.90%) ⬆️
src/compute/arithmetics/basic/mul.rs 89.36% <93.75%> (+0.90%) ⬆️
src/compute/arithmetics/basic/sub.rs 89.36% <93.75%> (+0.90%) ⬆️
src/compute/arithmetics/mod.rs 96.66% <96.55%> (+70.86%) ⬆️
src/compute/arithmetics/basic/rem.rs 84.90% <100.00%> (-0.55%) ⬇️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3ed162...800b1f7. Read the comment docs.

@jorgecarleitao jorgecarleitao force-pushed the result_arithmetics branch 4 times, most recently from 78c1770 to 34e0eba Compare November 15, 2021 17:08
@jorgecarleitao jorgecarleitao marked this pull request as ready for review November 15, 2021 20:00
Copy link
Contributor

@yjhmelody yjhmelody left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but if you can use expect to prompt the user that the type does not match, it will be better than unwrap directly

@jorgecarleitao
Copy link
Owner Author

@yjhmelody , thanks! Could you clarify what do you mean with

the user that the type does not match, it will be better than unwrap directly

So, the design here is: the statically-typed versions of the operators do not check for the type; the dynamically-typed versions of the operators check for type.

I am uncertain about this choice, though: my hypothesis here:

  1. query engines know the logical types associated to the arrays
  2. they also know whether an operation is valid for a given operator (via can_*)

thus, they can match for a valid datatype via

if can_add(array1.data_type(), array2.data_type()) {
    Ok(add(array1, array2))
} else {
    // custom add for arrow2-unsupported type or error
}

Could you clarify your thoughts on this? Also @houqp @ritchie46 @sundy-li

@yjhmelody
Copy link
Contributor

I agree with you. I just suggest not to use the unwrap method directly, but to use the expect method

@jorgecarleitao
Copy link
Owner Author

iirc unwrapping a Result and will show the result's error message on the panic.

@jorgecarleitao jorgecarleitao merged commit a8af882 into main Nov 24, 2021
@jorgecarleitao jorgecarleitao deleted the result_arithmetics branch November 24, 2021 19:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: Remove the code related to the Operator enumeration
2 participants