2024-08-14 malli | Clojure Slack Archive

malli

niwinz 2024-08-14T12:32:11.413269Z

Before opening an issue, I wanted to ask first if this change makes sense. I changed the -transform-map-keys implementation to the following on my own code:

(defn -transform-map-keys
  [f]
  (fn [o]
    (if (map? o)
      (reduce-kv (fn [res k v]
                   (let [k' (f k)]
                     (if (= k k')
                       res
                       (-> res
                           (assoc k' v)
                           (dissoc k)))))
                 o
                 o)
      o)))

The current implementation in maili, if you pass a record to decode with the key-transformer, it will replace the record with a map thanks to (into {} (map (.... Changing the impl to use reduce-kv preserves the type. Does it make sense that there would be an issue or even a PR?

niwinz 2024-08-14T12:37:11.268659Z

O maybe a way to pass a custom function to key-transformer and leave the current behavior as default but give the ability to replace it

ikitommi 2024-08-14T14:24:27.622659Z

I consider it as a bug that Records are flattened to maps with the key-transformers.

ikitommi 2024-08-14T14:27:08.350819Z

PR welcome, but I think the reduce-kv is slower than the current transducer. Maybe have special handling for record?, e.g.

(cond
  (record?) reduce-kv
  (map?) transducer
  :else default)

ikitommi 2024-08-14T14:28:19.472599Z

better to run perf-tests on these to make sure how they perform

ikitommi 2024-08-14T14:43:38.502459Z

it seems the reduce-kv is slower with plain maps, current:

Evaluation count : 3503592 in 6 samples of 583932 calls.
             Execution time mean : 168,090682 ns
    Execution time std-deviation : 0,712479 ns
   Execution time lower quantile : 167,320484 ns ( 2,5%)
   Execution time upper quantile : 169,110612 ns (97,5%)
                   Overhead used : 1,801948 ns

new:

Evaluation count : 2250630 in 6 samples of 375105 calls.
             Execution time mean : 273,808160 ns
    Execution time std-deviation : 6,733026 ns
   Execution time lower quantile : 266,023063 ns ( 2,5%)
   Execution time upper quantile : 282,614571 ns (97,5%)
                   Overhead used : 1,801948 ns

ikitommi 2024-08-14T14:44:16.286249Z

so, this should be just for record? s. PR welsome!

ikitommi 2024-08-14T14:46:28.770579Z

if one would not touch the keys, your approach would be much faster, as it would not assoc-dissoc anything, current:

Execution time mean : 109,704440 ns
    Execution time std-deviation : 0,840514 ns
   Execution time lower quantile : 108,709907 ns ( 2,5%)
   Execution time upper quantile : 110,634955 ns (97,5%)
                   Overhead used : 1,801948 ns

new:

Execution time mean : 8,723063 ns
    Execution time std-deviation : 0,501256 ns
   Execution time lower quantile : 8,447378 ns ( 2,5%)
   Execution time upper quantile : 9,581723 ns (97,5%)
                   Overhead used : 1,801948 ns

ikitommi 2024-08-14T14:47:04.143469Z

… but don’t think that is the common case (of not transforming the keys, e.g. running identity on keys).

niwinz 2024-08-14T15:01:22.830389Z

I finally ended up with a specific impl for record and for map

niwinz 2024-08-14T15:01:47.242189Z

I will do a PR this days

👍 1

Ben Sless 2024-08-14T15:24:38.270949Z

Did you test with a transient map?

niwinz 2024-08-14T15:25:48.670709Z

I have used transient map on my code, so I try to test it before PR.

Clojurians Log v2

malli