Bad parsing of Zarr metadata in case empty variable (v. 4.9.2)

[minomicetto]

I was experiencing a strange error due to the presence of an empty variable in input zarr dataset.
You can reproduce it just try to access any/most of daily datasets with the variable "tasmin" from https://cmip6.storage.googleapis.com/pangeo-cmip6.csv via s3, for instance

gs://cmip6/CMIP6/CMIP/NASA-GISS/GISS-E2-1-G/historical/r1i1p1f1/day/tasmin/gn/v20181015/
gs://cmip6/CMIP6/CMIP/AWI/AWI-CM-1-1-MR/historical/r1i1p1f1/day/tasmin/gn/v20181218/
gs://cmip6/CMIP6/CMIP/BCC/BCC-ESM1/historical/r1i1p1f1/day/tasmin/gn/v20181220/
gs://cmip6/CMIP6/CMIP/SNU/SAM0-UNICON/historical/r1i1p1f1/day/tasmin/gn/v20190323/
gs://cmip6/CMIP6/CMIP/CCCma/CanESM5/historical/r1i1p1f1/day/tasmin/gn/v20190429/
gs://cmip6/CMIP6/CMIP/MRI/MRI-ESM2-0/historical/r1i1p1f1/day/tasmin/gn/v20190603/
gs://cmip6/CMIP6/CMIP/HAMMOZ-Consortium/MPI-ESM-1-2-HAM/historical/r1i1p1f1/day/tasmin/gn/v20190627/
gs://cmip6/CMIP6/CMIP/MPI-M/MPI-ESM1-2-LR/historical/r1i1p1f1/day/tasmin/gn/v20190710/
gs://cmip6/CMIP6/CMIP/MPI-M/MPI-ESM1-2-HR/historical/r1i1p1f1/day/tasmin/gn/v20190710/
gs://cmip6/CMIP6/CMIP/NUIST/NESM3/historical/r1i1p1f1/day/tasmin/gn/v20190812/
gs://cmip6/CMIP6/CMIP/NCC/NorESM2-LM/historical/r1i1p1f1/day/tasmin/gn/v20190815/
gs://cmip6/CMIP6/CMIP/CAS/FGOALS-g3/historical/r1i1p1f1/day/tasmin/gn/v20190826/
gs://cmip6/CMIP6/CMIP/MIROC/MIROC6/historical/r1i1p1f1/day/tasmin/gn/v20191016/
gs://cmip6/CMIP6/CMIP/CSIRO-ARCCSS/ACCESS-CM2/historical/r1i1p1f1/day/tasmin/gn/v20191108/
gs://cmip6/CMIP6/CMIP/NCC/NorESM2-MM/historical/r1i1p1f1/day/tasmin/gn/v20191108/
gs://cmip6/CMIP6/CMIP/CSIRO/ACCESS-ESM1-5/historical/r1i1p1f1/day/tasmin/gn/v20191115/
gs://cmip6/CMIP6/CMIP/AWI/AWI-ESM-1-1-LR/historical/r1i1p1f1/day/tasmin/gn/v20200212/
gs://cmip6/CMIP6/CMIP/AS-RCEC/TaiESM1/historical/r1i1p1f1/day/tasmin/gn/v20200626/
gs://cmip6/CMIP6/CMIP/NCC/NorCPM1/historical/r1i1p1f1/day/tasmin/gn/v20200724/
gs://cmip6/CMIP6/CMIP/CMCC/CMCC-ESM2/historical/r1i1p1f1/day/tasmin/gn/v20210114/

and consider the empty variable "height". See, for example, https://storage.googleapis.com/cmip6/CMIP6/CMIP/NASA-GISS/GISS-E2-1-G/historical/r1i1p1f1/day/tasmin/gn/v20181015/height/.zarray for further information regarding the empty variable "height".

More in detail, a NC_ENCZARR is returned when the nc library checks the variable "zarr_rank" to be equal to 0 at
https://github.com/Unidata/netcdf-c/blob/main/libnczarr/zsync.c#L1668
as, I believe, it assumes the variable "height" is not scalar (it has not "shape" nor "chunk").
Actually, the nc library can detect this just at https://github.com/Unidata/netcdf-c/blob/main/libnczarr/zsync.c#L1608

Of course, the best solution is to remove any references to "height" from input dataset as they are useless but, in general, empty variables are not uncommon, even in NetCDF files.
A naive solution I adopted is to change https://github.com/Unidata/netcdf-c/blob/main/libnczarr/zsync.c#L1659 from

if(zvar->scalar) {

to

if(zvar->scalar || !zarr_rank) {

so that the empty variable "height" is considered scalar and, then, skipped (I believe)... but I don't know what bad consequences this modification has.
Could you provide a smarter solution to handle this kind of empty variables in general, please?

Bye

[DennisHeimbigner]

Interesting, I would have said that such an empty variable is illegal in Zarr.
Have you checked the Zarr V2 spec?
Note also that pure Zarr has no concept of scalar -- that is an NCZarr extension.

Having said that, we should be able to provide a more reasonable response than a failure.
I will implement your proposal or something similar.

[minomicetto]

Thanks Dennis.
Considering Zarr specification reported at https://zarr.readthedocs.io/en/stable/spec/v2.html, I see no particular constraint is provided, for example, about the metadata called "shape", defined as "A list of integers defining the length of each dimension of the array". In the examples, this list always contains at least one element, it never is not empty, so that the simplest data should be an one-dimension array whose length is provided by the only item in "shape" list. I assume if the only item of the list is "1" then the array could be consided as a "scalar" variable (beyond the NCZarr extension where this concept is well defined). Anyway, no explicit meaning is provided by the specification in case of arrays without a "shape" (where this list is empty).
I understand that, in case there is an array without a "shape", the entire dataset "might" be considered as a non-compliant Zarr dataset, but I believe that negleting those variables of the dataset with this kind of inconsistency (eventually raising a simple warning) would be a more effectiveness approach, that would make NetCDF library more robust.
I thank you again.
Please, close this issue whenever you prefer.

[DennisHeimbigner]

I agree that we should do something sensible with malformed metadata whenever we can.
The question is always: try to suppress/ignore the problem vs reporting an error.
As a rule, we have reported an error. But as you say, a flaw in one variable should not
prevent reading other, well-formed variables.

[DennisHeimbigner]

I don't understand how to parse the above URLs since they are not Amazon S3.
But I have attached a version of zsync.c that you might try to see if it fixes this problem.
zsync.c.txt
Never mind.

[minomicetto]

However, I tried to verify your proposal for zsync.c but, by replacing this file with the one present in netcdf 4.9.2, I detect some compilation errors, I think, due to recent changes in the development code. Which commit did you start from?
I read the code of your proposal and it seems correct: if there are variables with zarr_rank equal to zero then those variables are suppressed: ok.
To test the code you can use the following command:

ncdump -hs https://storage.googleapis.com/cmip6/CMIP6/CMIP/NASA-GISS/GISS-E2-1-G/historical/r1i1p1f1/day/tasmin/gn/v20181015/#mode=zarr,s3

(instead using the protocol gs, use simply https to retrieve data from storage.googleapis.com via s3).
This command results in error in case the empty variable is not suppressed as follows:

ncdump: https://storage.googleapis.com/cmip6/CMIP6/CMIP/NASA-GISS/GISS-E2-1-G/historical/r1i1p1f1/day/tasmin/gn/v20181015/#mode=zarr,s3: NetCDF: NCZarr error

while returning the file header on success.

[DennisHeimbigner]

Yes, It turned out there are some knock-on complications that I needed to address.
I am finishing up a full PR to address this issue. Should be up in a day or two.