Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feat : Apply macros in i16 data type #940

Merged
merged 5 commits into from
Oct 15, 2022
Merged

Feat : Apply macros in i16 data type #940

merged 5 commits into from
Oct 15, 2022

Conversation

ChobobDev
Copy link
Contributor

As we now have new casting rules and the macro, the following PR aims to refactor the previous i16 data type under new casting rules.

@ChobobDev ChobobDev marked this pull request as draft October 14, 2022 05:47
@ChobobDev ChobobDev changed the title Feat : Apply new casting rules in 16 data type Feat : Apply new casting rules in i16 data type Oct 14, 2022
@ChobobDev ChobobDev marked this pull request as ready for review October 14, 2022 07:48
@coveralls
Copy link

coveralls commented Oct 14, 2022 •
edited
Loading

Pull Request Test Coverage Report for Build 3255701499

  • 43 of 43 (100.0%) changed or added relevant lines in 2 files are covered.
  • 92 unchanged lines in 8 files lost coverage.
  • Overall coverage decreased (-0.2%) to 97.807%
Files with Coverage Reduction New Missed Lines %
core/src/executor/evaluate/error.rs 1 94.12%
pkg/rust/tests/glue.rs 1 98.84%
core/src/translate/query.rs 2 98.91%
core/src/executor/execute.rs 3 94.12%
core/src/data/row.rs 6 93.55%
test-suite/src/tester/mod.rs 19 87.5%
core/src/parse_sql.rs 26 43.48%
core/src/executor/aggregate/mod.rs 34 81.82%
Totals Coverage Status
Change from base Build 3247652740: -0.2%
Covered Lines: 37554
Relevant Lines: 38396
💛 - Coveralls

ever0de
ever0de reviewed Oct 14, 2022
View reviewed changes
Copy link
Member

@ever0de ever0de left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good. I left a few small comments.
[nit-picking] Instead of adding a macro to force the "new casting rule" in the PR title, apply the macro? It seems to fit the same thing.

core/src/data/value/binary_op/integer/i16.rs Outdated Show resolved Hide resolved
core/src/data/value/binary_op/integer/i16.rs Outdated Show resolved Hide resolved
core/src/data/value/binary_op/integer/macros.rs Show resolved Hide resolved
@ever0de ever0de added the improvement Improvements for existing features label Oct 14, 2022
@ChobobDev ChobobDev changed the title Feat : Apply new casting rules in i16 data type Feat : Apply macros in i16 data type Oct 15, 2022
ever0de
ever0de approved these changes Oct 15, 2022
View reviewed changes
Copy link
Member

@ever0de ever0de left a comment

Choose a reason for hiding this comment

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

LGTM!!!

panarch
panarch approved these changes Oct 15, 2022
View reviewed changes
Copy link
Member

@panarch panarch left a comment

Choose a reason for hiding this comment

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

Wow it is hard to believe that we can now have a new numeric type efficiently like this!
Appreciating again about the new macro @ever0de, thanks a lot @ChobobDev
All looks nice!

@panarch panarch merged commit c07bd61 into gluesql:main Oct 15, 2022
@ChobobDev ChobobDev deleted the feature/data_type_i16 branch November 5, 2022 05:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ever0de ever0de ever0de approved these changes

@panarch panarch panarch approved these changes

@devgony devgony Awaiting requested review from devgony

Assignees

@ChobobDev ChobobDev

Labels
improvement Improvements for existing features
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ChobobDev @coveralls @panarch @ever0de