improve performance of join by make it internal

[wader]

Avoids exponential string append

Before:
BenchmarkJoinShort
BenchmarkJoinShort-4 26947 44399 ns/op
BenchmarkJoinLong
BenchmarkJoinLong-4 145 7295904 ns/op

After:
BenchmarkJoinShort
BenchmarkJoinShort-4 81789 14456 ns/op
BenchmarkJoinLong
BenchmarkJoinLong-4 22002 53791 ns/op

[pkoppstein]

This is just a question: does this version behave properly when given a stream as argument?

$ gojq -nr '["a","b"] | join(",", "\n")'
a,b
a
b

gojq -nr '["a","b"] | join(empty)'

[wader]

Good question, yes should work, internal functions does not know about generators, will only get values. But i'm not sure how it get compiled to make it work in these cases, now i got curious. So i guess a call to a function that has any argument that results in no outputs will itself result in no output? seems resonable and the only way it could be? :)

Tried your examples with the change:

$ go run cmd/gojq/main.go -nr '["a","b"] | join(",", "\n")'
a,b
a
b
$ go run cmd/gojq/main.go -nr '["a","b"] | join(empty)'

[wader]

I think it makes sense to change the error if we do it this way or?

[wader]

Maybe it's possible to have a more generic internal help function for improving performance of reduce + binop builtin functions? like keep map over array in jq and then pass result to a internal function with argument for how it should aggregate?

[itchyny]

You don't need to worry about stream as argument of internal functions because they are processed before called.
Don't break {x:1,y:2} | join(",").

[wader]

The object case got a bit messy. It does the same thing as .[], each in sorted key order which is different from jq.

[itchyny]

This growing complexity is the reason I stopped making it internal. I don't like the duplicates with the executor and pathValue to be used by internal functions. So how about def join($x): [.[]] | _join($x); and making _join internal? Or if type != "array" then [.[]] end | _join($x) if we avoid cloning arrays.

[wader]

I agree, i've revised the implementation to your second suggestion. By avoid cloning the array do you mean implement our own version of strings.Join so that we don't have to copy it?

[wader]

Also i noticed that the benchmarks were all wrong, i accidentally used []string as input so it was just measuring the functions throwing error, I was a bit surprised how fast the current one was with long arrays.

[wader]

This is to allow [1] | join(1), but would be nice to move this check earlier somehow

[wader]

Updated to use own string builder, long when from 57242 to 48389

[wader]

guessed length, should be better?

[itchyny]

By avoid cloning the array do you mean implement our own version of strings.Join so that we don't have to copy it?

No, use strings.Join. I just mean [.[]] | _join($x) clones the input array.

[wader]

Ok switching back to strings.Join

[itchyny]

Thank you.

[wader]

👍 appreciate your comments and suggestions