Merge pull request #18 from stevemkim/issue-17

Fix issue #17: bytes-to-array makes a copy of byte buffer

project.clj

@@ -1,4 +1,4 @@
-(defproject io.mandoline/mandoline-core "0.1.8"
+(defproject io.mandoline/mandoline-core "0.1.9"
   :description
     "Mandoline is a distributed store for multi-dimensional arrays"
   :license {:name "Apache License, version 2.0"

src/io/mandoline/impl.clj

@@ -182,11 +182,30 @@
            :index
            (wrappers-fun (proto/index connection var-name mdata options) res))))
 
-(defn- ^ucar.ma2.Array bytes-to-array
+(defn ^ucar.ma2.Array bytes-to-array
   "Coerces a byte-buffer to a new Array with given data type and shape."
   [^ByteBuffer byte-buffer ^DataType dtype shape]
-  ;; todo do we have unnecessary copying here?
-  (Array/factory dtype (int-array shape) byte-buffer))
+  (locking byte-buffer
+    (let [p (.position byte-buffer)
+          r (.remaining byte-buffer)
+          ; If the ByteBuffer is not perfectly aligned with its
+          ; underlying byte[] storage, make a copy. This copy defends
+          ; against a bug in the ucar.ma2.Array/factory method, which
+          ; does not honor the position and limit of the ByteBuffer and
+          ; reads out-of-range bytes in the underlying byte[] when the
+          ; data type is DataType/BYTE.
+          ; TODO: Fix the bug in upstream ucar netCDF library so that we
+          ; don't need this copy overhead.
+          bb (if-not (and (= 0 (.arrayOffset byte-buffer) p)
+                          (= (.capacity byte-buffer) (.limit byte-buffer) r))
+               (let [dst (byte-array r)]
+                 (.get byte-buffer dst)
+                 (ByteBuffer/wrap dst))
+               byte-buffer)
+          array (Array/factory dtype (int-array shape) bb)]
+      ; Resume original position
+      (.position byte-buffer p)
+      array)))
 
 (defn hash->slab [hash chunk-store dtype slice]
   (assert hash)

test/io/mandoline/impl_test.clj

@@ -1,11 +1,15 @@
-(ns io.mandoline.impl-test (:require
+(ns io.mandoline.impl-test
+  (:require
     [clojure.test :refer :all]
     [io.mandoline
      [chunk :as chunk]
      [impl :as impl]
      [slab :as slab]
      [slice :as slice]]
-    [io.mandoline.impl.protocol :as proto]))
+    [io.mandoline.impl.protocol :as proto])
+  (:import
+    [java.nio ByteBuffer]
+    [ucar.ma2 Array DataType]))
 
 
 ;; Verify that when writing a variable, we don't repeatedly write an identical
@@ -33,3 +37,34 @@
       ;; Even though we were writing thousands of chunks, they were all the same,
       ;; and hopefully we didn't write that one chunk too many times.
       (is (< 0 @chunks-written 100)))))
+
+(deftest test-bytes-to-array
+  (let [elements (map byte (range 30))
+        ; Create a ByteBuffer instance that is backed by a 25-element
+        ; byte array, with position=3 and limit=19. Diagram:
+        ;
+        ; byte[25] =========================
+        ; slice        =====================
+        ; limit        ===================
+        ; position        ================
+        byte-buffer (-> (byte-array elements)
+                      (ByteBuffer/wrap)
+                      (.position 4) ; advance position by 4
+                      (.slice) ; create a slice that shares the underlying byte[]
+                      (.limit 19) ; trim to 19 long
+                      (.position 3)) ; advance position by 3
+        data-type DataType/BYTE
+        shape [2 4 2]
+        array (impl/bytes-to-array byte-buffer data-type shape)]
+    (is (instance? Array array)
+        "bytes-to-array returns an instance of ucar.ma2.Array")
+    (is (= (.getClassType data-type) (.getElementType array))
+        "bytes-to-array returns an Array with the expected element type")
+    (is (= shape (vec (.getShape array)))
+        "bytes-to-array returns an Array with the expected shape")
+    (is (= (take (- 19 3) (drop (+ 4 3) elements)) (seq (.copyTo1DJavaArray array)))
+        "bytes-to-array returns an Array that contains the expected elements")
+    (is (= 3 (.position byte-buffer))
+        "bytes-to-array does not change the position of the original ByteBuffer")
+    (is (= 19 (.limit byte-buffer))
+        "bytes-to-array does not change the limit of the original ByteBuffer")))