Skip to content

Commit 4a73a43

Browse files
konstibobclaude
andauthored
Honour the Zarr v2 order key instead of ignoring it (#95)
`.zarray`'s `order` was parsed into `ArrayMetadata.order` and then never read, so `"order": "F"` was silently treated as row-major in both directions: * Reading an F-order store written by zarr-python returned the elements of each chunk transposed. A 3x4 int32 array holding [[0,1,2,3],[4,5,6,7], [8,9,10,11]] read back as [0, 4, 8, 1, 5, 9, 2, 6, 10, 3, 7, 11]. * Writing was worse: `withOrder(Order.F)` put `"order": "F"` into `.zarray` while laying the chunk out row-major, so zarr-java emitted stores that every other implementation misreads. zarr-python read such a store as [[0,3,6,9],[1,4,7,10],[2,5,8,11]]. Because the reader shared the bug, the round trip through zarr-java itself returned the correct values and no Java-only test could see the corruption. Zarr v3 dropped `order` and expresses the same layout with the `transpose` codec, so `order` is modelled the same way here: a new `FortranOrderCodec` array->array codec reverses all axes ahead of the `BytesCodec`, whose row-major serialization of the reversed view is exactly the column-major serialization of the logical chunk. Reversing axes is its own inverse, so one operation serves both directions. The codec is never serialized; `order` stays the on-disk representation, and it is only inserted for rank > 1, where the two orders actually differ. `decode` materializes the result with `Array.copy()`. A permuted view answers iterators and `get(int[])` correctly but keeps a column-major backing store, so linear accessors such as `getInt(int)` would walk it in the wrong order — and the single-full-chunk fast path in `core.Array.read` hands a decoded chunk straight to the caller. Testing uses zarr-python as the oracle, since a Java-only round trip cannot detect a symmetric bug: * `testdata/golden/` gains four fixtures written by zarr-python (1.3 kB in total, no compressor, so the chunk files are raw element bytes), covering a C/F pair, rank 3, and a chunk grid that leaves partial edge chunks. `src/test/python-scripts/generate_v2_order_golden.py` regenerates them. * `ZarrV2OrderTest` checks both directions against those fixtures offline, with no Python at test time: reads must yield `arange`, and writing `arange` must reproduce zarr-python's chunk bytes exactly. It also pins down that C and F differ on disk (uncompressed and compressed), that the two coincide for rank 1, that a single-full-chunk read is physically contiguous, and that `order` survives `resize` and attribute updates. 7 of its 15 tests fail without this change. * `ZarrPythonTests` gains `testReadOrderV2`/`testWriteOrderV2`, which drive the installed zarr-python in both directions so that a divergence from the committed fixtures is noticed after a zarr-python upgrade. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent cab2efc commit 4a73a43

22 files changed

Lines changed: 739 additions & 0 deletions

File tree

‎src/main/java/dev/zarr/zarrjava/v2/Array.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
import dev.zarr.zarrjava.utils.Utils;
1414
import dev.zarr.zarrjava.v2.codec.Codec;
1515
import dev.zarr.zarrjava.v2.codec.core.BytesCodec;
16+
import dev.zarr.zarrjava.v2.codec.core.FortranOrderCodec;
1617

1718
import javax.annotation.Nonnull;
1819
import javax.annotation.Nullable;
@@ -36,6 +37,12 @@ protected Array(StoreHandle storeHandle, ArrayMetadata arrayMetadata) throws IOE
3637
this.metadata = arrayMetadata;
3738
this.codecPipeline = new CodecPipeline(Utils.concatArrays(
3839
new Codec[]{},
40+
// "order": "F" means the chunk is serialized column-major. Reversing all axes before
41+
// the BytesCodec turns its row-major serialization into a column-major one. For rank
42+
// 0 and 1 both orders are identical, so no codec is needed there.
43+
metadata.order == Order.F && metadata.ndim() > 1
44+
? new Codec[]{new FortranOrderCodec()}
45+
: new Codec[]{},
3946
metadata.filters == null ? new Codec[]{} : metadata.filters,
4047
new Codec[]{new BytesCodec(arrayMetadata.endianness.toEndian())},
4148
metadata.compressor == null ? new Codec[]{} : new Codec[]{metadata.compressor}
Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
package dev.zarr.zarrjava.v2.codec.core;
2+
3+
import dev.zarr.zarrjava.ZarrException;
4+
import dev.zarr.zarrjava.core.ArrayMetadata;
5+
import dev.zarr.zarrjava.core.codec.ArrayArrayCodec;
6+
import dev.zarr.zarrjava.v2.codec.Codec;
7+
import ucar.ma2.Array;
8+
9+
/**
10+
* Realizes the Fortran (column-major) chunk layout that Zarr v2 selects with {@code "order": "F"}.
11+
* <p>
12+
* Zarr v2 stores the elements of a chunk either in row-major ({@code "C"}) or column-major
13+
* ({@code "F"}) order. Zarr v3 dropped the {@code order} key and expresses the same thing with the
14+
* {@code transpose} codec, so the v2 {@code order} key is modelled here the same way: as an
15+
* array &rarr; array codec that reverses all axes. The row-major serialization of an array with
16+
* reversed axes is exactly the column-major serialization of the original array.
17+
* <p>
18+
* This codec is never part of the {@code .zarray} document. It is inserted into the codec pipeline
19+
* by {@link dev.zarr.zarrjava.v2.Array} whenever the metadata says {@code "order": "F"}, and the
20+
* {@code order} key alone remains the on-disk representation.
21+
*/
22+
public class FortranOrderCodec extends ArrayArrayCodec implements Codec {
23+
24+
/**
25+
* Reversing all axes is its own inverse, so encoding and decoding differ only in what they do
26+
* with the resulting view.
27+
*/
28+
private Array reverseAxes(Array chunkArray) throws ZarrException {
29+
int ndim = resolveArrayMetadata().ndim();
30+
if (chunkArray.getRank() != ndim) {
31+
throw new ZarrException(
32+
"Chunk array has rank " + chunkArray.getRank() + ", but the array has " + ndim
33+
+ " dimensions.");
34+
}
35+
int[] reversedAxes = new int[ndim];
36+
for (int i = 0; i < ndim; i++) {
37+
reversedAxes[i] = ndim - 1 - i;
38+
}
39+
return chunkArray.permute(reversedAxes);
40+
}
41+
42+
/**
43+
* Turns the logical chunk into the axis-reversed view that the following
44+
* {@link dev.zarr.zarrjava.core.codec.core.BytesCodec} then serializes row-major, which yields
45+
* the column-major serialization of the logical chunk. The view is handed on as-is: the
46+
* {@code BytesCodec} flattens in logical index order, so no copy is needed here.
47+
*/
48+
@Override
49+
public Array encode(Array chunkArray) throws ZarrException {
50+
return reverseAxes(chunkArray);
51+
}
52+
53+
/**
54+
* Turns the axis-reversed array that came out of the
55+
* {@link dev.zarr.zarrjava.core.codec.core.BytesCodec} back into the logical chunk.
56+
* <p>
57+
* The result is materialized with {@link Array#copy()} rather than returned as a permuted view.
58+
* A view's logical order is correct for iterators and index-based access, but its backing store
59+
* is still in column-major order, so linear accessors such as {@code getInt(int)} would return
60+
* elements in the wrong order. Chunks decoded here can be handed straight to callers by the
61+
* single-full-chunk fast path in {@link dev.zarr.zarrjava.core.Array#read(long[], long[])}, so
62+
* they have to behave like any other decoded chunk.
63+
*/
64+
@Override
65+
public Array decode(Array chunkArray) throws ZarrException {
66+
return reverseAxes(chunkArray).copy();
67+
}
68+
69+
/**
70+
* Reports the axis-reversed shapes to the following codec, so that the
71+
* {@link dev.zarr.zarrjava.core.codec.core.BytesCodec} reads and writes the chunk bytes with the
72+
* reversed chunk shape.
73+
*/
74+
@Override
75+
public ArrayMetadata.CoreArrayMetadata resolveArrayMetadata() throws ZarrException {
76+
super.resolveArrayMetadata();
77+
int ndim = arrayMetadata.ndim();
78+
long[] reversedShape = new long[ndim];
79+
int[] reversedChunkShape = new int[ndim];
80+
for (int i = 0; i < ndim; i++) {
81+
reversedShape[i] = arrayMetadata.shape[ndim - 1 - i];
82+
reversedChunkShape[i] = arrayMetadata.chunkShape[ndim - 1 - i];
83+
}
84+
return new ArrayMetadata.CoreArrayMetadata(
85+
reversedShape,
86+
reversedChunkShape,
87+
arrayMetadata.dataType,
88+
arrayMetadata.parsedFillValue
89+
);
90+
}
91+
92+
@Override
93+
public long computeEncodedSize(long inputByteLength,
94+
ArrayMetadata.CoreArrayMetadata arrayMetadata) {
95+
return inputByteLength;
96+
}
97+
98+
@Override
99+
public Codec evolveFromCoreArrayMetadata(ArrayMetadata.CoreArrayMetadata arrayMetadata) {
100+
return this;
101+
}
102+
}

‎src/test/java/dev/zarr/zarrjava/ZarrPythonTests.java‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -360,4 +360,82 @@ public void testGroupReadWriteV3() throws Exception {
360360
Assertions.assertArrayEquals(new int[]{16, 16, 16}, result.getShape());
361361
assertIsTestdata(result, dataType);
362362
}
363+
364+
/**
365+
* The Zarr v2 {@code order} cases, as (shape, chunks, order).
366+
*/
367+
static Stream<Object[]> orderProviderV2() {
368+
return Stream.of(
369+
new Object[]{"3,4", "3,4", "C"},
370+
new Object[]{"3,4", "3,4", "F"},
371+
new Object[]{"2,3,4", "2,3,4", "F"},
372+
// Chunks divide neither dimension evenly, so the trailing chunks are partial.
373+
new Object[]{"5,7", "2,3", "F"},
374+
new Object[]{"5,7,3", "2,3,2", "F"},
375+
// For rank 1 both orders describe the same layout.
376+
new Object[]{"10", "4", "F"}
377+
);
378+
}
379+
380+
/**
381+
* Live counterpart to {@link ZarrV2OrderTest}, which covers the same ground offline against
382+
* committed fixtures. This runs against the installed zarr-python so that a change in the
383+
* reference implementation is noticed rather than silently diverging from the fixtures.
384+
*/
385+
@ParameterizedTest
386+
@MethodSource("orderProviderV2")
387+
public void testReadOrderV2(String shape, String chunks, String order) throws Exception {
388+
StoreHandle storeHandle = new FilesystemStore(TESTOUTPUT)
389+
.resolve("testReadOrderV2", shape + "_" + chunks + "_" + order);
390+
run_python_script("zarr_python_order_v2.py", "write", storeHandle.toPath().toString(),
391+
shape, chunks, order);
392+
393+
dev.zarr.zarrjava.v2.Array array = dev.zarr.zarrjava.v2.Array.open(storeHandle);
394+
ucar.ma2.Array result = array.read();
395+
396+
Assertions.assertEquals(dev.zarr.zarrjava.v2.Order.valueOf(order), array.metadata().order);
397+
for (int i = 0; i < result.getSize(); i++) {
398+
Assertions.assertEquals(i, result.getInt(i),
399+
"element " + i + " of a " + order + "-order array");
400+
}
401+
}
402+
403+
/**
404+
* The write direction, verified by zarr-python. zarr-java used to write {@code "order": "F"}
405+
* into {@code .zarray} while laying the chunk out row-major, which its own reader then read back
406+
* correctly &mdash; so only an external reader can catch it.
407+
*/
408+
@ParameterizedTest
409+
@MethodSource("orderProviderV2")
410+
public void testWriteOrderV2(String shape, String chunks, String order) throws Exception {
411+
StoreHandle storeHandle = new FilesystemStore(TESTOUTPUT)
412+
.resolve("testWriteOrderV2", shape + "_" + chunks + "_" + order);
413+
414+
long[] shapeArray = Arrays.stream(shape.split(",")).mapToLong(Long::parseLong).toArray();
415+
int[] chunksArray = Arrays.stream(chunks.split(",")).mapToInt(Integer::parseInt).toArray();
416+
417+
dev.zarr.zarrjava.v2.Array array = dev.zarr.zarrjava.v2.Array.create(
418+
storeHandle,
419+
dev.zarr.zarrjava.v2.Array.metadataBuilder()
420+
.withShape(shapeArray)
421+
.withChunks(chunksArray)
422+
.withDataType(dev.zarr.zarrjava.v2.DataType.INT32)
423+
.withOrder(dev.zarr.zarrjava.v2.Order.valueOf(order))
424+
.withFillValue(0)
425+
.build()
426+
);
427+
428+
int[] intShape = new int[shapeArray.length];
429+
for (int i = 0; i < shapeArray.length; i++) {
430+
intShape[i] = (int) shapeArray[i];
431+
}
432+
ucar.ma2.Array data = ucar.ma2.Array.factory(ucar.ma2.DataType.INT, intShape);
433+
for (int i = 0; i < data.getSize(); i++) {
434+
data.setInt(i, i);
435+
}
436+
array.write(new long[shapeArray.length], data);
437+
438+
run_python_script("zarr_python_order_v2.py", "read", storeHandle.toPath().toString(),
439+
shape, chunks, order);
440+
}
363441
}

0 commit comments

Comments
 (0)