Using nccopy, setting deflate level to 0 with shuffling results in an uncompressed, shuffled file

[bronaugh]

We noticed this recently because a co-worker of mine created compressed files with shuffling, but then decompressed them due to the I/O and CPU overhead of compression (as compression forces chunk-sized reads). The problem is, chunk-sized reads were still happening. The reason seems to be that while compression was disabled, shuffling was not; and this has the same effect of forcing chunk-sized reads.

I think it's fairly clear that setting compression to none should disable shuffling as well. I've written a patch which includes this behaviour as well as allowing enabling/disabling of shuffling independent of compression.

[bronaugh]

diff --git a/ncdump/nccopy.c b/ncdump/nccopy.c
index 6718b74..af7a416 100644
--- a/ncdump/nccopy.c
+++ b/ncdump/nccopy.c
@@ -40,11 +40,13 @@ int optind;
 #define NC_CLASSIC_MODEL 0x0100 /* Enforce classic model if netCDF-4 not available. */
 #endif

+#define UNDEFINED -1
+
 /* Global variables for command-line requests */
 char *progname;               /* for error messages */
 static int option_kind = SAME_AS_INPUT;
-static int option_deflate_level = -1;  /* default, compress output only if input compressed */
-static int option_shuffle_vars = NC_NOSHUFFLE; /* default, no shuffling on compression */
+static int option_deflate_level = UNDEFINED;   /* default, compress output only if input compressed */
+static int option_shuffle_vars = UNDEFINED; /* default, no shuffling on compression */
 static int option_fix_unlimdims = 0; /* default, preserve unlimited dimensions */
 static char* option_chunkspec = 0;   /* default, no chunk specification */
 static size_t option_copy_buffer_size = COPY_BUFFER_SIZE;
@@ -534,15 +536,20 @@ copy_var_specials(int igrp, int varid, int ogrp, int o_varid)
        * with command-line options */
        int shuffle, deflate, deflate_level;
        NC_CHECK(nc_inq_var_deflate(igrp, varid, &shuffle, &deflate, &deflate_level));
+       if(option_shuffle_vars == UNDEFINED) {
+         if(option_deflate_level == 0)
+           shuffle = NC_NOSHUFFLE;
+       } else {
+         shuffle = option_shuffle_vars;
+       }
+
        if(deflate_level == 0 && deflate == 1) /* TODO: why is this needed??  Bug in nc_inq_var_deflate? */
            deflate = 0;
-       if(option_deflate_level >= 0) { /* change output compression, if requested */
+       if(option_deflate_level != UNDEFINED) { /* change output compression, if requested */
          deflate_level = option_deflate_level;
          deflate=1;
        }
-       if(shuffle==0 && option_shuffle_vars != 0) {
-         shuffle = option_shuffle_vars;
-       }
+
        if(deflate != 0 || shuffle != 0) {
            NC_CHECK(nc_def_var_deflate(ogrp, o_varid, shuffle, deflate_level > 0, deflate_level));
        }
@@ -1511,6 +1518,7 @@ usage(void)
            1 classic, 2 64-bit offset, 3 netCDF-4, 4 netCDF-4 classic model\n\
   [-d n]    set deflation compression level, default same as input (0=none 9=max)\n\
   [-s]      add shuffle option to deflation compression\n\
+  [-S]      remove shuffle option to deflation compression\n\
   [-c chunkspec] specify chunking for dimensions, e.g. \"dim1/N1,dim2/N2,...\"\n\
   [-u]      convert unlimited dimensions to fixed-size dimensions in output copy\n\
   [-w]      write whole output file from diskless netCDF on close\n\
@@ -1578,7 +1586,7 @@ main(int argc, char**argv)
        usage();
     }

