zed icon indicating copy to clipboard operation
zed copied to clipboard

bug passing null values to CosumeAsPartial

Open mccanne opened this issue 4 years ago • 2 comments

The collect aggregator is receiving null values for some reason in ConsumeAsPartial().

Let's investigate this and either eliminate the call or document it better.

Recreate with

collect(x) by key with -limit 1 in.zson

where in.zson is

      {key:"a",x:1(int32)}
      {key:"a",x:-1(int32)}
      {key:"b",x:2(int32)}
      {key:"b",x:1(int32)}
      {key:"a",x:8(int32)}
      {key:"b",x:1(int32)}
      {key:"a"}
      {key:"a"}
      {key:"a"}

mccanne avatar Oct 12 '21 18:10 mccanne

When filed, this issue lacked some detail such as repro commit or saying if there was something specific about the output considered incorrect. In an attempt to recreate what's missing, here's a repro using Zed commit 6ec94b9 from Oct 11, 2021, i.e., the day before this issue was filed.

$ cat in.zson 
{key:"a",x:1(int32)}
{key:"a",x:-1(int32)}
{key:"b",x:2(int32)}
{key:"b",x:1(int32)}
{key:"a",x:8(int32)}
{key:"b",x:1(int32)}
{key:"a"}
{key:"a"}
{key:"a"}

$ zq -version
Version: v0.31.0-7-g6ec94b9f

$ zq -z 'collect(x) by key with -limit 1' in.zson
{key:"a",collect:[1(int32),-1(int32),8(int32)]}
{key:"b",collect:[2(int32),1(int32),1(int32)]}

And now here's the same query with current tip of main, commit 26dbda0, which produces exactly the same output. This indicates the original issue is still with us, or that the issue was originally pointing out something deeper in the code that's invisible in a user-facing repro like this. I can't speak for the latter.

$ zq -version
Version: v1.2.0-20-g26dbda03

$ zq -z 'collect(x) by key with -limit 1' in.zson
{key:"a",collect:[1(int32),-1(int32),8(int32)]}
{key:"b",collect:[2(int32),1(int32),1(int32)]}

On the assumption it's the former, when reviewing this with @mccanne and @nwt, a collective likely guess as to the issue's original intent was to point out that when the aggregation processed the {key:"a"} records those should have produced error("missing") in the output that could have been silenced with quiet().

philrz avatar Jul 27 '22 17:07 philrz

I showed the findings in the last comment to @mccanne and he wasn't sure what to make of it. I'll keep this one on hold until he has some time to look closer & check the code.

philrz avatar Aug 24 '22 20:08 philrz

This issue is about the following comment. I've confirmed that it's still relevant.

https://github.com/brimdata/zed/blob/32098eef37e121b96e62abc24d2e2ec8152457a0/runtime/expr/agg/collect.go#L71-L75

nwt avatar Feb 07 '23 20:02 nwt