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

Made DynComparator Send+Sync #414

Merged
merged 1 commit into from
Sep 16, 2021

Conversation

yjshen
Copy link
Contributor

@yjshen yjshen commented Sep 16, 2021

This PR is a port of apache/arrow-rs#542 . As I quote the rationale here:

The cost of building a comparator (initialising a DynComparator) is often significantly higher than the actual cost of executing the comparator's closure on two row IDs. Therefore it makes sense to build the comparator once, and re-use the returned DynComparator for each row you are comparing the two arrays on.

Due to the explicit lifetime of DynComparator I recently found it problematic to store the DynComparator on an object that was used across threads in an async environment.

By refactoring the build_compare implementations I was able to remove the lifetime and it's much easier to use in the Datafusion operator I'm improving.

@jorgecarleitao jorgecarleitao changed the title Port arrow-rs#542: remove lifetime from DynComparator Made DynComparator Send+Sync Sep 16, 2021
@codecov
Copy link

codecov bot commented Sep 16, 2021

Codecov Report

Merging #414 (6d8c76c) into main (16a09ee) will decrease coverage by 0.00%.
The diff coverage is 86.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #414      +/-   ##
==========================================
- Coverage   80.96%   80.95%   -0.01%     
==========================================
  Files         347      347              
  Lines       22141    22135       -6     
==========================================
- Hits        17926    17920       -6     
  Misses       4215     4215              
Impacted Files Coverage Δ
src/array/ord.rs 64.21% <86.95%> (-2.13%) ⬇️

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 16a09ee...6d8c76c. Read the comment docs.

@jorgecarleitao jorgecarleitao merged commit 6c83a6d into jorgecarleitao:main Sep 16, 2021
@yjshen
Copy link
Contributor Author

yjshen commented Sep 16, 2021

Thank you @jorgecarleitao for the quick response. 👍

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.

2 participants