brimdata / super

A novel data lake based on super-structured data
https://zed.brimdata.io/
BSD 3-Clause "New" or "Revised" License
1.39k stars 64 forks source link

ZQL "c=count()" syntax no longer works #593

Closed philrz closed 4 years ago

philrz commented 4 years ago

Start with the following input data:

cat bug.zng
#0:record[a:int]
0:[1;]
0:[2;]

As of zq commit 1759030, the following syntax was working:

# zq "c=count()" bug.zng
#0:record[c:count]
0:[2;]

Then since the following commit 91e062c ("Support field expressions in reducers (#260)" from @mattnibs) it fails to parse.

# zq "c=count()" bug.zng
parse error: 1:10 (9): rule reducerExpr: interface conversion: interface {} is *ast.FieldRead, not string

Note that the following alternate syntax has always worked (though to run on current master the input data would need to use a different type such as int64):

# zq "count() as c" bug.zng
#0:record[c:count]
0:[2;]

@mccanne cited this regression as unintentional, so I'm opening this as a bug.

FWIW, with similar language topics in the past, we've sometimes debated whether we should have multiple syntaxes to achieve the same thing or standardize on just one. We might want to have that debate as we're addressing this, or just address it so both work as they used and move forward.

henridf commented 4 years ago

Offline decision: we'll add this c=count() syntax, and remove the count() as c syntax.

philrz commented 4 years ago

Verified in zq commit 8cdcf4b.

Updating the input data to be in sync with current data types, we're back to the previous syntax that everyone seems to prefer:

$ cat bug.zng
#0:record[a:int64]
0:[1;]
0:[2;]

$ zq -t "c=count()" bug.zng
#0:record[c:uint64]
0:[2;]

The as syntax is retired. Attempts to use it now generate an error message.

$ zq -t "count() as c" bug.zng
parse error: 1:9 (8): no match found, expected: " ", ",", "-limit", "\f", "\t", "\u00a0", "\ufeff", "\v", "by"i, "|" or EOF

The same is true in Brim app commit 137766e talking to zqd commit 8cdcf4b.

image

image

(This is unrelated to this specific issue, but as long as I'm pasting outputs I'll make an observation: The error message in the app is more helpful than the one on the command-line.)

Thanks @henridf!