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.

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

feat!: Add TinyInt / i8 type #190

Merged
merged 32 commits into from
Oct 5, 2024

Conversation

Ashu999
Copy link
Contributor

@Ashu999 Ashu999 commented Sep 27, 2024

/closes #154
/claim #154

Rationale for this change

We already have SmallInt / i16, Int / i32, and BigInt / i64 data types supported. Adding an additional TinyInt / i8 data type would be helpful to some users and would additionally have (minor) performance benefits with those columns.

What changes are included in this PR?

Added a TinyInt variant to the various Column enum types.
Added a branch for handling TinyInt everywhere that SmallInt, Int, and BigInt are handled.

Are these changes tested?

Added unit tests covering the additional code.

@algora-pbc algora-pbc bot mentioned this pull request Sep 27, 2024
3 tasks
@Ashu999 Ashu999 changed the title feat: Add TinyInt / i8 type (WIP) feat: Add TinyInt / i8 type Sep 27, 2024
@Ashu999 Ashu999 marked this pull request as ready for review September 27, 2024 11:14
@Ashu999
Copy link
Contributor Author

Ashu999 commented Sep 27, 2024

@Dustin-Ray, I have a question regarding tests:

There are some tests in codebase in which: either one of integer type is tested against any other non-integer type.
(eg. SmallInt, Decimal)

should i add a similar test with TinyInt there? (like TinyInt, Decimal) ?

@Ashu999
Copy link
Contributor Author

Ashu999 commented Sep 27, 2024

@Dustin-Ray changes can be reviewed now

@Dustin-Ray
Copy link
Contributor

should i add a similar test with TinyInt there? (like TinyInt, Decimal) ?

In general yes, we require thorough tests to cover every case possible. The tests you linked to are surrounding OwnedColumn operations and should have some coverage for Tiny int where possible.

@Dustin-Ray
Copy link
Contributor

@Dustin-Ray changes can be reviewed now

Please check that the CI passes locally, check, tests, and clippy are currently failing. Ping me when these are fixed and I will run CI again

@Ashu999
Copy link
Contributor Author

Ashu999 commented Sep 28, 2024

cargo check, clippy, test passing locally. Now I'm adding more tests as discussed above.

@Dustin-Ray
Copy link
Contributor

cargo check, clippy, test passing locally. Now I'm adding more tests as discussed above.

we have custom ci commands, you need to run:

cargo clippy --all-targets --all-features -- -D warnings
cargo check --all-targets --all-features
cargo test --all-features

@Ashu999
Copy link
Contributor Author

Ashu999 commented Sep 28, 2024

ran above mentioned commands, they are passing.

added as many tests as possible, let me know if i missed any @Dustin-Ray

@Dustin-Ray
Copy link
Contributor

ran above mentioned commands, they are passing.

great thanks, will review asap

@Dustin-Ray
Copy link
Contributor

LGTM - Thanks!

@iajoiner let us know what you think then we can maybe wrap this one up

iajoiner
iajoiner previously approved these changes Oct 4, 2024
@iajoiner iajoiner changed the title feat: Add TinyInt / i8 type feat!: Add TinyInt / i8 type Oct 4, 2024
@iajoiner iajoiner requested a review from JayWhite2357 October 4, 2024 19:27
JayWhite2357
JayWhite2357 previously approved these changes Oct 5, 2024
Copy link
Contributor

@JayWhite2357 JayWhite2357 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!
You will need to resolve the branch conflicts with main. Once that is done, I'll need to reapprove.

@Ashu999 Ashu999 dismissed stale reviews from JayWhite2357, iajoiner, and Dustin-Ray via 9a25a18 October 5, 2024 03:47
@Ashu999
Copy link
Contributor Author

Ashu999 commented Oct 5, 2024

done @JayWhite2357

@Ashu999 Ashu999 requested a review from JayWhite2357 October 5, 2024 03:51
JayWhite2357
JayWhite2357 previously approved these changes Oct 5, 2024
Copy link
Contributor

@JayWhite2357 JayWhite2357 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

@JayWhite2357 JayWhite2357 enabled auto-merge (squash) October 5, 2024 14:30
@JayWhite2357 JayWhite2357 merged commit 48b2f0b into spaceandtimelabs:main Oct 5, 2024
11 checks passed
Copy link

github-actions bot commented Oct 5, 2024

🎉 This PR is included in version 0.28.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@JayWhite2357
Copy link
Contributor

/approve
@Ashu999 #229 may be a similar issue that you could tackle next

Copy link

algora-pbc bot commented Oct 7, 2024

@JayWhite2357: The claim has been successfully added to reward-all. You can visit your dashboard to complete the payment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add TinyInt / i8 type
4 participants