Add metadata builder functions #12
Conversation
paleolimbot
force-pushed
the
build-metadata
branch
from
a3c3ddc to
66073ee
Compare
Codecov Report
@@ Coverage Diff @@
## main #12 +/- ##
==========================================
- Coverage 91.97% 90.32% -1.66%
==========================================
Files 5 6 +1
Lines 798 930 +132
Branches 30 38 +8
==========================================
+ Hits 734 840 +106
- Misses 41 59 +18
- Partials 23 31 +8
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
src/nanoarrow/metadata.c
Outdated
| return NANOARROW_OK; | ||
| } | ||
|
|
||
| ArrowErrorCode ArrowMetadataBuilderAppendView(struct ArrowBuffer* buffer, |
There was a problem hiding this comment.
Worth possibly exposing this variant too?
There was a problem hiding this comment.
I pushed a compromise of sorts...all values are now ArrowStringView but all keys stay const char*, which is somewhere inbetween the programmability and non-null terminated-ness of the ArrowStringView and the reality that keys are almost always "a literal string". I tried having both be ArrowStringView but that was rather painful to actually use.
There was a problem hiding this comment.
Wonder if a helper macro to convert const char* to struct ArrowStringView might help work around that?
There was a problem hiding this comment.
I added ArrowCharView():
arrow-nanoarrow/src/nanoarrow/nanoarrow.h
Lines 237 to 238 in 1a4bb88
It's still a little awkward feeling if I convert keys to use struct ArrowStringView but I don't really mind either way.
There was a problem hiding this comment.
not a big deal for me either, though it is a little awkward to have the types differ between key/value even if there is a pragmatic reason
There was a problem hiding this comment.
I'll change them to be the string views...anybody looking for convenience isn't writing C code anyway!
| if (value == NULL) { | ||
| return NANOARROW_OK; | ||
| } |
There was a problem hiding this comment.
Hmm, to me it's a little weird to accept NULL as the value and then just do nothing with it. If we just considered append(key, NULL) to be an error, we could drop this, and then we could pass the views by value instead of indirecting through a pointer
There was a problem hiding this comment.
I moved these to be internal implementation details...the NULL mostly just helps with minimizing the number of times one has to copy a metadata string (correspondingly, I added ArrowMetadatBuilderRemove() to make it more explicit).
paleolimbot
force-pushed
the
build-metadata
branch
from
eae585a to
56f63d8
Compare
src/nanoarrow/typedefs_inline.h
Outdated
| out.data = value; | ||
| out.n_bytes = 0; | ||
| if (value) { | ||
| while (*value++) { |
There was a problem hiding this comment.
why not strlen? to avoid the include?
There was a problem hiding this comment.
Yes...too much of a hack? I'll check if string.h gets included in another inline file anyway (it probably does).
There was a problem hiding this comment.
+I don't think it's a big deal to include string.h (it'll certainly raise less eyebrows than this)
3 participants
Fixes #6.
This PR adds functions
ArrowMetadataBuilderAppend()(for blindly but efficiently adding a key/value pair to the end of some metadata) andArrowMetadataBuilderSet()(to less efficiently replace or remove a value for key).The use case I had in mind is building extension type metadata from some input (i.e., make an output schema like the input except with new extension type or with new serialized extension type metadata). It's rather difficult to replicate the "replace" or "remove" behaviour otherwise.