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

refactor: Add wrappers around callbacks to improve syntax and debuggability #338

Merged
merged 17 commits into from
Dec 19, 2023

Conversation

paleolimbot
Copy link
Member

@paleolimbot paleolimbot commented Dec 14, 2023

This PR adds thin wrappers around C function pointers (like is done in Arrow C++) for Arrow C Data/Stream interface structures and uses them in all tests and internal code. The motivation for this was:

  • Calling x.release(&x) is a tiny bit verbose and involves specifying x twice
  • Wrappers make it easier to debug (by breakpoint or via debug checks)
  • Iterating through streams is very verbose if you want to catch all the error messages (in some places of the code it looks like I didn't bother, and in other places it looks like I assumed that the result of get_last_error was non-NULL, which is not true). The wrappers let you do NANOARROW_RETURN_NOT_OK(ArrowArrayStreamGetNext(stream, out, error)) which better integrates into the nanoarrow error handling pattern.

The changes are:

  • Release functions were added to nanoarrow_types.h
  • A few error handling functions were moved to nanoarrow_types.h since they were used in the wrappers
  • All function pointer calls I could find were updated to use the wrapper functions

@codecov-commenter
Copy link

codecov-commenter commented Dec 14, 2023

Codecov Report

Attention: 24 lines in your changes are missing coverage. Please review.

Comparison is base (3e02822) 88.15% compared to head (abec234) 88.12%.

Files Patch % Lines
src/nanoarrow/schema.c 41.66% 7 Missing ⚠️
.../nanoarrow_device/src/nanoarrow/nanoarrow_device.c 20.00% 4 Missing ⚠️
...anoarrow_ipc/src/nanoarrow/nanoarrow_ipc_decoder.c 42.85% 4 Missing ⚠️
src/nanoarrow/array.c 55.55% 4 Missing ⚠️
r/src/array_stream.c 60.00% 2 Missing ⚠️
src/nanoarrow/nanoarrow_types.h 96.22% 2 Missing ⚠️
...nanoarrow_ipc/src/nanoarrow/nanoarrow_ipc_reader.c 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #338      +/-   ##
==========================================
- Coverage   88.15%   88.12%   -0.03%     
==========================================
  Files          73       72       -1     
  Lines       11529    11579      +50     
==========================================
+ Hits        10163    10204      +41     
- Misses       1366     1375       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@paleolimbot paleolimbot marked this pull request as ready for review December 14, 2023 19:56
@paleolimbot paleolimbot force-pushed the helper-methods-better branch from 2bad164 to 2b70f9f Compare December 19, 2023 17:05
@paleolimbot paleolimbot merged commit 248b498 into apache:main Dec 19, 2023
@paleolimbot paleolimbot deleted the helper-methods-better branch December 19, 2023 17:27
@paleolimbot paleolimbot added this to the nanoarrow 0.4.0 milestone Jan 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants