`group-by-column->indexes` returns something that uses some kind of different key lookup equality? about tech.ml.dataset

[cnuernber]

It does - due to previous bugs it uses a linked hash map.

[harold]

Which bugs? Why is it better this way?

[cnuernber]

Specifically you use a linked hash map when you care about order of insertion or modification there was an issue around group-by-column I think but I can't find it. There is an expectation among people coming from R that the order of the returned map matches the order the data was found in the original column.

We don't respect this for group-by-column-agg as there we use the concurrent hashmap but for the non-massive cases of group-by I tried to respect this ordering.

[harold]

oic, that's cool and makes sense - I wonder if it would be possible 🍰 to have cake and also eat it by using a Clojure sorted-map? --- this situation was very confusing yesterday because CSV import helpfully made a column integers (nice, saves ram!) but then after group-by-column->indexes another source of the same numbers must have been Longs, and so every lookup was returning nil.

[cnuernber]

Linked is not same as sorted - linked is insertion order so order of the column data.

The map itself should have had longs in it regardless - are you saying the keys in the group-by-column->indexes were integers? Regardless that sounds like a lot of pain. We should file a small repro of that because it is annoying. I am working on a linked hash map in hamf to address this which will use the normal equiv pathways.

Unequivically it is fairly wrong but it is extremely wrong if the map keys aren't longs.

[cnuernber]

The big change is in hamf

[harold]

Linked is not same as sorted - linked is insertion order so order of the column data.

Got it! I was thinking of it wrong.

are you saying the keys in the group-by-column->indexes were integers?

Ah, no, sorry for the confusion, the keys in the lookup map (result of group-by-column->indexes) were longs, but the column values they were derived from were int32.

So, when the ints in the column were used to lookup from the map key'd by longs, it failed.

We should file a small repro of that because it is annoying.

Here's a minimal case - the real case, in the annealer, was more delicate and convoluted, as you can imagine

However, this demonstrates the unexpected nil.

repl'ing this stuff is 🎲 actually dicey, because Integers print as things that read as Longs - so it's very easy to get confused.

user> (require '[tech.v3.dataset :as ds])
nil
user> (spit "tmp.csv" "a\n111111\n2222222\n333333\n")
nil
user> (def ds (ds/->dataset "tmp.csv"))
#'user/ds
user> (get ds "a")
#tech.v3.dataset.column<int32>[3]
a
[111111, 2222222, 333333]
user> (def lookup (ds/group-by-column->indexes ds "a"))
#'user/lookup
user> (def v (first (int-array (get ds "a"))))
#'user/v
user> v
111111
user> (get lookup v)
nil
user> (get lookup 111111)
[0]

[cnuernber]

• https://github.com/cnuernber/dtype-next/blob/master/test/tech/v3/datatype_test.clj#L855
• https://github.com/techascent/tech.ml.dataset/blob/master/test/tech/v3/dataset_test.clj#L1719

The bigger question to me is when will we find the first subtle issues with the linkedhashmap implementation...

[harold]

when will we find the first subtle issues with the linkedhashmap implementation...

With a sufficently royal interpretation of the 'we' in this question, the answer becomes "in due time" (:

New tests look cool and relevant, I look forward to upgrading and taking out some of the hacks (into {} ...) I've put in place.