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

Added lower #641

Merged
merged 7 commits into from
Dec 6, 2021
Merged

Added lower #641

merged 7 commits into from
Dec 6, 2021

Conversation

Xuanwo
Copy link
Contributor

@Xuanwo Xuanwo commented Nov 27, 2021

Signed-off-by: Xuanwo github@xuanwo.io

Part of #635

Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
@codecov
Copy link

codecov bot commented Nov 27, 2021

Codecov Report

Merging #641 (6eba31a) into main (b853d95) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 6eba31a differs from pull request most recent head fa9693b. Consider uploading reports for the commit fa9693b to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #641      +/-   ##
==========================================
+ Coverage   69.91%   69.94%   +0.02%     
==========================================
  Files         299      300       +1     
  Lines       16628    16644      +16     
==========================================
+ Hits        11625    11641      +16     
  Misses       5003     5003              
Impacted Files Coverage Δ
src/compute/lower.rs 100.00% <100.00%> (ø)
src/datatypes/schema.rs 37.93% <0.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 b853d95...fa9693b. Read the comment docs.

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.

Thanks a lot! Left some comments

src/compute/lower.rs Outdated Show resolved Hide resolved
src/compute/lower.rs Outdated Show resolved Hide resolved
src/compute/lower.rs Outdated Show resolved Hide resolved
Co-authored-by: Jorge Leitao <jorgecarleitao@gmail.com>
src/compute/lower.rs Outdated Show resolved Hide resolved
src/compute/lower.rs Outdated Show resolved Hide resolved
@ghuls
Copy link
Contributor

ghuls commented Dec 2, 2021

Shouldn't the test include some real UTF8 characters instead of only the ASCII subset?
Like some of the characters listed here: https://www.ibm.com/docs/en/i/7.2?topic=tables-unicode-lowercase-uppercase-conversion-mapping-table

Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo
Copy link
Contributor Author

Xuanwo commented Dec 6, 2021

Shouldn't the test include some real UTF8 characters instead of only the ASCII subset?

Addressed, thanks for advice.

Signed-off-by: Xuanwo <github@xuanwo.io>
@jorgecarleitao jorgecarleitao merged commit 998882e into jorgecarleitao:main Dec 6, 2021
@jorgecarleitao jorgecarleitao changed the title compute: Add lower support Added lower Dec 6, 2021
@jorgecarleitao jorgecarleitao added the feature A new feature label Dec 6, 2021
@jorgecarleitao
Copy link
Owner

Thanks a lot, really well tested! 💯

@Xuanwo Xuanwo deleted the lower branch December 7, 2021 02:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants