Merge pull request #2017 from DennisHeimbigner/zarrfillv.dmh

NCZarr is outputting fill value as an array instead of a singleton.
Unidata · Jun 7, 2021 · 1ee604a · 1ee604a
2 parents 9426298 + 6f03935
commit 1ee604a
Show file tree

Hide file tree

Showing 7 changed files with 118 additions and 105 deletions.
diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md
@@ -7,6 +7,7 @@ This file contains a high-level description of this package's evolution. Release
 
 ## 4.8.1 - TBD
 
+* [Bug Fix] Store NCZarr fillvalue as a singleton instead of a 1-element array. See [Github #2017](https://github.com/Unidata/netcdf-c/issues/2017).  
 * [Bug Fixes] The netcdf-c library was incorrectly determining the scope of dimension; similar to the type scope problem. See [Github #2012](https://github.com/Unidata/netcdf-c/pull/2012) for more information.
 * [Bug Fix] Re-enable DAP2 authorization testing. See [Github #2011](https://github.com/Unidata/netcdf-c/issues/2011).  
 * [Bug Fix] Fix bug with windows version of mkstemp that causes failure to create more than 26 temp files. See [Github #1998](https://github.com/Unidata/netcdf-c/pull/1998).  

diff --git a/libnczarr/zcvt.c b/libnczarr/zcvt.c
@@ -324,8 +324,12 @@ NCZ_stringconvert(nc_type typeid, size_t len, void* data0, NCjson** jdatap)
 	/* Create a string valued json object */
 	if((stat = NCJnewstringn(NCJ_STRING,len,src,&jdata)))
 	    goto done;
-    } else { /* for all other values, create an array of values */
-	if((stat = NCJnew(NCJ_ARRAY,&jdata))) goto done;
+    } else { /* all other cases */
+	if(len == 0) {stat = NC_EINVAL; goto done;}
+	if(len > 1) {
+	    if((stat = NCJnew(NCJ_ARRAY,&jdata))) goto done;
+        } else /* return a singletone */
+   	    jdata = NULL;
 	for(i=0;i<len;i++) {
   	    char* special = NULL;
 	    double d;
@@ -371,7 +375,10 @@ NCZ_stringconvert(nc_type typeid, size_t len, void* data0, NCjson** jdatap)
 	    if(special) {nullfree(str); str = strdup(special);}
 	    jvalue->value = str;
 	    str = NULL;
-	    nclistpush(jdata->contents,jvalue);
+	    if(len == 1)
+		jdata = jvalue;
+	    else
+	        nclistpush(jdata->contents,jvalue);
 	    jvalue = NULL;
 	    src += typelen;
 	}

diff --git a/libnczarr/zsync.c b/libnczarr/zsync.c
@@ -15,7 +15,7 @@ static int ncz_jsonize_atts(NCindex* attlist, NCjson** jattrsp);
 static int load_jatts(NCZMAP* map, NC_OBJ* container, NCjson** jattrsp, NClist** atypes);
 static int zconvert(nc_type typeid, size_t typelen, void* dst, NCjson* src);
 static int computeattrinfo(const char* name, NClist* atypes, NCjson* values,
-		nc_type* typeidp, size_t* lenp, void** datap);
+		nc_type* typeidp, size_t* typelenp, size_t* lenp, void** datap);
 static int parse_group_content(NCjson* jcontent, NClist* dimdefs, NClist* varnames, NClist* subgrps);
 static int parse_group_content_pure(NCZ_FILE_INFO_T*  zinfo, NC_GRP_INFO_T* grp, NClist* varnames, NClist* subgrps);
 static int define_grp(NC_FILE_INFO_T* file, NC_GRP_INFO_T* grp);
@@ -28,7 +28,7 @@ static int locategroup(NC_FILE_INFO_T* file, size_t nsegs, NClist* segments, NC_
 static int createdim(NC_FILE_INFO_T* file, const char* name, size64_t dimlen, NC_DIM_INFO_T** dimp);
 static int parsedimrefs(NC_FILE_INFO_T*, NClist* dimnames,  size64_t* shape, NC_DIM_INFO_T** dims, int create);
 static int decodeints(NCjson* jshape, size64_t* shapes);
-static int computeattrdata(nc_type* typeidp, NCjson* values, size_t* lenp, void** datap);
+static int computeattrdata(nc_type* typeidp, NCjson* values, size_t* typelenp, size_t* lenp, void** datap);
 static int inferattrtype(NCjson* values, nc_type* typeidp);
 static int mininttype(unsigned long long u64, int negative);
 static int computedimrefs(NC_FILE_INFO_T* file, NC_VAR_INFO_T* var, int purezarr, int xarray, int ndims, NClist* dimnames, size64_t* shapes, NC_DIM_INFO_T** dims);
@@ -244,6 +244,7 @@ ncz_sync_var(NC_FILE_INFO_T* file, NC_VAR_INFO_T* var)
     NCjson* jncvar = NULL;
     NCjson* jdimrefs = NULL;
     NCjson* jtmp = NULL;
+    NCjson* jfill = NULL;
     size64_t shape[NC_MAX_VAR_DIMS];
     NCZ_VAR_INFO_T* zvar = var->format_var_info;
 
@@ -325,7 +326,6 @@ ncz_sync_var(NC_FILE_INFO_T* file, NC_VAR_INFO_T* var)
     if(!var->no_fill) {
 	int fillsort;
 	int atomictype = var->type_info->hdr.id;
-	NCjson* jfill = NULL;
 	/* A scalar value providing the default value to use for uninitialized
 	   portions of the array, or ``null`` if no fill_value is to be used. */
 	/* Use the defaults defined in netdf.h */
@@ -339,9 +339,17 @@ ncz_sync_var(NC_FILE_INFO_T* file, NC_VAR_INFO_T* var)
 	    if((stat = nc4_get_default_fill_value(atomictype,var->fill_value))) goto done;
 	}
         /* Convert var->fill_value to a string */
-	if((stat = NCZ_stringconvert(atomictype,1,var->fill_value,&jfill)))
-	    goto done;
-	if((stat = NCJinsert(jvar,"fill_value",jfill))) goto done;
+	if((stat = NCZ_stringconvert(atomictype,1,var->fill_value,&jfill))) goto done;
+	if(jfill->sort == NCJ_ARRAY) { /* stringconvert should prevent this from happening */
+	    assert(NCJlength(jfill) > 0);
+	    if((stat = NCJarrayith(jfill,0,&jtmp))) goto done; /* use the 0th element */
+	    if((stat = NCJclone(jtmp,&jtmp))) goto done; /* clone so we can free it later */
+	    NCJreclaim(jfill);
+	    jfill = jtmp;
+	    jtmp = NULL;
+        }
+        if((stat = NCJinsert(jvar,"fill_value",jfill))) goto done;
+        jfill = NULL;
     }
 
     /* order key */
@@ -463,6 +471,7 @@ ncz_sync_var(NC_FILE_INFO_T* file, NC_VAR_INFO_T* var)
     NCJreclaim(jvar);
     NCJreclaim(jncvar);
     NCJreclaim(jtmp);
+    NCJreclaim(jfill);
     return THROW(stat);
 }
 
@@ -683,6 +692,8 @@ ncz_sync_atts(NC_FILE_INFO_T* file, NC_OBJ* container, NCindex* attlist)
 /**
 @internal Convert a list of attributes to corresponding json.
 Note that this does not push to the file.
+Also note that attributes of length 1 are stored as singletons, not arrays.
+This is to be more consistent with pure zarr. 
 @param attlist - [in] the attributes to dictify
 @param jattrsp - [out] the json'ized att list
 @return NC_NOERR
@@ -858,11 +869,11 @@ Extract type and data for an attribute
 */
 static int
 computeattrinfo(const char* name, NClist* atypes, NCjson* values,
-		nc_type* typeidp, size_t* lenp, void** datap)
+		nc_type* typeidp, size_t* typelenp, size_t* lenp, void** datap)
 {
     int stat = NC_NOERR;
     int i;
-    size_t len;
+    size_t len, typelen;
     void* data;
     nc_type typeid;
 
@@ -880,10 +891,11 @@ computeattrinfo(const char* name, NClist* atypes, NCjson* values,
     }
     if(typeid >= NC_STRING)
 	{stat = NC_EINTERNAL; goto done;}
-    if((stat = computeattrdata(&typeid, values, &len, &data))) goto done;
+    if((stat = computeattrdata(&typeid, values, &typelen, &len, &data))) goto done;
 
     if(typeidp) *typeidp = typeid;
     if(lenp) *lenp = len;
+    if(typelenp) *typelenp = typelen;
     if(datap) {*datap = data; data = NULL;}
 
 done:
@@ -895,7 +907,7 @@ computeattrinfo(const char* name, NClist* atypes, NCjson* values,
 Extract data for an attribute
 */
 static int
-computeattrdata(nc_type* typeidp, NCjson* values, size_t* lenp, void** datap)
+computeattrdata(nc_type* typeidp, NCjson* values, size_t* typelenp, size_t* lenp, void** datap)
 {
     int stat = NC_NOERR;
     size_t datalen;
@@ -911,7 +923,7 @@ computeattrdata(nc_type* typeidp, NCjson* values, size_t* lenp, void** datap)
 
     /* Collect the length of the attribute; might be a singleton  */
     switch (values->sort) {
-    case NCJ_DICT: stat = NC_EINTERNAL; goto done;
+    case NCJ_DICT: stat = NC_ENCZARR; goto done;
     case NCJ_ARRAY:
 	datalen = nclistlength(values->contents);
 	break;
@@ -923,21 +935,22 @@ computeattrdata(nc_type* typeidp, NCjson* values, size_t* lenp, void** datap)
 	break;
     }
 
-    /* Allocate data space */
-    if((stat = NC4_inq_atomic_type(typeid, NULL, &typelen)))
-	goto done;
-    if(typeid == NC_CHAR)
-        data = malloc(typelen*(datalen+1));
-    else
-        data = malloc(typelen*datalen);
-    if(data == NULL)
-	{stat = NC_ENOMEM; goto done;}
-
-    /* convert to target type */	
-    if((stat = zconvert(typeid, typelen, data, values)))
-	goto done;
-
+    if(datalen > 0) {
+        /* Allocate data space */
+        if((stat = NC4_inq_atomic_type(typeid, NULL, &typelen)))
+	    goto done;
+        if(typeid == NC_CHAR)
+            data = malloc(typelen*(datalen+1));
+        else
+            data = malloc(typelen*datalen);
+        if(data == NULL)
+	    {stat = NC_ENOMEM; goto done;}
+        /* convert to target type */	
+        if((stat = zconvert(typeid, typelen, data, values)))
+   	    goto done;
+    }
     if(lenp) *lenp = datalen;
+    if(typelenp) *typelenp = typelen;
     if(datap) {*datap = data; data = NULL;}
     if(typeidp) *typeidp = typeid; /* return possibly inferred type */
 
@@ -982,7 +995,7 @@ inferattrtype(NCjson* value, nc_type* typeidp)
 	typeid = NC_CHAR;
 	break;
     default:
-	return NC_EINTERNAL;
+	return NC_ENCZARR;
     }
     if(typeidp) *typeidp = typeid;
     return NC_NOERR;
@@ -1203,7 +1216,7 @@ ncz_read_atts(NC_FILE_INFO_T* file, NC_OBJ* container)
 	    /* Create the attribute */
 	    /* Collect the attribute's type and value  */
 	    if((stat = computeattrinfo(key->value,atypes,value,
-				   &typeid,&len,&data)))
+				   &typeid,NULL,&len,&data)))
 		goto done;
 	    if((stat = ncz_makeattr(container,attlist,key->value,typeid,len,data,&att)))
 		goto done;
@@ -1430,9 +1443,10 @@ define_vars(NC_FILE_INFO_T* file, NC_GRP_INFO_T* grp, NClist* varnames)
 	    if(jvalue == NULL)
 		var->no_fill = 1;
 	    else {
+		size_t fvlen;
 		typeid = var->type_info->hdr.id;
 		var->no_fill = 0;
-		if((stat = computeattrdata(&typeid, jvalue, NULL, &var->fill_value)))
+		if((stat = computeattrdata(&typeid, jvalue, NULL, &fvlen, &var->fill_value)))
 		    goto done;
 		assert(typeid == var->type_info->hdr.id);
 		/* Note that we do not create the _FillValue

diff --git a/ncdap_test/pathcvt.c b/ncdap_test/pathcvt.c
@@ -27,10 +27,11 @@
 /*
 Synopsis
 
-pathcvt [-u|-w|-m|-c] [-e] PATH
+pathcvt [-u|-w|-m|-c] [-e] [-d <driveletter>] PATH
 
 Options
   -e add backslash escapes to '\' and ' '
+  -d <driveletter> use driveletter when needed; defaults to 'c'
 Output type options:
   -u convert to Unix form of path
   -w convert to Windows form of path
@@ -45,7 +46,8 @@ Default is to convert to the format used by the platform.
 
 struct Options {
     int target;
-    int escape;
+    int escapes;
+    int drive;
     int debug;
 } cvtoptions;
 
@@ -60,11 +62,13 @@ main(int argc, char** argv)
     char* inpath;
 
     memset((void*)&cvtoptions,0,sizeof(cvtoptions));
+    cvtoptions.drive = 'c';
 
-    while ((c = getopt(argc, argv, "cD:ehmuw")) != EOF) {
+    while ((c = getopt(argc, argv, "cD:d:ehmuw")) != EOF) {
 	switch(c) {
 	case 'c': cvtoptions.target = NCPD_CYGWIN; break;
-	case 'e': cvtoptions.escape = 1; break;
+	case 'd': cvtoptions.drive = optarg[0]; break;
+	case 'e': cvtoptions.escapes = 1; break;
 	case 'h': usage(NULL); break;
 	case 'm': cvtoptions.target = NCPD_MSYS; break;
 	case 'u': cvtoptions.target = NCPD_NIX; break;
@@ -90,8 +94,8 @@ main(int argc, char** argv)
     if(cvtoptions.target == NCPD_UNKNOWN)
         cvtpath = NCpathcvt(inpath);
     else
-        cvtpath = NCpathcvt_test(inpath,cvtoptions.target,'c');
-    if(cvtpath && cvtoptions.escape) {
+        cvtpath = NCpathcvt_test(inpath,cvtoptions.target,(char)cvtoptions.drive);
+    if(cvtpath && cvtoptions.escapes) {
 	char* path = cvtpath; cvtpath = NULL;
         cvtpath = escape(path);
 	free(path);
@@ -116,20 +120,16 @@ escape(const char* path)
     const char* p;
     char* q;
     char* epath = NULL;
+    const char* escapes = " \\";
 
     epath = (char*)malloc((2*slen) + 1);
     if(epath == NULL) usage("out of memtory");
     p = path;
     q = epath;
     for(;*p;p++) {
-	switch (*p) {
-	case '\\': case ' ':
+	if(strchr(escapes,*p) != NULL)
 	    *q++ = '\\';
-	    /* fall thru */
-	default:
-	    *q++ = *p;
-	    break;
-	}
+        *q++ = *p;
     }
     *q = '\0';
     return epath;

diff --git a/ncdap_test/ref_pathcvt.txt b/ncdap_test/ref_pathcvt.txt
@@ -1,45 +1,36 @@
-path: /xxx/a/b
-	/xxx/a/b
-	/cygdrive/c/xxx/a/b
-	/c/xxx/a/b
-	c:\\xxx\\a\\b
-path: d:/x/y
-	/d/x/y
-	/cygdrive/d/x/y
-	/d/x/y
-	d:\\x\\y
-path: /cygdrive/d/x/y
-	/d/x/y
-	/cygdrive/d/x/y
-	/d/x/y
-	d:\\x\\y
-path: /d/x/y
-	/d/x/y
-	/cygdrive/d/x/y
-	/d/x/y
-	d:\\x\\y
-path: /cygdrive/d
-	/d
-	/cygdrive/d
-	/d
-	d:
-path: /d
-	/d
-	/cygdrive/d
-	/d
-	d:
-path: /cygdrive/d/git/netcdf-c/dap4_test/test_anon_dim.2.syn
-	/d/git/netcdf-c/dap4_test/test_anon_dim.2.syn
-	/cygdrive/d/git/netcdf-c/dap4_test/test_anon_dim.2.syn
-	/d/git/netcdf-c/dap4_test/test_anon_dim.2.syn
-	d:\\git\\netcdf-c\\dap4_test\\test_anon_dim.2.syn
-path: d:\x\y
-	/d/x/y
-	/cygdrive/d/x/y
-	/d/x/y
-	d:\\x\\y
-path: d:\x\y w\z
-	/d/x/y w/z
-	/cygdrive/d/x/y w/z
-	/d/x/y w/z
-	d:\\x\\y\\ w\\z
+path: -u: |/xxx/x/y| => |/xxx/x/y|
+path: -c: |/xxx/x/y| => |/cygdrive/c/xxx/x/y|
+path: -m: |/xxx/x/y| => |/c/xxx/x/y|
+path: -w: |/xxx/x/y| => |c:\\xxx\\x\\y|
+path: -u: |d:/x/y| => |/d/x/y|
+path: -c: |d:/x/y| => |/cygdrive/d/x/y|
+path: -m: |d:/x/y| => |/d/x/y|
+path: -w: |d:/x/y| => |d:\\x\\y|
+path: -u: |/cygdrive/d/x/y| => |/d/x/y|
+path: -c: |/cygdrive/d/x/y| => |/cygdrive/d/x/y|
+path: -m: |/cygdrive/d/x/y| => |/d/x/y|
+path: -w: |/cygdrive/d/x/y| => |d:\\x\\y|
+path: -u: |/d/x/y| => |/d/x/y|
+path: -c: |/d/x/y| => |/cygdrive/d/x/y|
+path: -m: |/d/x/y| => |/d/x/y|
+path: -w: |/d/x/y| => |d:\\x\\y|
+path: -u: |/cygdrive/d| => |/d|
+path: -c: |/cygdrive/d| => |/cygdrive/d|
+path: -m: |/cygdrive/d| => |/d|
+path: -w: |/cygdrive/d| => |d:|
+path: -u: |/d| => |/d|
+path: -c: |/d| => |/cygdrive/d|
+path: -m: |/d| => |/d|
+path: -w: |/d| => |d:|
+path: -u: |/cygdrive/d/git/netcdf-c/dap4_test/test_anon_dim.2.syn| => |/d/git/netcdf-c/dap4_test/test_anon_dim.2.syn|
+path: -c: |/cygdrive/d/git/netcdf-c/dap4_test/test_anon_dim.2.syn| => |/cygdrive/d/git/netcdf-c/dap4_test/test_anon_dim.2.syn|
+path: -m: |/cygdrive/d/git/netcdf-c/dap4_test/test_anon_dim.2.syn| => |/d/git/netcdf-c/dap4_test/test_anon_dim.2.syn|
+path: -w: |/cygdrive/d/git/netcdf-c/dap4_test/test_anon_dim.2.syn| => |d:\\git\\netcdf-c\\dap4_test\\test_anon_dim.2.syn|
+path: -u: |d:\x\y| => |/d/x/y|
+path: -c: |d:\x\y| => |/cygdrive/d/x/y|
+path: -m: |d:\x\y| => |/d/x/y|
+path: -w: |d:\x\y| => |d:\\x\\y|
+path: -u: |d:\x\y w\z| => |/d/x/y\ w/z|
+path: -c: |d:\x\y w\z| => |/cygdrive/d/x/y\ w/z|
+path: -m: |d:\x\y w\z| => |/d/x/y\ w/z|
+path: -w: |d:\x\y w\z| => |d:\\x\\y\ w\\z|
diff --git a/ncdap_test/testauth.sh b/ncdap_test/testauth.sh
@@ -41,7 +41,7 @@ LOCALRCFILES="$WD/.dodsrc $WD/.daprc $WD/.ncrc $WD/$NETRC $WD/$NETRCIMP"
 HOMERCFILES="$HOME/.dodsrc $HOME/.daprc $HOME/.ncrc $HOME/$NETRC $HOME/$NETRCIMP"
 NETRCFILE=$WD/$NETRC
 DAPRCFILE=$WD/$RC
-if test "x$FP_ISMSVC" = x1 ; then
+if test "x$FP_ISMSVC" != x ; then
     LOCALRCFILES=`${execdir}/pathcvt "$LOCALRCFILES"`
     HOMERCFILES=`${execdir}/pathcvt "$HOMERCFILES"`
     NETRCFILE=`${execdir}/pathcvt "$NETRCFILE"`