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

Compute: add partial option into CastOptions #561

Merged
merged 5 commits into from
Nov 4, 2021

Conversation

sundy-li
Copy link
Collaborator

@sundy-li sundy-li commented Oct 31, 2021

Changes:

  • Add partial option into CastOptions, this is trying to be compatible with MySQL behavior, 123 abc - > 123
  • Try to make CastOptions pub again

closes #569

@codecov
Copy link

codecov bot commented Oct 31, 2021

Codecov Report

Merging #561 (5d38f22) into main (5fc843d) will increase coverage by 0.12%.
The diff coverage is 94.01%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #561      +/-   ##
==========================================
+ Coverage   78.92%   79.04%   +0.12%     
==========================================
  Files         382      395      +13     
  Lines       24276    24540     +264     
==========================================
+ Hits        19160    19398     +238     
- Misses       5116     5142      +26     
Impacted Files Coverage Δ
src/compute/cast/dictionary_to.rs 25.00% <12.50%> (-2.03%) ⬇️
src/compute/cast/binary_to.rs 80.00% <100.00%> (+4.13%) ⬆️
src/compute/cast/mod.rs 91.34% <100.00%> (+5.72%) ⬆️
src/compute/cast/utf8_to.rs 89.39% <100.00%> (+1.06%) ⬆️
tests/it/compute/cast.rs 99.35% <100.00%> (+0.05%) ⬆️
src/array/binary/ffi.rs 60.71% <0.00%> (-23.91%) ⬇️
src/array/primitive/ffi.rs 58.33% <0.00%> (-16.67%) ⬇️
src/array/utf8/ffi.rs 64.28% <0.00%> (-14.67%) ⬇️
src/array/list/ffi.rs 65.51% <0.00%> (-14.49%) ⬇️
src/ffi/ffi.rs 85.41% <0.00%> (-9.90%) ⬇️
... and 52 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 5fc843d...5d38f22. Read the comment docs.

@sundy-li
Copy link
Collaborator Author

sundy-li commented Nov 2, 2021

@jorgecarleitao Hello, is there any problem with this change?

Copy link
Owner

@jorgecarleitao jorgecarleitao left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. Missed this one. Thanks a lot for the PR!

Was not aware of the partial; cool concept. Left some comments: imo we should break the API, make CastOptions first class, and remove the old APIs that were branching on the wrapping, as we now have 4 cases to cater for.

src/compute/cast/binary_to.rs Outdated Show resolved Hide resolved
src/compute/cast/utf8_to.rs Outdated Show resolved Hide resolved
src/compute/cast/utf8_to.rs Outdated Show resolved Hide resolved
src/compute/cast/mod.rs Outdated Show resolved Hide resolved
@jorgecarleitao jorgecarleitao merged commit 3c10b16 into jorgecarleitao:main Nov 4, 2021
@jorgecarleitao
Copy link
Owner

Looks awesome! Thanks a lot for the PR.

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.

Made cast accept CastOptions parameter
3 participants