Add ArrowArrayViewGetIntervalUnsafe #270
Conversation
Codecov Report
@@ Coverage Diff @@
## main #270 +/- ##
==========================================
- Coverage 87.14% 87.14% -0.01%
==========================================
Files 63 66 +3
Lines 9750 10129 +379
==========================================
+ Hits 8497 8827 +330
- Misses 1253 1302 +49
... and 3 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
src/nanoarrow/array_inline.h
Outdated
| break; | ||
| } | ||
| default: | ||
| out = NULL; |
There was a problem hiding this comment.
maybe memset(out, 0, sizeof(struct ArrowInterval)?
There was a problem hiding this comment.
Or setting all components to 0? Or nothing? It's undefined behaviour but I'm not sure what out = NULL would actually do.
There was a problem hiding this comment.
I think the downside to setting to 0 is that it is ambiguous if you've had an error or an interval is legitimately 0
There was a problem hiding this comment.
Agreed... perhaps nothing then so that nobody relies on that behavior?
There was a problem hiding this comment.
Sure removed that for now. Another thought I had was returning an ArrowInterval; that would break the usage pattern with GetDecimalUnsafe but arguably be more consistent with GetStringUnsafe, GetBytesUnsafe, etc...
Those methods do assign some members to NULL so the caller has a way of checking if they've made a mistake
There was a problem hiding this comment.
I think the motivation there was to make it more likely that some test would fail...perhaps some future documentation could make it more clear that it's the caller's responsibility to type check first (in theory that's what the Unsafe bit is trying to communicate).
Returning the ArrowInterval is definitely an option...my hunch is that it would be inefficient since it you'd have to potentially copy a few of the fields for every item in the loop; however, I have no evidence for that. I think what you have here is a good compromise to support all three interval types. If there's overwhelming feedback suggesting that it's confusing or slow it can always be changed pre 1.0.
Follow on to #258