-    while ((c = getopt(argc, argv, "k:d:sum:c:h:e:rwxg:G:v:V:")) != -1) {
+    while ((c = getopt(argc, argv, "k:d:sSum:c:h:e:rwxg:G:v:V:")) != -1) {
        switch(c) {
         case 'k': /* for specifying variant of netCDF format to be generated 
                      Possible values are:
@@ -1615,8 +1623,15 @@ main(int argc, char**argv)
            }
            break;
        case 's':               /* shuffling, may improve compression */
+           if(option_shuffle_vars != UNDEFINED)
+               error("Can't specify both -s and -S!");
            option_shuffle_vars = NC_SHUFFLE;
            break;
+       case 'S':               /* shuffling, may improve compression */
+           if(option_shuffle_vars != UNDEFINED)
+               error("Can't specify both -s and -S!");
+           option_shuffle_vars = NC_NOSHUFFLE;
+           break;
        case 'u':               /* convert unlimited dimensions to fixed size */
            option_fix_unlimdims = 1;
            break;

[russrew]

Thanks for the fix. I don't think there is actually any use case for supporting shuffling without compression, so I think we don't need the "-S" option you've added. I'm assuming with your fix, if "-d0" is specified without "-s", the new copy will not use compression or shuffling, and hence no chunking, but I haven't tested that yet. I think the correct behavior should be to use contiguous layout rather than chunking if "-d0" is specified and no other option is specified that would require chunking, such as "-c chunkspec". Please let me know if I've overlooked a need for the "-S" option.

[bronaugh]

Currently, if you use nccopy -s to enable shuffling on a dataset, there is no way to turn shuffling off. Likewise, if you create a shuffled, compressed dataset, there is no way to turn shuffling off. Not providing functionality to remove shuffling seems like a really bad plan; if this functionality existed, I wouldn't have had to patch nccopy to remove shuffling from files we removed compression from -- I would have simply used the -S switch with nccopy, as I have done with the patched version.

Regarding the behaviour when disabling compression -- yes, line 541 of the patched version will set NC_NOSHUFFLE in that case.

Regarding chunking, as it stands turning off shuffling and compression definitely doesn't change the fact that it's a chunked layout, as shown by ncdump -h -s on a file I tested on:

                tasmin:_Storage = "chunked" ;
                tasmin:_ChunkSizes = 1, 510, 1068 ;

I'm not 100% sure what the I/O implications of chunking are. I presume that if the data is not shuffled and not compressed, then a linear layout can be assumed and the data can be read in by only reading in the needed values, not the entire chunk as is required if the data is shuffled and/or compressed. IF I am correct about this (and it is an if), then I'm not sure if chunked vs contiguous really matters much at all here.

[russrew]

It turns out that there are other properties on a netCDF-4 or HDF5 file that require chunking, besides it being compressed or using shuffling:

• use of unlimited (record) dimensions, supporting extendible variables
• optimizing frequent I/O access patterns, for performance
• use of other I/O pipeline filters, such as checksums or user-defined custom filters

Therefore, I think the problem you have identified with nccopy requires a different (but similar) fix, that explicitly turns off chunking in the output copy, using contiguous layout for all variables. In addition to turning off compression and shuffling, this would also fix the size of all unlimited dimensions.

I propose to use "-C" for this purpose, replacing chunked layout with contiguous layout for all variables. Specifying "-C", besides turning off compression and shuffling, would also make any unlimited dimensions fixed in size, as the "-u" option now does. It would be an error to specify compression or chunking with "-C".

I invite comments on this proposal.

[bronaugh]

I think this is not a good proposal. It changes the format of the data as a side effect; this is counter-intuitive behaviour. My own perspective is that keeping the switches simple and clear, having exactly the effects that they declare, and making sure that every flag that can be set, can also be cleared, is the desirable way to go here.

[russrew]

Agree that keeping switches simple and clear is desirable. However, variable "shuffling" only makes sense in the context of deflation, so turning shuffling off when deflation is turned off with "-d0" seems simple and clear to me. Rather than the complex "-C" switch for turning off chunking, it turns out that use of a "/" chunkspec appears to be a better way to do this, since it lists no dimensions and no chunk lengths. I've implemented this and am testing it for inclusion in the 4.3.2 version.

This is Jira ticket: https://bugtracking.unidata.ucar.edu/browse/NCF-295