Correct treatment of ncells=NA for non-character columns

TileDB-Inc · Mar 14, 2024 · a2cf224 · a2cf224
1 parent 2bfd402
commit a2cf224
Show file tree

Hide file tree

Showing 2 changed files with 12 additions and 10 deletions.
diff --git a/R/Attribute.R b/R/Attribute.R
@@ -63,10 +63,13 @@ tiledb_attr <- function(name,
                         ctx = tiledb_get_context()
                         ) {
     if (missing(name)) name <- ""
-    stopifnot(`The 'type' argument for is mandatory` = !missing(type),
-              `The 'ctx' argument must be a tiledb_ctx` = is(ctx, "tiledb_ctx"),
-              `The 'name' argument must be a scalar string` = is.scalar(name, "character"),
-              `The 'filter_list' argument must be a tiledb_filter_list instance` = is(filter_list, "tiledb_filter_list"))
+    if (is.na(ncells)) ncells <- NA_integer_ 		# the specific NA for ints (as basic NA is bool)
+    stopifnot("The 'name' argument must be a scalar string" = is.scalar(name, "character"),
+              "The 'type' argument is mandatory" = !missing(type),
+              "The 'ncells' argument must be numeric or NA" = is.integer(ncells) || is.na(ncells),
+              "The 'filter_list' argument must be a tiledb_filter_list instance" =
+                  is(filter_list, "tiledb_filter_list"),
+              "The 'ctx' argument must be a tiledb_ctx" = is(ctx, "tiledb_ctx"))
     ptr <- libtiledb_attribute(ctx@ptr, name, type, filter_list@ptr, ncells, nullable)
     attr <- new("tiledb_attr", ptr = ptr)
     if (!is.null(enumeration))

diff --git a/src/libtiledb.cpp b/src/libtiledb.cpp
@@ -1394,11 +1394,12 @@ XPtr<tiledb::Attribute> libtiledb_attribute(XPtr<tiledb::Context> ctx,
                                             int ncells,
                                             bool nullable) {
     check_xptr_tag<tiledb::Context>(ctx);
+    spdl::debug(tfm::format("[libtiledb_attribute] Attr name %s type %s ncells %d nullable %s",
+                            name, type, ncells, nullable ? "true" : "false"));
     tiledb_datatype_t attr_dtype = _string_to_tiledb_datatype(type);
     if (ncells < 1 && ncells != R_NaInt) {
         Rcpp::stop("ncells must be >= 1 (or NA for variable cells)");
     }
-
     // placeholder, overwritten in all branches below
     XPtr<tiledb::Attribute> attr = XPtr<tiledb::Attribute>(static_cast<tiledb::Attribute*>(nullptr));
 
@@ -1432,11 +1433,6 @@ XPtr<tiledb::Attribute> libtiledb_attribute(XPtr<tiledb::Context> ctx,
                attr_dtype == TILEDB_STRING_ASCII ||
                attr_dtype == TILEDB_STRING_UTF8) {
         attr = make_xptr<tiledb::Attribute>(new tiledb::Attribute(*ctx.get(), name, attr_dtype));
-        uint64_t num = static_cast<uint64_t>(ncells);
-        if (ncells == R_NaInt) {
-            num = TILEDB_VAR_NUM;           // R's NA is different from TileDB's NA
-        }
-        attr->set_cell_val_num(num);
 #if TILEDB_VERSION >= TileDB_Version(2,10,0)
     } else if (attr_dtype == TILEDB_BOOL) {
         attr = make_xptr<tiledb::Attribute>(new tiledb::Attribute(*ctx.get(), name, attr_dtype));
@@ -1450,6 +1446,9 @@ XPtr<tiledb::Attribute> libtiledb_attribute(XPtr<tiledb::Context> ctx,
                    "and character (CHAR,ASCII,UTF8) attributes are supported "
                    "-- seeing %s which is not", type.c_str());
     }
+    // R's NA is different from TileDB's NA so test for NA_integer_, else cast
+    uint64_t num = (ncells == R_NaInt) ? TILEDB_VAR_NUM : static_cast<uint64_t>(ncells);
+    attr->set_cell_val_num(num);
     attr->set_filter_list(*fltrlst);
     attr->set_nullable(nullable);
     return attr;