Add support for HDF5 transient types

[ZedThree]

These are compound or enum datatypes stored directly in the dataset itself, and so aren't read as standalone objects during the initial metadata gathering when the file is opened

Closes #2651
Fixes #267

I've added a fairly simple test, maybe it should be expanded to include transient types on attributes, and other types (like enums, as in #267)

This change unfortunately also causes nc_test4_tst_h_refs to fail, and I've not managed to work out why. Fixed

[WardF]

To expand on Dennis' comment, we'll need to consider the same issues outlined in #2652. I've been occupied with summer intern stuff the last few days, let me refresh my memory and tag in folk as/if need be.

[ZedThree]

I'm pretty sure this should be fine for netCDF Java, assuming it uses the C library to read HDF5 files, as the unnamed type gets read in by the netCDF C library on the initial file open as it iterates over all objects in the HDF5 file. It's just that instead of being encountered as a standalone HDF5 object in the file, it's encountered as part of reading the metadata for the associated dataset. It's still added to the list of known types in the file, and can be queried:

    if (nc_open(FILE_NAME, NC_NOWRITE, &ncid)) ERR;

    if (nc_inq_typeids(ncid, &num_types, NULL)) ERR;
    typeids = (int*)malloc(num_types * sizeof(int));
    if (nc_inq_typeids(ncid, NULL, typeids)) ERR;

    if (nc_inq_user_type(ncid, typeids[0], name, &size, &base_nc_type, &nfields, &class)) ERR;
    printf("\n\n***** type name: '%s', size: %zu, fields: %zu\n\n", name, size, nfields);

prints: ***** type name: '', size: 16, fields: 2.

The missing type name is perhaps the only point of contention in regards to the data model, but an unambiguous name could be given to it on the netCDF side, e.g. <variable name>_type

There's also no issues of backwards compatibility that I can see -- no previously valid files will become invalid. It also only affects reading, so newer versions of netCDF won't make possibly invalid files for older versions.
More files written by other tools (such as h5netcdf with invalid_netcdf=True) would be able to be read using this feature.

[ZedThree]

The appveyor build is failing, I guess because it's not using a C99 compliant compiler and the new test uses double complex. I can change the test to use a struct instead.

The failing MacOS build has this error:

Error:curl error: Couldn't connect to server
curl error details: 
Warning:oc_open: Could not read url
/Users/runner/work/netcdf-c/netcdf-c/build/ncdump/ncdump: [cache][prefetch]https://remotetest.unidata.ucar.edu/dts/test.07?types.f32: NetCDF: I/O failure

It looks like a network failure? But everything else works, so that's a bit odd

[DennisHeimbigner]

You could use your hdf5 test program to create the test file and then see what happens when
netcdf-java tries to open it.

[ethanrd]

@ZedThree - The netCDF-Java library uses its all Java implementation to read nc-4/HDF5 files. Writing, on the other hand, is done through the netCDF-C library.

[DennisHeimbigner]

Right, but it must be able to read the file created by your hdf5 program.

[ZedThree]

I don't have a Java development environment set up, but I was able to use the ToolsUI utility to view the file created by this test, and it was able to view the presence of the variables at least:

[ZedThree]

The one failing test is a dap4 test, which this PR doesn't touch at all:

1/1 Test #215: dap4_test_test_hyrax .............***Failed    2.11 sec
unknown argument test_hyrax.sh
test_hyrax.sh:
testing: dap4://test.opendap.org/opendap/nc4_test_files/nc4_nc_classic_comp.nc#checksummode=ignore
testing: dap4://test.opendap.org/opendap/nc4_test_files/nc4_unsigned_types_comp.nc#checksummode=ignore
testing: dap4://test.opendap.org/opendap/nc4_test_files/nc4_strings_comp.nc#checksummode=ignore
***FAIL: url=http://test.opendap.org/opendap/nc4_test_files/nc4_strings_comp.nc#mode=dap4&checksummode=ignore httpcode=[50](https://github.com/Unidata/netcdf-c/actions/runs/4404151098/jobs/7713416895#step:12:51)0 errmsg->
<?xml version="1.0" encoding="UTF-8"?>
<dap4:Error xmlns:dap4="http://xml.opendap.org/ns/DAP/4.0#" httpcode="500">
  <dap4:Message>ERROR - Unable to parse expected &amp;lt;BESError&amp;gt; object from stream! Message: Error on line 43: Content is not allowed in trailing section.</dap4:Message>
  <dap4:Context />
  <dap4:OtherInformation />
</dap4:Error>


