From be3d432ef08fe71fa6d591820b966da225da40a7 Mon Sep 17 00:00:00 2001 From: Dewey Dunnington Date: Mon, 8 Jan 2024 20:00:00 +0000 Subject: [PATCH] chore: Enable more compiler warnings (#351) As noted in https://github.com/apache/arrow-nanoarrow/issues/224, we can use a format check attribute to ensure that format strings/types match and check that `ArrowErrorCode` is always checked. While I was in it for compiler warnings, I enabled a few more that recently caused a problem in the Arrow R package. To minimize impact on existing usage, these are only enabled when `NANOARROW_DEBUG` is defined (as it is for all CMake debug builds and in our CI). --- .github/workflows/r-check.yaml | 16 ++++++------- CMakeLists.txt | 10 ++++++--- r/R/buffer.R | 11 +++++++-- r/src/altrep.c | 2 +- r/src/array.c | 14 ++++++------ r/src/array_view.c | 2 +- r/src/as_array.c | 32 +++++++++++++++----------- r/src/buffer.c | 9 +++++--- r/src/buffer.h | 4 ++-- r/src/convert_array_stream.c | 11 +++++++-- r/src/materialize.c | 10 ++++----- r/src/materialize_chr.h | 2 +- r/src/materialize_common.h | 2 +- r/src/materialize_int.h | 6 ++--- r/src/pointers.c | 8 +++++-- r/src/schema.c | 31 +++++++++++++++++-------- r/src/util.c | 4 ++-- r/src/util.h | 4 ++-- r/tests/testthat/test-buffer.R | 5 +++++ src/nanoarrow/array.c | 8 +++---- src/nanoarrow/array_inline.h | 4 ++-- src/nanoarrow/array_test.cc | 10 ++++----- src/nanoarrow/buffer_inline.h | 24 ++++++++++---------- src/nanoarrow/buffer_test.cc | 2 +- src/nanoarrow/nanoarrow.h | 9 +++++++- src/nanoarrow/nanoarrow_hpp_test.cc | 6 ++--- src/nanoarrow/nanoarrow_types.h | 30 +++++++++++++++++++++++++ src/nanoarrow/schema.c | 35 +++++++++++++++++------------ src/nanoarrow/schema_test.cc | 18 +++++++-------- 29 files changed, 211 insertions(+), 118 deletions(-) diff --git a/.github/workflows/r-check.yaml b/.github/workflows/r-check.yaml index c4fe69d4f..d13cd580c 100644 --- a/.github/workflows/r-check.yaml +++ b/.github/workflows/r-check.yaml @@ -60,15 +60,15 @@ jobs: http-user-agent: ${{ matrix.config.http-user-agent }} use-public-rspm: true - - name: Install valgrind on r-devel - if: matrix.config.r == 'devel' - run: sudo apt-get install -y valgrind - - # Until new pkgbuild is on CRAN, we do the install step to ensure - # the C library is vendored from this commit - - name: Vendor nanoarrow into the R package + # Check package install with extra warning flags except on Windows, + # where the R headers cause some problems + - name: Check R CMD INSTALL + if: matrix.config.os != 'windows-latest' + env: + PKG_CPPFLAGS: "-DNANOARROW_DEBUG" + PKG_CFLAGS: "-Werror -Wall -Wextra -Wpedantic -Wconversion -Wno-unused-parameter -Wno-sign-conversion -Wno-cast-function-type" run: | - R CMD INSTALL r + R CMD INSTALL r --preclean shell: bash - uses: r-lib/actions/setup-r-dependencies@v2 diff --git a/CMakeLists.txt b/CMakeLists.txt index 1e44b6fee..e72225d9c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -127,20 +127,24 @@ else() PRIVATE -Wall -Werror -Wextra + -Wpedantic -Wno-type-limits -Wno-unused-parameter -Wmaybe-uninitialized - -Wpedantic - -Wunused-result) + -Wunused-result + -Wconversion + -Wno-sign-conversion) elseif(CMAKE_C_COMPILER_ID STREQUAL "AppleClang" OR CMAKE_C_COMPILER_ID STREQUAL "Clang") target_compile_options(nanoarrow PRIVATE -Wall -Werror -Wextra + -Wpedantic -Wdocumentation -Wno-unused-parameter - -Wshorten-64-to-32) + -Wconversion + -Wno-sign-conversion) endif() endif() diff --git a/r/R/buffer.R b/r/R/buffer.R index d3b15cd6b..6fc7e4f76 100644 --- a/r/R/buffer.R +++ b/r/R/buffer.R @@ -42,14 +42,21 @@ as_nanoarrow_buffer.nanoarrow_buffer <- function(x, ...) { #' @export as_nanoarrow_buffer.default <- function(x, ...) { + msg <- NULL result <- tryCatch( .Call(nanoarrow_c_as_buffer_default, x), - error = function(...) NULL + error = function(e) { + msg <<- conditionMessage(e) + NULL + } ) - if (is.null(result)) { + if (is.null(result) && is.null(msg)) { cls <- paste(class(x), collapse = "/") stop(sprintf("Can't convert object of type %s to nanoarrow_buffer", cls)) + } else if (is.null(result)) { + cls <- paste(class(x), collapse = "/") + stop(sprintf("Can't convert object of type %s to nanoarrow_buffer: %s", cls, msg)) } result diff --git a/r/src/altrep.c b/r/src/altrep.c index b4c1c5c00..94fd72031 100644 --- a/r/src/altrep.c +++ b/r/src/altrep.c @@ -77,7 +77,7 @@ static SEXP nanoarrow_altstring_elt(SEXP altrep_sexp, R_xlen_t i) { } struct ArrowStringView item = ArrowArrayViewGetStringUnsafe(&converter->array_view, i); - return Rf_mkCharLenCE(item.data, item.size_bytes, CE_UTF8); + return Rf_mkCharLenCE(item.data, (int)item.size_bytes, CE_UTF8); } static SEXP nanoarrow_altstring_materialize(SEXP altrep_sexp) { diff --git a/r/src/array.c b/r/src/array.c index 14e4ca610..0b84cd5f3 100644 --- a/r/src/array.c +++ b/r/src/array.c @@ -55,7 +55,7 @@ SEXP nanoarrow_c_array_set_length(SEXP array_xptr, SEXP length_sexp) { Rf_error("array$length must be finite and greater than zero"); } - array->length = length; + array->length = (int64_t)length; return R_NilValue; } @@ -70,7 +70,7 @@ SEXP nanoarrow_c_array_set_null_count(SEXP array_xptr, SEXP null_count_sexp) { Rf_error("array$null_count must be finite and greater than -1"); } - array->null_count = null_count; + array->null_count = (int64_t)null_count; return R_NilValue; } @@ -85,7 +85,7 @@ SEXP nanoarrow_c_array_set_offset(SEXP array_xptr, SEXP offset_sexp) { Rf_error("array$offset must be finite and greater than zero"); } - array->offset = offset; + array->offset = (int64_t)offset; return R_NilValue; } @@ -220,8 +220,8 @@ static int move_array_buffers(struct ArrowArray* src, struct ArrowArray* dst, dst->offset = src->offset; if (src->n_buffers != dst->n_buffers) { - ArrowErrorSet(error, "Expected %ld buffer(s) but got %ld", dst->n_buffers, - src->n_buffers); + ArrowErrorSet(error, "Expected %ld buffer(s) but got %ld", (long)dst->n_buffers, + (long)src->n_buffers); return EINVAL; } @@ -230,8 +230,8 @@ static int move_array_buffers(struct ArrowArray* src, struct ArrowArray* dst, } if (src->n_children != dst->n_children) { - ArrowErrorSet(error, "Expected %ld child(ren) but got %ld", dst->n_children, - src->n_children); + ArrowErrorSet(error, "Expected %ld child(ren) but got %ld", (long)dst->n_children, + (long)src->n_children); return EINVAL; } diff --git a/r/src/array_view.c b/r/src/array_view.c index 031e06bc6..46b478026 100644 --- a/r/src/array_view.c +++ b/r/src/array_view.c @@ -39,7 +39,7 @@ SEXP nanoarrow_c_array_view(SEXP array_xptr, SEXP schema_xptr) { struct ArrowSchema* schema = nanoarrow_schema_from_xptr(schema_xptr); struct ArrowError error; - ArrowErrorSet(&error, ""); + ArrowErrorInit(&error); struct ArrowArrayView* array_view = (struct ArrowArrayView*)ArrowMalloc(sizeof(struct ArrowArrayView)); diff --git a/r/src/as_array.c b/r/src/as_array.c index 0f7563e76..f6f3a98f0 100644 --- a/r/src/as_array.c +++ b/r/src/as_array.c @@ -95,7 +95,7 @@ static void as_array_int(SEXP x_sexp, struct ArrowArray* array, SEXP schema_xptr for (int64_t i = first_null; i < len; i++) { uint8_t is_valid = x_data[i] != NA_INTEGER; null_count += !is_valid; - ArrowBitmapAppend(&bitmap, is_valid, 1); + ArrowBitmapAppendUnsafe(&bitmap, is_valid, 1); } ArrowArraySetValidityBitmap(array, &bitmap); @@ -141,13 +141,16 @@ static void as_array_lgl(SEXP x_sexp, struct ArrowArray* array, SEXP schema_xptr for (int64_t i = 0; i < len; i++) { if (x_data[i] == NA_INTEGER) { has_nulls = 1; - ArrowBitmapAppend(&value_bitmap, 0, 1); + ArrowBitmapAppendUnsafe(&value_bitmap, 0, 1); } else { - ArrowBitmapAppend(&value_bitmap, x_data[i] != 0, 1); + ArrowBitmapAppendUnsafe(&value_bitmap, x_data[i] != 0, 1); } } - ArrowArraySetBuffer(array, 1, &value_bitmap.buffer); + result = ArrowArraySetBuffer(array, 1, &value_bitmap.buffer); + if (result != NANOARROW_OK) { + Rf_error("ArrowArraySetBuffer() failed"); + } // Set the array fields array->length = len; @@ -166,7 +169,7 @@ static void as_array_lgl(SEXP x_sexp, struct ArrowArray* array, SEXP schema_xptr for (int64_t i = 0; i < len; i++) { uint8_t is_valid = x_data[i] != NA_INTEGER; null_count += !is_valid; - ArrowBitmapAppend(&bitmap, is_valid, 1); + ArrowBitmapAppendUnsafe(&bitmap, is_valid, 1); } ArrowArraySetValidityBitmap(array, &bitmap); @@ -219,7 +222,7 @@ static void as_array_dbl(SEXP x_sexp, struct ArrowArray* array, SEXP schema_xptr if (R_IsNA(x_data[i]) || R_IsNaN(x_data[i])) { buffer_data[i] = 0; } else { - buffer_data[i] = x_data[i]; + buffer_data[i] = (int64_t)x_data[i]; } } @@ -245,7 +248,7 @@ static void as_array_dbl(SEXP x_sexp, struct ArrowArray* array, SEXP schema_xptr n_overflow++; buffer_data[i] = 0; } else { - buffer_data[i] = x_data[i]; + buffer_data[i] = (int32_t)x_data[i]; } } @@ -283,7 +286,7 @@ static void as_array_dbl(SEXP x_sexp, struct ArrowArray* array, SEXP schema_xptr for (int64_t i = first_null; i < len; i++) { uint8_t is_valid = !R_IsNA(x_data[i]) && !R_IsNaN(x_data[i]); null_count += !is_valid; - ArrowBitmapAppend(&bitmap, is_valid, 1); + ArrowBitmapAppendUnsafe(&bitmap, is_valid, 1); } ArrowArraySetValidityBitmap(array, &bitmap); @@ -316,7 +319,10 @@ static void as_array_chr(SEXP x_sexp, struct ArrowArray* array, SEXP schema_xptr struct ArrowBuffer* offset_buffer = ArrowArrayBuffer(array, 1); struct ArrowBuffer* data_buffer = ArrowArrayBuffer(array, 2); - ArrowBufferReserve(offset_buffer, (len + 1) * sizeof(int32_t)); + result = ArrowBufferReserve(offset_buffer, (len + 1) * sizeof(int32_t)); + if (result != NANOARROW_OK) { + Rf_error("ArrowBufferReserve() failed"); + } int64_t null_count = 0; int32_t cumulative_len = 0; @@ -338,7 +344,7 @@ static void as_array_chr(SEXP x_sexp, struct ArrowArray* array, SEXP schema_xptr if (result != NANOARROW_OK) { Rf_error("ArrowBufferAppend() failed"); } - cumulative_len += item_size; + cumulative_len += (int32_t)item_size; vmaxset(vmax); } @@ -361,7 +367,7 @@ static void as_array_chr(SEXP x_sexp, struct ArrowArray* array, SEXP schema_xptr for (int64_t i = 0; i < len; i++) { uint8_t is_valid = STRING_ELT(x_sexp, i) != NA_STRING; - ArrowBitmapAppend(&bitmap, is_valid, 1); + ArrowBitmapAppendUnsafe(&bitmap, is_valid, 1); } ArrowArraySetValidityBitmap(array, &bitmap); @@ -470,7 +476,7 @@ static void as_array_list(SEXP x_sexp, struct ArrowArray* array, SEXP schema_xpt Rf_error("ArrowBufferAppend() failed"); } - cumulative_len += item_size; + cumulative_len += (int32_t)item_size; ArrowBufferAppendUnsafe(offset_buffer, &cumulative_len, sizeof(int32_t)); } @@ -489,7 +495,7 @@ static void as_array_list(SEXP x_sexp, struct ArrowArray* array, SEXP schema_xpt for (int64_t i = 0; i < len; i++) { uint8_t is_valid = VECTOR_ELT(x_sexp, i) != R_NilValue; - ArrowBitmapAppend(&bitmap, is_valid, 1); + ArrowBitmapAppendUnsafe(&bitmap, is_valid, 1); } ArrowArraySetValidityBitmap(array, &bitmap); diff --git a/r/src/buffer.c b/r/src/buffer.c index 96f8fa2d0..55e522288 100644 --- a/r/src/buffer.c +++ b/r/src/buffer.c @@ -64,7 +64,10 @@ SEXP nanoarrow_c_as_buffer_default(SEXP x_sexp) { if (x_sexp != NA_STRING) { data = CHAR(x_sexp); break; + } else { + Rf_error("NA_character_ not supported in as_nanoarrow_buffer()"); } + break; default: Rf_error("Unsupported type"); } @@ -178,8 +181,8 @@ SEXP nanoarrow_c_buffer_info(SEXP buffer_xptr) { ""}; SEXP info = PROTECT(Rf_mkNamed(VECSXP, names)); SET_VECTOR_ELT(info, 0, R_MakeExternalPtr(buffer->data, NULL, buffer_xptr)); - SET_VECTOR_ELT(info, 1, Rf_ScalarReal(buffer->size_bytes)); - SET_VECTOR_ELT(info, 2, Rf_ScalarReal(buffer->capacity_bytes)); + SET_VECTOR_ELT(info, 1, Rf_ScalarReal((double)buffer->size_bytes)); + SET_VECTOR_ELT(info, 2, Rf_ScalarReal((double)buffer->capacity_bytes)); SET_VECTOR_ELT(info, 3, buffer_type_sexp); SET_VECTOR_ELT(info, 4, buffer_data_type_sexp); SET_VECTOR_ELT(info, 5, Rf_ScalarInteger(element_size_bits)); @@ -189,7 +192,7 @@ SEXP nanoarrow_c_buffer_info(SEXP buffer_xptr) { SEXP nanoarrow_c_buffer_head_bytes(SEXP buffer_xptr, SEXP max_bytes_sexp) { struct ArrowBuffer* buffer = buffer_from_xptr(buffer_xptr); - int64_t max_bytes = REAL(max_bytes_sexp)[0]; + int64_t max_bytes = (int64_t)REAL(max_bytes_sexp)[0]; if (buffer->size_bytes <= max_bytes) { return buffer_xptr; diff --git a/r/src/buffer.h b/r/src/buffer.h index ba60a4106..2dcc49eb8 100644 --- a/r/src/buffer.h +++ b/r/src/buffer.h @@ -72,11 +72,11 @@ static inline SEXP buffer_borrowed_xptr(const void* addr, int64_t size_bytes, static inline void buffer_borrowed_xptr_set_type(SEXP buffer_xptr, enum ArrowBufferType buffer_type, enum ArrowType buffer_data_type, - int32_t element_size_bits) { + int64_t element_size_bits) { SEXP buffer_types_sexp = PROTECT(Rf_allocVector(INTSXP, 3)); INTEGER(buffer_types_sexp)[0] = buffer_type; INTEGER(buffer_types_sexp)[1] = buffer_data_type; - INTEGER(buffer_types_sexp)[2] = element_size_bits; + INTEGER(buffer_types_sexp)[2] = (int32_t)element_size_bits; R_SetExternalPtrTag(buffer_xptr, buffer_types_sexp); UNPROTECT(1); } diff --git a/r/src/convert_array_stream.c b/r/src/convert_array_stream.c index 55d68a2c0..b5315f388 100644 --- a/r/src/convert_array_stream.c +++ b/r/src/convert_array_stream.c @@ -30,8 +30,15 @@ SEXP nanoarrow_c_convert_array_stream(SEXP array_stream_xptr, SEXP ptype_sexp, SEXP size_sexp, SEXP n_sexp) { struct ArrowArrayStream* array_stream = nanoarrow_array_stream_from_xptr(array_stream_xptr); - double size = REAL(size_sexp)[0]; - double n = REAL(n_sexp)[0]; + int64_t size = (int64_t)(REAL(size_sexp)[0]); + + double n_real = REAL(n_sexp)[0]; + int n; + if (R_FINITE(n_real)) { + n = (int)n_real; + } else { + n = INT_MAX; + } SEXP schema_xptr = PROTECT(nanoarrow_schema_owning_xptr()); struct ArrowSchema* schema = nanoarrow_output_schema_from_xptr(schema_xptr); diff --git a/r/src/materialize.c b/r/src/materialize.c index f1c89dbec..c2bca993b 100644 --- a/r/src/materialize.c +++ b/r/src/materialize.c @@ -89,11 +89,11 @@ void nanoarrow_set_rownames(SEXP x, R_xlen_t len) { if (len <= INT_MAX) { SEXP rownames = PROTECT(Rf_allocVector(INTSXP, 2)); INTEGER(rownames)[0] = NA_INTEGER; - INTEGER(rownames)[1] = -len; + INTEGER(rownames)[1] = (int)(-len); Rf_setAttrib(x, R_RowNamesSymbol, rownames); UNPROTECT(1); } else { - SEXP length_dbl = PROTECT(Rf_ScalarReal(len)); + SEXP length_dbl = PROTECT(Rf_ScalarReal((double)len)); SEXP seq_len_symbol = PROTECT(Rf_install("seq_len")); SEXP seq_len_call = PROTECT(Rf_lang2(seq_len_symbol, length_dbl)); SEXP rownames_call = PROTECT(Rf_lang2(R_AsCharacterSymbol, seq_len_call)); @@ -294,9 +294,9 @@ static int nanoarrow_materialize_other(struct RConverter* converter, (struct ArrowArray*)converter->array_view.array, schema_xptr, converter_xptr)); Rf_setAttrib(array_xptr, R_ClassSymbol, nanoarrow_cls_array); - SEXP offset_sexp = - PROTECT(Rf_ScalarReal(converter->src.array_view->offset + converter->src.offset)); - SEXP length_sexp = PROTECT(Rf_ScalarReal(converter->src.length)); + SEXP offset_sexp = PROTECT( + Rf_ScalarReal((double)(converter->src.array_view->offset + converter->src.offset))); + SEXP length_sexp = PROTECT(Rf_ScalarReal((double)converter->src.length)); SEXP fun = PROTECT(Rf_install("convert_fallback_other")); SEXP call = PROTECT( diff --git a/r/src/materialize_chr.h b/r/src/materialize_chr.h index dfe9823af..2db422b54 100644 --- a/r/src/materialize_chr.h +++ b/r/src/materialize_chr.h @@ -79,7 +79,7 @@ static inline int nanoarrow_materialize_chr(struct RConverter* converter) { } else { item = ArrowArrayViewGetStringUnsafe(src->array_view, src->offset + i); SET_STRING_ELT(dst->vec_sexp, dst->offset + i, - Rf_mkCharLenCE(item.data, item.size_bytes, CE_UTF8)); + Rf_mkCharLenCE(item.data, (int)item.size_bytes, CE_UTF8)); } } diff --git a/r/src/materialize_common.h b/r/src/materialize_common.h index 4f5c52f88..6c811b615 100644 --- a/r/src/materialize_common.h +++ b/r/src/materialize_common.h @@ -109,7 +109,7 @@ struct RConverter { static inline void warn_lossy_conversion(int64_t count, const char* msg) { SEXP fun = PROTECT(Rf_install("warn_lossy_conversion")); - SEXP count_sexp = PROTECT(Rf_ScalarReal(count)); + SEXP count_sexp = PROTECT(Rf_ScalarReal((double)count)); SEXP msg_sexp = PROTECT(Rf_mkString(msg)); SEXP call = PROTECT(Rf_lang3(fun, count_sexp, msg_sexp)); Rf_eval(call, nanoarrow_ns_pkg); diff --git a/r/src/materialize_int.h b/r/src/materialize_int.h index 57dff58f6..884d99d76 100644 --- a/r/src/materialize_int.h +++ b/r/src/materialize_int.h @@ -80,7 +80,7 @@ static inline int nanoarrow_materialize_int(struct ArrayViewSlice* src, // No need to bounds check for these types for (R_xlen_t i = 0; i < dst->length; i++) { result[dst->offset + i] = - ArrowArrayViewGetIntUnsafe(src->array_view, src->offset + i); + (int32_t)ArrowArrayViewGetIntUnsafe(src->array_view, src->offset + i); } // Set any nulls to NA_INTEGER @@ -107,7 +107,7 @@ static inline int nanoarrow_materialize_int(struct ArrayViewSlice* src, result[dst->offset + i] = NA_INTEGER; n_bad_values++; } else { - result[dst->offset + i] = value; + result[dst->offset + i] = (int32_t)value; } } else { result[dst->offset + i] = NA_INTEGER; @@ -120,7 +120,7 @@ static inline int nanoarrow_materialize_int(struct ArrayViewSlice* src, result[dst->offset + i] = NA_INTEGER; n_bad_values++; } else { - result[dst->offset + i] = value; + result[dst->offset + i] = (int32_t)value; } } } diff --git a/r/src/pointers.c b/r/src/pointers.c index 4b04452bb..7e74b329e 100644 --- a/r/src/pointers.c +++ b/r/src/pointers.c @@ -38,7 +38,9 @@ SEXP nanoarrow_c_pointer(SEXP obj_sexp) { if (TYPEOF(obj_sexp) == EXTPTRSXP) { return obj_sexp; } else if (TYPEOF(obj_sexp) == REALSXP && Rf_length(obj_sexp) == 1) { - intptr_t ptr_int = REAL(obj_sexp)[0]; + // Note that this is not a good idea to actually do; however, is provided for + // backward compatibility with early versions of the arrow R package. + intptr_t ptr_int = (intptr_t)(REAL(obj_sexp)[0]); return R_MakeExternalPtr((void*)ptr_int, R_NilValue, R_NilValue); } else if (TYPEOF(obj_sexp) == STRSXP && Rf_length(obj_sexp) == 1) { const char* text = CHAR(STRING_ELT(obj_sexp, 0)); @@ -56,8 +58,10 @@ SEXP nanoarrow_c_pointer(SEXP obj_sexp) { } SEXP nanoarrow_c_pointer_addr_dbl(SEXP ptr) { + // Note that this is not a good idea to actually do; however, is provided for + // backward compatibility with early versions of the arrow R package. uintptr_t ptr_int = (uintptr_t)R_ExternalPtrAddr(nanoarrow_c_pointer(ptr)); - return Rf_ScalarReal(ptr_int); + return Rf_ScalarReal((double)ptr_int); } SEXP nanoarrow_c_pointer_addr_chr(SEXP ptr) { diff --git a/r/src/schema.c b/r/src/schema.c index fb00435d8..a160d8fed 100644 --- a/r/src/schema.c +++ b/r/src/schema.c @@ -141,7 +141,11 @@ static SEXP schema_metadata_to_list(const char* metadata) { } struct ArrowMetadataReader reader; - ArrowMetadataReaderInit(&reader, metadata); + int result = ArrowMetadataReaderInit(&reader, metadata); + if (result != NANOARROW_OK) { + Rf_error("ArrowMetadataReaderInit() failed"); + } + SEXP names = PROTECT(Rf_allocVector(STRSXP, reader.remaining_keys)); SEXP values = PROTECT(Rf_allocVector(VECSXP, reader.remaining_keys)); @@ -149,8 +153,12 @@ static SEXP schema_metadata_to_list(const char* metadata) { struct ArrowStringView value; R_xlen_t i = 0; while (reader.remaining_keys > 0) { - ArrowMetadataReaderRead(&reader, &key, &value); - SET_STRING_ELT(names, i, Rf_mkCharLenCE(key.data, key.size_bytes, CE_UTF8)); + result = ArrowMetadataReaderRead(&reader, &key, &value); + if (result != NANOARROW_OK) { + Rf_error("ArrowMetadataReaderRead() failed"); + } + + SET_STRING_ELT(names, i, Rf_mkCharLenCE(key.data, (int)key.size_bytes, CE_UTF8)); SEXP value_raw = PROTECT(Rf_allocVector(RAWSXP, value.size_bytes)); memcpy(RAW(value_raw), value.data, value.size_bytes); SET_VECTOR_ELT(values, i, value_raw); @@ -197,7 +205,7 @@ SEXP nanoarrow_c_schema_to_list(SEXP schema_xptr) { } SET_VECTOR_ELT(result, 2, schema_metadata_to_list(schema->metadata)); - SET_VECTOR_ELT(result, 3, Rf_ScalarInteger(schema->flags)); + SET_VECTOR_ELT(result, 3, Rf_ScalarInteger((int)schema->flags)); if (schema->n_children > 0) { SEXP children_sexp = PROTECT(Rf_allocVector(VECSXP, schema->n_children)); @@ -237,7 +245,7 @@ static SEXP mkStringView(struct ArrowStringView* view) { return R_NilValue; } - SEXP chr = PROTECT(Rf_mkCharLenCE(view->data, view->size_bytes, CE_UTF8)); + SEXP chr = PROTECT(Rf_mkCharLenCE(view->data, (int)view->size_bytes, CE_UTF8)); SEXP str = PROTECT(Rf_allocVector(STRSXP, 1)); SET_STRING_ELT(str, 0, chr); UNPROTECT(2); @@ -303,7 +311,7 @@ SEXP nanoarrow_c_schema_parse(SEXP schema_xptr) { schema_view.type == NANOARROW_TYPE_SPARSE_UNION) { int8_t type_ids[128]; int num_type_ids = _ArrowParseUnionTypeIds(schema_view.union_type_ids, type_ids); - if (num_type_ids == -1) { + if (num_type_ids == -1 || num_type_ids > 127) { Rf_error("Invalid type IDs in union type: '%s'", schema_view.union_type_ids); } @@ -333,13 +341,18 @@ SEXP nanoarrow_c_schema_format(SEXP schema_xptr, SEXP recursive_sexp) { struct ArrowSchema* schema = (struct ArrowSchema*)R_ExternalPtrAddr(schema_xptr); - int64_t size_needed = ArrowSchemaToString(schema, NULL, 0, recursive); + int64_t size_needed = ArrowSchemaToString(schema, NULL, 0, recursive != 0); + if (size_needed >= INT_MAX) { + size_needed = INT_MAX - 1; + } + // Using an SEXP because Rf_mkCharLenCE could jump SEXP formatted_sexp = PROTECT(Rf_allocVector(RAWSXP, size_needed + 1)); - ArrowSchemaToString(schema, (char*)RAW(formatted_sexp), size_needed + 1, recursive); + ArrowSchemaToString(schema, (char*)RAW(formatted_sexp), size_needed + 1, + recursive != 0); SEXP result_sexp = PROTECT(Rf_allocVector(STRSXP, 1)); SET_STRING_ELT(result_sexp, 0, - Rf_mkCharLenCE((char*)RAW(formatted_sexp), size_needed, CE_UTF8)); + Rf_mkCharLenCE((char*)RAW(formatted_sexp), (int)size_needed, CE_UTF8)); UNPROTECT(2); return result_sexp; } diff --git a/r/src/util.c b/r/src/util.c index e61588a0d..a278f1876 100644 --- a/r/src/util.c +++ b/r/src/util.c @@ -54,11 +54,11 @@ void nanoarrow_init_cached_sexps(void) { } SEXP nanoarrow_c_preserved_count(void) { - return Rf_ScalarReal(nanoarrow_preserved_count()); + return Rf_ScalarReal((double)nanoarrow_preserved_count()); } SEXP nanoarrow_c_preserved_empty(void) { - return Rf_ScalarReal(nanoarrow_preserved_empty()); + return Rf_ScalarReal((double)nanoarrow_preserved_empty()); } SEXP nanoarrow_c_preserve_and_release_on_other_thread(SEXP obj) { diff --git a/r/src/util.h b/r/src/util.h index 28b1bde0d..86e23c232 100644 --- a/r/src/util.h +++ b/r/src/util.h @@ -60,9 +60,9 @@ static inline void check_trivial_alloc(const void* ptr, const char* ptr_type) { // possible. static inline SEXP length_sexp_from_int64(int64_t value) { if (value < INT_MAX) { - return Rf_ScalarInteger(value); + return Rf_ScalarInteger((int)value); } else { - return Rf_ScalarReal(value); + return Rf_ScalarReal((double)value); } } diff --git a/r/tests/testthat/test-buffer.R b/r/tests/testthat/test-buffer.R index c7ce8434c..031218b77 100644 --- a/r/tests/testthat/test-buffer.R +++ b/r/tests/testthat/test-buffer.R @@ -62,6 +62,11 @@ test_that("buffers can be printed", { }) test_that("as_nanoarrow_buffer() errors for unsupported types", { + expect_error( + as_nanoarrow_buffer(NA_character_), + "NA_character_ not supported" + ) + expect_error( as_nanoarrow_buffer(environment()), "Can't convert object of type environment to nanoarrow_buffer" diff --git a/src/nanoarrow/array.c b/src/nanoarrow/array.c index 65e60878c..3f24ccbc9 100644 --- a/src/nanoarrow/array.c +++ b/src/nanoarrow/array.c @@ -415,7 +415,7 @@ static ArrowErrorCode ArrowArrayFinalizeBuffers(struct ArrowArray* array) { case NANOARROW_TYPE_LARGE_BINARY: case NANOARROW_TYPE_LARGE_STRING: if (ArrowArrayBuffer(array, 2)->data == NULL) { - ArrowBufferAppendUInt8(ArrowArrayBuffer(array, 2), 0); + NANOARROW_RETURN_NOT_OK(ArrowBufferAppendUInt8(ArrowArrayBuffer(array, 2), 0)); } break; default: @@ -585,8 +585,8 @@ ArrowErrorCode ArrowArrayViewInitFromSchema(struct ArrowArrayView* array_view, } memset(array_view->union_type_id_map, -1, 256); - int8_t n_type_ids = _ArrowParseUnionTypeIds(schema_view.union_type_ids, - array_view->union_type_id_map + 128); + int32_t n_type_ids = _ArrowParseUnionTypeIds(schema_view.union_type_ids, + array_view->union_type_id_map + 128); for (int8_t child_index = 0; child_index < n_type_ids; child_index++) { int8_t type_id = array_view->union_type_id_map[128 + child_index]; array_view->union_type_id_map[type_id] = child_index; @@ -1151,7 +1151,7 @@ static int ArrowArrayViewValidateFull(struct ArrowArrayView* array_view, error, "[%ld] Expected union offset for child id %d to be between 0 and %ld but " "found offset value %ld", - (long)i, (int)child_id, (long)child_length, offset); + (long)i, (int)child_id, (long)child_length, (long)offset); return EINVAL; } } diff --git a/src/nanoarrow/array_inline.h b/src/nanoarrow/array_inline.h index 1a844c070..10cfbbe0d 100644 --- a/src/nanoarrow/array_inline.h +++ b/src/nanoarrow/array_inline.h @@ -61,7 +61,7 @@ static inline int8_t _ArrowArrayUnionTypeId(struct ArrowArray* array, return child_index; } -static inline int8_t _ArrowParseUnionTypeIds(const char* type_ids, int8_t* out) { +static inline int32_t _ArrowParseUnionTypeIds(const char* type_ids, int8_t* out) { if (*type_ids == '\0') { return 0; } @@ -113,7 +113,7 @@ static inline int8_t _ArrowParsedUnionTypeIdsWillEqualChildIndices(const int8_t* static inline int8_t _ArrowUnionTypeIdsWillEqualChildIndices(const char* type_id_str, int64_t n_children) { int8_t type_ids[128]; - int8_t n_type_ids = _ArrowParseUnionTypeIds(type_id_str, type_ids); + int32_t n_type_ids = _ArrowParseUnionTypeIds(type_id_str, type_ids); return _ArrowParsedUnionTypeIdsWillEqualChildIndices(type_ids, n_type_ids, n_children); } diff --git a/src/nanoarrow/array_test.cc b/src/nanoarrow/array_test.cc index 08b3b72f8..01016084e 100644 --- a/src/nanoarrow/array_test.cc +++ b/src/nanoarrow/array_test.cc @@ -140,7 +140,7 @@ TEST(ArrayTest, ArrayTestInitFromSchema) { TEST(ArrayTest, ArrayTestSetBitmap) { struct ArrowBitmap bitmap; ArrowBitmapInit(&bitmap); - ArrowBitmapAppend(&bitmap, true, 9); + ASSERT_EQ(ArrowBitmapAppend(&bitmap, true, 9), NANOARROW_OK); struct ArrowArray array; ASSERT_EQ(ArrowArrayInitFromType(&array, NANOARROW_TYPE_INT32), NANOARROW_OK); @@ -162,11 +162,11 @@ TEST(ArrayTest, ArrayTestSetBuffer) { struct ArrowBuffer buffer0, buffer1, buffer2; ArrowBufferInit(&buffer0); - ArrowBufferAppend(&buffer0, validity_bitmap, 1); + ASSERT_EQ(ArrowBufferAppend(&buffer0, validity_bitmap, 1), NANOARROW_OK); ArrowBufferInit(&buffer1); - ArrowBufferAppend(&buffer1, offsets, 9 * sizeof(int32_t)); + ASSERT_EQ(ArrowBufferAppend(&buffer1, offsets, 9 * sizeof(int32_t)), NANOARROW_OK); ArrowBufferInit(&buffer2); - ArrowBufferAppend(&buffer2, data, strlen(data)); + ASSERT_EQ(ArrowBufferAppend(&buffer2, data, strlen(data)), NANOARROW_OK); struct ArrowArray array; ASSERT_EQ(ArrowArrayInitFromType(&array, NANOARROW_TYPE_STRING), NANOARROW_OK); @@ -1558,7 +1558,7 @@ TEST(ArrayTest, ArrayViewTestBasic) { struct ArrowArray array; // Build with no validity buffer - ArrowArrayInitFromType(&array, NANOARROW_TYPE_INT32); + ASSERT_EQ(ArrowArrayInitFromType(&array, NANOARROW_TYPE_INT32), NANOARROW_OK); ASSERT_EQ(ArrowBufferAppendInt32(ArrowArrayBuffer(&array, 1), 11), NANOARROW_OK); ASSERT_EQ(ArrowBufferAppendInt32(ArrowArrayBuffer(&array, 1), 12), NANOARROW_OK); ASSERT_EQ(ArrowBufferAppendInt32(ArrowArrayBuffer(&array, 1), 13), NANOARROW_OK); diff --git a/src/nanoarrow/buffer_inline.h b/src/nanoarrow/buffer_inline.h index 190943f0a..e27ff67c6 100644 --- a/src/nanoarrow/buffer_inline.h +++ b/src/nanoarrow/buffer_inline.h @@ -245,17 +245,17 @@ static inline void _ArrowBitsUnpackInt32(const uint8_t word, int32_t* out) { } static inline void _ArrowBitmapPackInt8(const int8_t* values, uint8_t* out) { - *out = (values[0] | ((values[1] + 0x1) & 0x2) | ((values[2] + 0x3) & 0x4) | - ((values[3] + 0x7) & 0x8) | ((values[4] + 0xf) & 0x10) | - ((values[5] + 0x1f) & 0x20) | ((values[6] + 0x3f) & 0x40) | - ((values[7] + 0x7f) & 0x80)); + *out = (uint8_t)(values[0] | ((values[1] + 0x1) & 0x2) | ((values[2] + 0x3) & 0x4) | + ((values[3] + 0x7) & 0x8) | ((values[4] + 0xf) & 0x10) | + ((values[5] + 0x1f) & 0x20) | ((values[6] + 0x3f) & 0x40) | + ((values[7] + 0x7f) & 0x80)); } static inline void _ArrowBitmapPackInt32(const int32_t* values, uint8_t* out) { - *out = (values[0] | ((values[1] + 0x1) & 0x2) | ((values[2] + 0x3) & 0x4) | - ((values[3] + 0x7) & 0x8) | ((values[4] + 0xf) & 0x10) | - ((values[5] + 0x1f) & 0x20) | ((values[6] + 0x3f) & 0x40) | - ((values[7] + 0x7f) & 0x80)); + *out = (uint8_t)(values[0] | ((values[1] + 0x1) & 0x2) | ((values[2] + 0x3) & 0x4) | + ((values[3] + 0x7) & 0x8) | ((values[4] + 0xf) & 0x10) | + ((values[5] + 0x1f) & 0x20) | ((values[6] + 0x3f) & 0x40) | + ((values[7] + 0x7f) & 0x80)); } static inline int8_t ArrowBitGet(const uint8_t* bits, int64_t i) { @@ -295,7 +295,7 @@ static inline void ArrowBitsUnpackInt8(const uint8_t* bits, int64_t start_offset } // last byte - const int bits_remaining = i_end % 8 == 0 ? 8 : i_end % 8; + const int bits_remaining = (int)(i_end % 8 == 0 ? 8 : i_end % 8); for (int i = 0; i < bits_remaining; i++) { *out++ = ArrowBitGet(&bits[bytes_last_valid], i); } @@ -334,7 +334,7 @@ static inline void ArrowBitsUnpackInt32(const uint8_t* bits, int64_t start_offse } // last byte - const int bits_remaining = i_end % 8 == 0 ? 8 : i_end % 8; + const int bits_remaining = (int)(i_end % 8 == 0 ? 8 : i_end % 8); for (int i = 0; i < bits_remaining; i++) { *out++ = ArrowBitGet(&bits[bytes_last_valid], i); } @@ -555,7 +555,7 @@ static inline void ArrowBitmapAppendInt32Unsafe(struct ArrowBitmap* bitmap, if ((out_i_cursor % 8) != 0) { int64_t n_partial_bits = _ArrowRoundUpToMultipleOf8(out_i_cursor) - out_i_cursor; for (int i = 0; i < n_partial_bits; i++) { - ArrowBitSetTo(bitmap->buffer.data, out_i_cursor++, values[i]); + ArrowBitSetTo(bitmap->buffer.data, out_i_cursor++, (uint8_t)values[i]); } out_cursor++; @@ -578,7 +578,7 @@ static inline void ArrowBitmapAppendInt32Unsafe(struct ArrowBitmap* bitmap, // Zero out the last byte *out_cursor = 0x00; for (int i = 0; i < n_remaining; i++) { - ArrowBitSetTo(bitmap->buffer.data, out_i_cursor++, values_cursor[i]); + ArrowBitSetTo(bitmap->buffer.data, out_i_cursor++, (uint8_t)values_cursor[i]); } out_cursor++; } diff --git a/src/nanoarrow/buffer_test.cc b/src/nanoarrow/buffer_test.cc index 141911fa5..4cf196b5d 100644 --- a/src/nanoarrow/buffer_test.cc +++ b/src/nanoarrow/buffer_test.cc @@ -512,7 +512,7 @@ TEST(BitmapTest, BitmapTestResize) { // Check normal usage, which is resize to the final length // after appending a bunch of values - ArrowBitmapResize(&bitmap, 200, false); + ASSERT_EQ(ArrowBitmapResize(&bitmap, 200, false), NANOARROW_OK); EXPECT_EQ(bitmap.buffer.size_bytes, 0); EXPECT_EQ(bitmap.buffer.capacity_bytes, 200 / 8); EXPECT_EQ(bitmap.size_bits, 0); diff --git a/src/nanoarrow/nanoarrow.h b/src/nanoarrow/nanoarrow.h index bf4d66f3a..f79928b3e 100644 --- a/src/nanoarrow/nanoarrow.h +++ b/src/nanoarrow/nanoarrow.h @@ -259,7 +259,8 @@ static inline void ArrowArrayStreamRelease(struct ArrowArrayStream* array_stream /// \brief Set the contents of an error using printf syntax. /// /// If error is NULL, this function does nothing and returns NANOARROW_OK. -ArrowErrorCode ArrowErrorSet(struct ArrowError* error, const char* fmt, ...); +NANOARROW_CHECK_PRINTF_ATTRIBUTE int ArrowErrorSet(struct ArrowError* error, + const char* fmt, ...); /// @} @@ -1131,6 +1132,12 @@ ArrowErrorCode ArrowBasicArrayStreamValidate(const struct ArrowArrayStream* arra /// @} +// Undefine ArrowErrorCode, which may have been defined to annotate functions that return +// it to warn for an unused result. +#if defined(ArrowErrorCode) +#undef ArrowErrorCode +#endif + // Inline function definitions #include "array_inline.h" #include "buffer_inline.h" diff --git a/src/nanoarrow/nanoarrow_hpp_test.cc b/src/nanoarrow/nanoarrow_hpp_test.cc index 068f2ee4e..373c182f6 100644 --- a/src/nanoarrow/nanoarrow_hpp_test.cc +++ b/src/nanoarrow/nanoarrow_hpp_test.cc @@ -33,7 +33,7 @@ TEST(NanoarrowHppTest, NanoarrowHppUniqueArrayTest) { nanoarrow::UniqueArray array; EXPECT_EQ(array->release, nullptr); - ArrowArrayInitFromType(array.get(), NANOARROW_TYPE_INT32); + ASSERT_EQ(ArrowArrayInitFromType(array.get(), NANOARROW_TYPE_INT32), NANOARROW_OK); ASSERT_EQ(ArrowArrayStartAppending(array.get()), NANOARROW_OK); ASSERT_EQ(ArrowArrayAppendInt(array.get(), 123), NANOARROW_OK); ASSERT_EQ(ArrowArrayFinishBuildingDefault(array.get(), nullptr), NANOARROW_OK); @@ -58,7 +58,7 @@ TEST(NanoarrowHppTest, NanoarrowHppUniqueSchemaTest) { nanoarrow::UniqueSchema schema; EXPECT_EQ(schema->release, nullptr); - ArrowSchemaInitFromType(schema.get(), NANOARROW_TYPE_INT32); + ASSERT_EQ(ArrowSchemaInitFromType(schema.get(), NANOARROW_TYPE_INT32), NANOARROW_OK); EXPECT_NE(schema->release, nullptr); EXPECT_STREQ(schema->format, "i"); @@ -174,7 +174,7 @@ TEST(NanoarrowHppTest, NanoarrowHppUniqueArrayViewTest) { // Use an ArrayView with children, since an ArrayView with no children // doesn't hold any resources ArrowArrayViewInitFromType(array_view.get(), NANOARROW_TYPE_STRUCT); - ArrowArrayViewAllocateChildren(array_view.get(), 2); + ASSERT_EQ(ArrowArrayViewAllocateChildren(array_view.get(), 2), NANOARROW_OK); EXPECT_EQ(array_view->storage_type, NANOARROW_TYPE_STRUCT); // move constructor diff --git a/src/nanoarrow/nanoarrow_types.h b/src/nanoarrow/nanoarrow_types.h index 2ad6b7d64..0d5085f29 100644 --- a/src/nanoarrow/nanoarrow_types.h +++ b/src/nanoarrow/nanoarrow_types.h @@ -169,6 +169,32 @@ struct ArrowArrayStream { } while (0) #endif +#if defined(NANOARROW_DEBUG) +// For checking ArrowErrorSet() calls for valid printf format strings/arguments +// If using mingw's c99-compliant printf, we need a different format-checking attribute +#if defined(__USE_MINGW_ANSI_STDIO) && defined(__MINGW_PRINTF_FORMAT) +#define NANOARROW_CHECK_PRINTF_ATTRIBUTE \ + __attribute__((format(__MINGW_PRINTF_FORMAT, 2, 3))) +#elif defined(__GNUC__) +#define NANOARROW_CHECK_PRINTF_ATTRIBUTE __attribute__((format(printf, 2, 3))) +#else +#define NANOARROW_CHECK_PRINTF_ATTRIBUTE +#endif + +// For checking calls to functions that return ArrowErrorCode +#if defined(__GNUC__) && (__GNUC__ >= 4) +#define NANOARROW_CHECK_RETURN_ATTRIBUTE __attribute__((warn_unused_result)) +#elif defined(_MSC_VER) && (_MSC_VER >= 1700) +#define NANOARROW_CHECK_RETURN_ATTRIBUTE _Check_return_ +#else +#define NANOARROW_CHECK_RETURN_ATTRIBUTE +#endif + +#else +#define NANOARROW_CHECK_RETURN_ATTRIBUTE +#define NANOARROW_CHECK_PRINTF_ATTRIBUTE +#endif + /// \brief Return code for success. /// \ingroup nanoarrow-errors #define NANOARROW_OK 0 @@ -177,6 +203,10 @@ struct ArrowArrayStream { /// \ingroup nanoarrow-errors typedef int ArrowErrorCode; +#if defined(NANOARROW_DEBUG) +#define ArrowErrorCode NANOARROW_CHECK_RETURN_ATTRIBUTE ArrowErrorCode +#endif + /// \brief Error type containing a UTF-8 encoded message. /// \ingroup nanoarrow-errors struct ArrowError { diff --git a/src/nanoarrow/schema.c b/src/nanoarrow/schema.c index 7c82401c8..9ff1ac734 100644 --- a/src/nanoarrow/schema.c +++ b/src/nanoarrow/schema.c @@ -612,8 +612,7 @@ static ArrowErrorCode ArrowSchemaViewParse(struct ArrowSchemaView* schema_view, // decimal case 'd': if (format[1] != ':' || format[2] == '\0') { - ArrowErrorSet(error, "Expected ':precision,scale[,bitwidth]' following 'd'", - format + 3); + ArrowErrorSet(error, "Expected ':precision,scale[,bitwidth]' following 'd'"); return EINVAL; } @@ -958,13 +957,15 @@ static ArrowErrorCode ArrowSchemaViewValidateNChildren( for (int64_t i = 0; i < schema_view->schema->n_children; i++) { child = schema_view->schema->children[i]; if (child == NULL) { - ArrowErrorSet(error, "Expected valid schema at schema->children[%d] but found NULL", - i); + ArrowErrorSet(error, + "Expected valid schema at schema->children[%ld] but found NULL", + (long)i); return EINVAL; } else if (child->release == NULL) { ArrowErrorSet( error, - "Expected valid schema at schema->children[%d] but found a released schema", i); + "Expected valid schema at schema->children[%ld] but found a released schema", + (long)i); return EINVAL; } } @@ -1132,8 +1133,7 @@ ArrowErrorCode ArrowSchemaViewInit(struct ArrowSchemaView* schema_view, } const char* format_end_out; - ArrowErrorCode result = - ArrowSchemaViewParse(schema_view, format, &format_end_out, error); + int result = ArrowSchemaViewParse(schema_view, format, &format_end_out, error); if (result != NANOARROW_OK) { if (error != NULL) { @@ -1176,10 +1176,12 @@ ArrowErrorCode ArrowSchemaViewInit(struct ArrowSchemaView* schema_view, schema_view->extension_name = ArrowCharView(NULL); schema_view->extension_metadata = ArrowCharView(NULL); - ArrowMetadataGetValue(schema->metadata, ArrowCharView("ARROW:extension:name"), - &schema_view->extension_name); - ArrowMetadataGetValue(schema->metadata, ArrowCharView("ARROW:extension:metadata"), - &schema_view->extension_metadata); + NANOARROW_RETURN_NOT_OK(ArrowMetadataGetValue(schema->metadata, + ArrowCharView("ARROW:extension:name"), + &schema_view->extension_name)); + NANOARROW_RETURN_NOT_OK(ArrowMetadataGetValue(schema->metadata, + ArrowCharView("ARROW:extension:metadata"), + &schema_view->extension_metadata)); return NANOARROW_OK; } @@ -1367,7 +1369,9 @@ int64_t ArrowMetadataSizeOf(const char* metadata) { struct ArrowMetadataReader reader; struct ArrowStringView key; struct ArrowStringView value; - ArrowMetadataReaderInit(&reader, metadata); + if (ArrowMetadataReaderInit(&reader, metadata) != NANOARROW_OK) { + return 0; + } int64_t size = sizeof(int32_t); while (ArrowMetadataReaderRead(&reader, &key, &value) == NANOARROW_OK) { @@ -1383,7 +1387,7 @@ static ArrowErrorCode ArrowMetadataGetValueInternal(const char* metadata, struct ArrowMetadataReader reader; struct ArrowStringView existing_key; struct ArrowStringView existing_value; - ArrowMetadataReaderInit(&reader, metadata); + NANOARROW_RETURN_NOT_OK(ArrowMetadataReaderInit(&reader, metadata)); while (ArrowMetadataReaderRead(&reader, &existing_key, &existing_value) == NANOARROW_OK) { @@ -1410,7 +1414,10 @@ ArrowErrorCode ArrowMetadataGetValue(const char* metadata, struct ArrowStringVie char ArrowMetadataHasKey(const char* metadata, struct ArrowStringView key) { struct ArrowStringView value = ArrowCharView(NULL); - ArrowMetadataGetValue(metadata, key, &value); + if (ArrowMetadataGetValue(metadata, key, &value) != NANOARROW_OK) { + return 0; + } + return value.data != NULL; } diff --git a/src/nanoarrow/schema_test.cc b/src/nanoarrow/schema_test.cc index 4a3dfdc8b..e909aee14 100644 --- a/src/nanoarrow/schema_test.cc +++ b/src/nanoarrow/schema_test.cc @@ -388,7 +388,7 @@ TEST(SchemaTest, SchemaInitUnion) { TEST(SchemaTest, SchemaSetFormat) { struct ArrowSchema schema; - ArrowSchemaInitFromType(&schema, NANOARROW_TYPE_UNINITIALIZED); + ASSERT_EQ(ArrowSchemaInitFromType(&schema, NANOARROW_TYPE_UNINITIALIZED), NANOARROW_OK); EXPECT_EQ(ArrowSchemaSetFormat(&schema, "i"), NANOARROW_OK); EXPECT_STREQ(schema.format, "i"); @@ -401,7 +401,7 @@ TEST(SchemaTest, SchemaSetFormat) { TEST(SchemaTest, SchemaSetName) { struct ArrowSchema schema; - ArrowSchemaInitFromType(&schema, NANOARROW_TYPE_UNINITIALIZED); + ASSERT_EQ(ArrowSchemaInitFromType(&schema, NANOARROW_TYPE_UNINITIALIZED), NANOARROW_OK); EXPECT_EQ(ArrowSchemaSetName(&schema, "a_name"), NANOARROW_OK); EXPECT_STREQ(schema.name, "a_name"); @@ -414,7 +414,7 @@ TEST(SchemaTest, SchemaSetName) { TEST(SchemaTest, SchemaSetMetadata) { struct ArrowSchema schema; - ArrowSchemaInitFromType(&schema, NANOARROW_TYPE_UNINITIALIZED); + ASSERT_EQ(ArrowSchemaInitFromType(&schema, NANOARROW_TYPE_UNINITIALIZED), NANOARROW_OK); // Encoded metadata string for "key": "value" std::string simple_metadata = SimpleMetadata(); @@ -430,7 +430,7 @@ TEST(SchemaTest, SchemaSetMetadata) { TEST(SchemaTest, SchemaAllocateDictionary) { struct ArrowSchema schema; - ArrowSchemaInitFromType(&schema, NANOARROW_TYPE_UNINITIALIZED); + ASSERT_EQ(ArrowSchemaInitFromType(&schema, NANOARROW_TYPE_UNINITIALIZED), NANOARROW_OK); EXPECT_EQ(ArrowSchemaAllocateDictionary(&schema), NANOARROW_OK); EXPECT_EQ(schema.dictionary->release, nullptr); @@ -443,7 +443,7 @@ TEST(SchemaTest, SchemaCopySimpleType) { ARROW_EXPECT_OK(ExportType(*int32(), &schema)); struct ArrowSchema schema_copy; - ArrowSchemaDeepCopy(&schema, &schema_copy); + ASSERT_EQ(ArrowSchemaDeepCopy(&schema, &schema_copy), NANOARROW_OK); ASSERT_NE(schema_copy.release, nullptr); EXPECT_STREQ(schema.format, "i"); @@ -458,7 +458,7 @@ TEST(SchemaTest, SchemaCopyNestedType) { ARROW_EXPECT_OK(ExportType(*struct_type, &schema)); struct ArrowSchema schema_copy; - ArrowSchemaDeepCopy(&schema, &schema_copy); + ASSERT_EQ(ArrowSchemaDeepCopy(&schema, &schema_copy), NANOARROW_OK); ASSERT_NE(schema_copy.release, nullptr); EXPECT_STREQ(schema_copy.format, "+s"); @@ -476,7 +476,7 @@ TEST(SchemaTest, SchemaCopyDictType) { ARROW_EXPECT_OK(ExportType(*struct_type, &schema)); struct ArrowSchema schema_copy; - ArrowSchemaDeepCopy(&schema, &schema_copy); + ASSERT_EQ(ArrowSchemaDeepCopy(&schema, &schema_copy), NANOARROW_OK); ASSERT_STREQ(schema_copy.format, "i"); ASSERT_NE(schema_copy.dictionary, nullptr); @@ -494,7 +494,7 @@ TEST(SchemaTest, SchemaCopyFlags) { ASSERT_FALSE(schema.flags & ARROW_FLAG_NULLABLE); struct ArrowSchema schema_copy; - ArrowSchemaDeepCopy(&schema, &schema_copy); + ASSERT_EQ(ArrowSchemaDeepCopy(&schema, &schema_copy), NANOARROW_OK); ASSERT_NE(schema_copy.release, nullptr); ASSERT_EQ(schema.flags, schema_copy.flags); @@ -513,7 +513,7 @@ TEST(SchemaTest, SchemaCopyMetadata) { ARROW_EXPECT_OK(ExportField(*int_field, &schema)); struct ArrowSchema schema_copy; - ArrowSchemaDeepCopy(&schema, &schema_copy); + ASSERT_EQ(ArrowSchemaDeepCopy(&schema, &schema_copy), NANOARROW_OK); ASSERT_NE(schema_copy.release, nullptr); EXPECT_STREQ(schema_copy.name, "field_name");