Default implementation of column.IterableOrderedMap

[earwin]

A lot of people are going to reinvent this, and way fewer people are going to have special cases where a custom impl will make any practical sense. So I suggest providing one out of the box.

As an example, here's the implementation I'm currently using: https://gist.github.com/earwin/149740749c8aaccde5ea626169c32735

It has some go1.23 iter.Seq usage, but that's non-core functionality and can be dropped.

[earwin]

Anyone?
I can do a pull request, no problem, if this is something the library needs in maintainers' opinion.

[jkaflik]

Hi @earwin

Contribution from your side would be much appreciated. Thanks!

[earwin]

@jkaflik Cool! I'll make it happen.

I have a (non-blocking, related) question regarding the API though:

Why are we inventing our own iterator interface at all, when go1.23 already has a great one?
What's more, even if range construct over new iterators is go1.23+ only, that doesn't preclude you from using the API in earlier versions.
What if we swap:

type MapIterator interface {
    Next() bool
    Key() any
    Value() any
}

for:

type MapIterator func(yield func(k any, v any) bool)

which in 1.23 has a convenient alias:

iter.Seq2[any,any]

This will make us future-compatible with all 1.23+ iterator goodness.
The current callsite for IterableOrderedMap would change from:

	if orderedMap, ok := v.(IterableOrderedMap); ok {
		var size int64
		iter := orderedMap.Iterator()
		for iter.Next() {
			key, value := iter.Key(), iter.Value()
			size++
			if err := col.keys.AppendRow(key); err != nil {
				return err
			}
			if err := col.values.AppendRow(value); err != nil {
				return err
			}
		}
		var prev int64
		if n := col.offsets.Rows(); n != 0 {
			prev = col.offsets.col.Row(n - 1)
		}
		col.offsets.col.Append(prev + size)
		return nil
	}

to (or thereabouts):

	if orderedMap, ok := v.(func(yield func(k any, v any) bool)); ok {
		var size int64
		var err error
		orderedMap(func(key any, value any) bool {
			size++
			if err = col.keys.AppendRow(key); err != nil {
				return false
			}
			if err = col.values.AppendRow(value); err != nil {
				return false
			}
			return true
		})
		if err != nil {
			return err
		}

		var prev int64
		if n := col.offsets.Rows(); n != 0 {
			prev = col.offsets.col.Row(n - 1)
		}
		col.offsets.col.Append(prev + size)
		return nil
	}

and in 1.23 to:

	if orderedMap, ok := v.(func(yield func(k any, v any) bool)); ok {
		var size int64
		
		for key, value := range orderedMap{
			size++
			if err := col.keys.AppendRow(key); err != nil {
				return err
			}
			if err := col.values.AppendRow(value); err != nil {
				return err
			}
		}
		
		var prev int64
		if n := col.offsets.Rows(); n != 0 {
			prev = col.offsets.col.Row(n - 1)
		}
		col.offsets.col.Append(prev + size)
		return nil
	}

You can't, unfortunately, cast things like iter.Seq2[string,string] to iter.Seq2[any,any], but a trivial adapter function is possible.

[earwin]

Another API question is, why do we have single interface for both read and write sides?

type IterableOrderedMap interface {
	Put(key any, value any)
	Iterator() MapIterator
}

If we split it, we can throw that Iterator() MapIterator thing away completely, and accept standard golang iterators (for Arrays as well!) on the write side.

[jkaflik]

Sorry for not having a look into this. We can have go stdlib type support and deprecate MapIterator without removing it. Sounds good.

[earwin]

A related, but independent question:

@jkaflik when the user passes a map[K]V for a clickhouse Map(K,V) column... is there any merit whatsoever for the current behaviour of writing out the entries in random order?
I'm beginning to think we should just bite the bullet and sort always (unless the user explicitly provides the keys in order of their choosing by passing an instance of IOM). It follows the principle of least astonishment.
This is not going to be a breaking change, since the client never guaranteed any order before, including random order.

[jkaflik]

@earwin

is there any merit whatsoever for the current behaviour of writing out the entries in random order?

The answer is no. IMO it's a bug. Can you please fill in issue with details?

[earwin]

I'll have a look at implementing this. Going to be a teeny bit tricky, since not all acceptable input types can be ordered. They only become orderable after conversion to CH types.