NetCDF: DAP server error

I can only reproduce this locally very rarely -- are there any known issues with this fileserver?

[DennisHeimbigner]

Try the ncdump tab also on toolsui

[ZedThree]

I tried the ncdump tab on a few files, and it didn't work for any of them. It doesn't produce any error messages, so I can't debug it

[ZedThree]

Is it possible for someone with access to re-run the nc-cmake-tests-oneoff-parallel (1.12.2) build to see if it is an intermittent failure, please?

[ZedThree]

Oh, I've just seen #2555 and #2558, could the test failure be related to them?

[WardF]

Re-running jobs. I'll echo what @ethanrd and @DennisHeimbigner mentioned above; if netCDF-Java can read these out of the box, great, if not, we'll need to have that broader conversation. But first things first, I've started the tests. Thanks!

[ZedThree]

It looks like netCDF-Java can already read these variables out of the box:

$ java -jar netcdfAll-5.5.3.jar tst_h_transient.h5  -v "var;bool_var"
SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.
netcdf tst_h_transient.h5 {
  types:
    enum bool_var { 'FALSE' = 0, 'TRUE' = 1};

  variables:
    enum bool_var bool_var;


    Structure {
      double r;
      double i;
    } var;



  data:
    var = 
      {
        r = 1.0
        i = 2.0
      } var(0)
    bool_var = 1
}

This is the file created as part of the new test for transient types. With this PR, netCDF-C can now read the file:

$ ncdump tst_h_transient.h5 
netcdf tst_h_transient {
types:
  byte enum  {FALSE = 0, TRUE = 1} ;
  compound  {
    double r ;
    double i ;
  }; // 
variables:
         bool_var ;
         var ;
data:

 bool_var = TRUE ;

 var = {1, 2} ;
}

and without:

$  ncdump tst_h_transient.h5 
netcdf tst_h_transient {
}

---

I'm still not sure why that one test is failing. I notice that #2681 was recently merged, maybe that will help?

[ZedThree]

Hello, is there anything I can do to help move this along? It would be a very useful feature to enable greater compatibility between different netCDF implementations: netCDF-Java and h5netcdf can both already read these kinds of files.

[WardF]

@ZedThree thanks for the ping!

[DennisHeimbigner]

This PR needs to be reversed. It is creating types with empty names and that violates the netcdf-4
specification. Plus it will cause various programs to crash.

[WardF]

It passed all the tests; do we need to update our test suite in a way that will test for the failures you're observing?

[DennisHeimbigner]

The test is bogus. It never checks for the type name.

[ZedThree]

It is creating types with empty names and that violates the netcdf-4 specification.

This is absolutely not the case, it only adds the ability to read such types. From my comment above:

There's also no issues of backwards compatibility that I can see -- no previously valid files will become invalid. It also only affects reading, so newer versions of netCDF won't make possibly invalid files for older versions.
More files written by other tools (such as h5netcdf with invalid_netcdf=True) would be able to be read using this feature.

NetCDF-Java can already read such types, so this only brings netCDF-C functionality in line with the Java library.

It's also not clear that it is against the spec. Is there a more formal spec than this page in the docs? In relation to user-defined types, it says:

Each user-defined data type in an HDF5 file exactly corresponds to a user-defined data type in the netCDF-4 file. Only base data types which correspond to netCDF-4 data types may be used.

I don't see the issue here.

Plus it will cause various programs to crash.

If you can provide a minimal reproducer, I will be happy to fix any bugs I've introduced and expand the test suite. As it is, the entire existing test suite passes along with the additional tests I wrote. As you requested, I also verified that netCDF-Java can already correctly read files with unnamed types in them.

[DennisHeimbigner]

This is absolutely not the case, it only adds the ability to read such types. From my comment above:

Modify your test program to print the type name.

[DennisHeimbigner]

NetCDF-Java can already read such types, so this only brings netCDF-C functionality in line with the Java library.

netcdf-java assigns names to those types. your code does not.

[DennisHeimbigner]

As it is, the entire existing test suite passes along with the additional tests I wrote.

But no other tests operate on a transient type. The only test that does is your test case.

[DennisHeimbigner]

The spec requires type names to be unique within a group. Your code produces types all with the same empty name.

[DennisHeimbigner]

The right thing to do here is to make sure that these transient types have proper names.
We have a precedent WRT anonymous dimensions in DAP4. I would suggest that we add a counter that is incremented whenever a transient type is created. Then we create the name of the type as, say, "_AnonymousN".
Also, it is possible to encounter transient types associated with attributes?