Skip to content

fix: nc_def_var_chunking_ints should dispatch to format-aware handler - #3399

Open
tomdurrant wants to merge 2 commits into
Unidata:mainfrom
tomdurrant:fix/nczarr-chunking-ints-dispatch
Open

fix: nc_def_var_chunking_ints should dispatch to format-aware handler#3399
tomdurrant wants to merge 2 commits into
Unidata:mainfrom
tomdurrant:fix/nczarr-chunking-ints-dispatch

Conversation

@tomdurrant

Copy link
Copy Markdown

Problem

nc_def_var_chunking_ints() is the Fortran-compatible wrapper (accepts int* chunksizes) called by nf90_def_var_chunking() in netcdf-fortran. It had two broken paths for NCZarr files:

  1. NCZarr-only builds (no HDF5): the stub in libsrc4/nc4cache.c returned NC_NOERR without doing anything, so chunk sizes were never applied. The variable kept default chunk sizes (full dimension), producing a single chunk file with full-variable data.

  2. HDF5-enabled builds: the implementation in libhdf5/hdf5var.c called nc_def_var_extra() which set var->chunksizes (metadata correct) but did NOT update NCZarr-specific cache state (zvar->chunkproduct, zvar->chunksize, zvar->cache->valid). The write path used stale cache values, writing full-variable-sized chunk data files.

Both cases produced correct .zarray metadata but wrong chunk data files — unreadable by zarr-python and xarray.

See: Unidata/netcdf-fortran#487

Fix

Make nc_def_var_chunking_ints() convert int* to size_t* and delegate to nc_def_var_chunking(), which dispatches to the correct format backend (NCZ_def_var_chunking for NCZarr, NC4_def_var_chunking for HDF5). Both correctly update all internal state.

Changes

  • Replace stub in libsrc4/nc4cache.c with dispatch-aware implementation
  • ifdef-out the HDF5-only version in libhdf5/hdf5var.c
  • Add regression test nczarr_test/tst_chunking_ints.c

Verification

  • C tests: all 4 chunk files correctly sized at 65536 bytes (was 262144)
  • Fortran reproducer: chunks correctly applied, output readable by both zarr-python and xarray
  • No HDF5 regressions: dispatch path unchanged for HDF5 files

nc_def_var_chunking_ints() is the Fortran-compatible wrapper (accepts int*
chunksizes instead of size_t*) called by nf90_def_var_chunking() in
netcdf-fortran. It had two broken paths for NCZarr files:

1. NCZarr-only builds (no HDF5): the stub in libsrc4/nc4cache.c returned
   NC_NOERR without doing anything, so chunk sizes were never applied.
   The variable kept default chunk sizes (full dimension), producing a
   single chunk file with full-variable data.

2. HDF5-enabled builds: the implementation in libhdf5/hdf5var.c called
   nc_def_var_extra() which set var->chunksizes (metadata correct) but
   did NOT update NCZarr-specific cache state (zvar->chunkproduct,
   zvar->chunksize, zvar->cache->valid). The write path used stale
   cache values, writing full-variable-sized chunk data files.

The fix: make nc_def_var_chunking_ints() convert int* to size_t* and
delegate to nc_def_var_chunking(), which dispatches to the correct
format backend (NCZ_def_var_chunking for NCZarr, NC4_def_var_chunking
for HDF5). Both correctly update all internal state.

Fixes Unidata/netcdf-fortran#487.
Reported-by: tdurrant

- Replace stub in libsrc4/nc4cache.c with dispatch-aware implementation
- ifdef-out the HDF5-only version in libhdf5/hdf5var.c
- Add regression test nczarr_test/tst_chunking_ints.c
@tomdurrant
tomdurrant requested a review from WardF as a code owner June 25, 2026 02:19
@CLAassistant

CLAassistant commented Jun 25, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@WardF

WardF commented Jul 1, 2026

Copy link
Copy Markdown
Member

Can you explain in plain language what issue you are addressing? This appears to be AI generated, and will be closed without a human in the loop.

@tomdurrant

Copy link
Copy Markdown
Author

Hi @WardF ,
When writing chunked data to NCZarr (the Zarr-based storage backend) from Fortran, the chunk sizes specified by the user were being silently ignored — the library would fall back to writing a single chunk covering the entire variable, producing incorrect output. This seems to be happening due to the Fortran wrapper function (nc_def_var_chunking_ints) bypassed the NCZarr-specific internal setup that the normal C function goes through. The fix routes it through the same code path so chunking works correctly for Fortran+NCZarr. Fixes are largely AI generated, there may be better ways to do it. But as things stand, user specified chunk sizes are not being applied in fortran applications.

@WardF

WardF commented Aug 3, 2026

Copy link
Copy Markdown
Member

The tests appear to be failing.

The regression test for nc_def_var_chunking_ints failed CI for two
reasons:

1. Missing declaration: nc_def_var_chunking_ints is a Fortran-compat
   wrapper declared in netcdf_f.h, which the test did not include.
   Every autotools/cmake job (Ubuntu, Docker, Cygwin, MINGW, UCRT64,
   macOS) failed to compile tst_chunking_ints.c with 'implicit
   declaration of function'. Add #include <netcdf_f.h>.

2. Raw stat() on Windows: MSVC compiled (lenient implicit decl) but
   the runtime size check read garbage (1782925324 bytes for a 64KB
   chunk) because plain struct stat/stat() is not portable on
   Windows. Use the codebase-standard NCstat()/struct _stat64
   pattern from ncpathmgr.h (same as libnczarr/zmap_file.c), which
   also performs the required NCpathcvt path conversion.
@tomdurrant

tomdurrant commented Aug 4, 2026

Copy link
Copy Markdown
Author

CI fix for the failing tests in this PR (pushed as fc3b900da).

The failing checks traced to two problems in the new regression test nczarr_test/tst_chunking_ints.c:

  1. Compile error on every autotools/cmake job (Docker regression, Cygwin autotools+cmake, MSYS2/MINGW64, UCRT64, macOS, Ubuntu): tst_chunking_ints.c:66 calls nc_def_var_chunking_ints() without a declaration — the function is declared in include/netcdf_f.h, which the test didn't include. Fixed by adding #include <netcdf_f.h>.

  2. MSVC-only runtime failure: MSVC compiles (implicit-declaration is only a warning there), so the test ran, but the chunk-file size check reported garbage (1782925324 bytes for a 64 KiB chunk). Plain stat()/struct stat is not portable on Windows — the codebase standard is NCstat() with struct _stat64 from ncpathmgr.h (same pattern as libnczarr/zmap_file.c). Rewrote get_file_size() accordingly.

Verified locally: tst_chunking_ints passes and all 25 nczarr_test ctests pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants