kuzudb / kuzu

Embeddable property graph database management system built for query speed and scalability. Implements Cypher.
https://kuzudb.com/
MIT License
1.38k stars 97 forks source link

Add int16 int32 support to get_as_arrow. #1483

Closed wenhoujx closed 1 year ago

wenhoujx commented 1 year ago

fixes #1482

add int16 and int32 support according to: https://arrow.apache.org/docs/format/CDataInterface.html#data-type-description-format-strings

please let me know where is the best place to add a test for this. (I haven't touched C++ for years ... )

I have read and agree to the terms under CLA.md.

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: +92.08 :tada:

Comparison is base (3e04497) 0.00% compared to head (7408e23) 92.08%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1483 +/- ## =========================================== + Coverage 0 92.08% +92.08% =========================================== Files 0 671 +671 Lines 0 23864 +23864 =========================================== + Hits 0 21974 +21974 - Misses 0 1890 +1890 ``` | [Impacted Files](https://codecov.io/gh/kuzudb/kuzu/pull/1483?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kuzudb) | Coverage Δ | | |---|---|---| | [src/common/arrow/arrow\_converter.cpp](https://codecov.io/gh/kuzudb/kuzu/pull/1483?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kuzudb#diff-c3JjL2NvbW1vbi9hcnJvdy9hcnJvd19jb252ZXJ0ZXIuY3Bw) | `63.55% <100.00%> (ø)` | | | [src/common/arrow/arrow\_row\_batch.cpp](https://codecov.io/gh/kuzudb/kuzu/pull/1483?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kuzudb#diff-c3JjL2NvbW1vbi9hcnJvdy9hcnJvd19yb3dfYmF0Y2guY3Bw) | `58.16% <100.00%> (ø)` | | ... and [669 files with indirect coverage changes](https://codecov.io/gh/kuzudb/kuzu/pull/1483/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kuzudb) Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kuzudb). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kuzudb)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

wenhoujx commented 1 year ago

@ray6080 thanks for the pointers, i updated the code and tests. Let me know if i need to further update. THanks !

ray6080 commented 1 year ago

@ray6080 thanks for the pointers, i updated the code and tests. Let me know if i need to further update. THanks !

Thanks! Looks good to me. Before merging the PR, can you manually squash your commits into one? We follow the principle of one commit for one pr, so it looks better on master commit history. We always do git rebase to squash commits in your branch into a single one first, then sync local master with the latest remote (if necessary), and git rebase master, finally force push the branch.

wenhoujx commented 1 year ago

great, i like squashing to one commit too.

done merging into one commit.

have you considered turn on github merge & squash option? https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/configuring-commit-squashing-for-pull-requests

ray6080 commented 1 year ago

have you considered turn on github merge & squash option? https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/configuring-commit-squashing-for-pull-requests

Good point, I think this will squash all commits of a pr into a single one, while we currently keep two for each. Though it doesn't matter, it's just that we didn't follow this way from day 1 😅

wenhoujx commented 1 year ago

y, that doesn't matter, thanks for reviewing my PR.