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

Refactored comparison Operator #604

Merged
merged 1 commit into from Nov 14, 2021
Merged

Refactored comparison Operator #604

merged 1 commit into from Nov 14, 2021

Conversation

jorgecarleitao
Copy link
Owner

@jorgecarleitao jorgecarleitao commented Nov 13, 2021

This PR removes the Operator and replaces it by different function calls as proposed by #527 . This does not close that issue yet because a similar PR is necessary for the arithmetic kernels.

Backward incompatible changes

This PR removes the enum Operator in compute::comparison. Instead, the API is composed by

arrow2::compute::comparison::Y;
arrow2::compute::comparison::X::Y;
// where X \in {primitive, boolean, utf8, binary}
// where Y \in {eq, neq, lt, lt_eq, gt, gt_eq, eq_scalar, neq_scalar, lt_scalar, lt_eq_scalar, gt_scalar, gt_eq_scalar}

the functions inside each X are statically-typed (e.g. expect Utf8Array), while the functions arrow2::compute::comparison::Y are dynamically-typed (i.e. expect Array and Scalar).

As usual, each dynamic variant contains a can_Y(&DataType) that returns whether the operation is valid / is implemented.

Also, the functions no longer return a Result and instead simply panic; this is because:

  • there is no obvious recovery when the arrays have a different length; the user should check that before using these functions.
  • whether a type is valid for each function is already exposed in can_Y, which users can use to check whether the operation will panic or not.

This simplifies the overall API and allows implementing equality to types that have no partial or total order defined.

@codecov
Copy link

codecov bot commented Nov 13, 2021

Codecov Report

Merging #604 (5b8bc9f) into main (73ee16d) will increase coverage by 0.03%.
The diff coverage is 54.08%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #604      +/-   ##
==========================================
+ Coverage   79.37%   79.41%   +0.03%     
==========================================
  Files         401      401              
  Lines       24966    24792     -174     
==========================================
- Hits        19817    19688     -129     
+ Misses       5149     5104      -45     
Impacted Files Coverage Δ
src/compute/regex_match.rs 75.86% <ø> (ø)
src/compute/sort/utf8.rs 100.00% <ø> (ø)
src/compute/comparison/mod.rs 23.07% <18.91%> (-67.28%) ⬇️
src/compute/comparison/binary.rs 82.08% <64.70%> (+13.55%) ⬆️
src/compute/comparison/utf8.rs 82.08% <64.70%> (+13.55%) ⬆️
src/compute/comparison/boolean.rs 94.00% <72.72%> (+11.59%) ⬆️
src/array/utf8/mod.rs 87.17% <100.00%> (ø)
src/compute/comparison/primitive.rs 100.00% <100.00%> (+8.98%) ⬆️
tests/it/compute/comparison.rs 100.00% <100.00%> (ø)

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 73ee16d...5b8bc9f. Read the comment docs.

@houqp
Copy link
Collaborator

houqp commented Nov 14, 2021

great simplification, thanks @yjhmelody for the suggestion :)

@jorgecarleitao jorgecarleitao merged commit a09834e into main Nov 14, 2021
@jorgecarleitao jorgecarleitao deleted the rm_op branch November 14, 2021 07:30
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.

None yet

2 participants