-
Notifications
You must be signed in to change notification settings - Fork 727
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
Add C data interface for decimal128 and timestamp #453
Conversation
@jorgecarleitao @kszucs I assume this needs integration tests (and docs if succeeded) with the C++ implementation, but I don't know how to add it. |
Codecov Report
@@ Coverage Diff @@
## master #453 +/- ##
==========================================
- Coverage 82.65% 82.61% -0.05%
==========================================
Files 165 165
Lines 45524 45602 +78
==========================================
+ Hits 37628 37673 +45
- Misses 7896 7929 +33
Continue to review full report at Codecov.
|
4c20853
to
6d9ea41
Compare
I think I found it (and added a test) |
e6a3b97
to
5540d6d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code looks good to me. Thank you @alippai
@jorgecarleitao could you please review this PR as I am not super familiar with the FFI bindings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a alot, @alippai , looks great! I left two small comments, but the implementation is solid! 👍
303b666
to
47c3c8c
Compare
de45abe
to
04c3d67
Compare
If you don't mind, I added an extra date32 integration test and timestamp support as well. |
Add extra date32 inttegration test
04c3d67
to
610ff6c
Compare
@jorgecarleitao @alamb all comments are addressed, I think this is ready now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thanks a lot, @alippai , excellent work 👍
* Add C data interface for decimal128 * Add timestamp support to C data interface Add extra date32 inttegration test
Which issue does this PR close?
Closes #413.
Rationale for this change
It's missing from the current API
What changes are included in this PR?
Handling of Decimal on the FFI is implemented
Are there any user-facing changes?
New datatype is added to a current interface