`collect` drops `error("missing")`, hangs sometimes on `null`

[chrismo]

❯ super -z -c "[1, error('missing'),'a'] | over this | collect(this)"       
[1,"a"]

Actually, adding null to this input array on the CLI just hangs, and I can't even Ctrl-C out, I had to kill the process.

❯ super -z -c "[1, error('missing'),'a',null] | over this | collect(this)"       

Some instances with null are ok

❯ super -z -c "[null] | over this | collect(this)"      
null

❯ super -z -c "[null,1] | over this | collect(this)"      
[1]

# this one hangs:
❯ super -z -c "[null,1,'a'] | over this | collect(this)"      

The hang obviously feels like a bug, but I'm not sure what the expected behavior is with collect and error("missing") and null

[philrz]

Thanks @chrismo. Yes, it definitely seems like there's plenty for us to look into here.

FWIW, the way it shows collect dropping the null is a known thing and we've done some work at addressing it as described in #5039 and #5041. There actually was recent discussion about reviving that effort, so this is timely.

I think the hang is a new finding though, so that'll definitely also be discussed with the team.

The drop of the error("missing") also feels new. If I was taking a stab at what I might have expected instead, I'd have thought it would have been preserved with the option to easily drop it via the quiet function if desired. Modifying your example to play around with what that might look like in practice:

$ super -version
Version: v1.18.0-359-g70c4cc66

$ super -z -c "[1, error('missing'),'a'] | over this | collect(quiet(this))"
[1,error("quiet"),"a"]

And that confuses me now since the docs page for quiet says:

Quiet errors are ignored by operators aggregate, cut, and yield.

I'll definitely discuss internally and provide an update.

[mattnibs]

The hang is fixed here: #5766

Suggesting we close this issue since we already have another issue for nulls in collect.

[philrz]

I've verified the hang is fixed in super commit 7f5ca96.

Repeating the original repro command line that previously caused a hang, now we see:

$ super -version
Version: v1.18.0-366-g7f5ca96c0

$ super -z -c "[1, error('missing'),'a',null] | over this | collect(this)"
[1,"a"]

Per @mattnibs's point above, indeed, we already have open issue #5039 to cover the dropped null. That leaves the topic of the dropped error("missing"). Since that has a similar flavor to the dropped null, I went ahead and grafted it onto that same issue (#5039 (comment)) in the event we can address both at the same time. If it needs its own separate issue, I'd be happy to break it out, though.

Thanks @mattnibs!