generic expression aggregation check all children are -> any child is

[CircArgs]

Summary

changes the generic logic checking if an expression is an aggregation to just checking if there is any child which aggregates rather than all children aggregating

this would be a problem because of the binop default is_aggregation:

curl -X POST http://localhost:8000/nodes/metric/ \
-H 'Content-Type: application/json' \
-d '{
    "name": "num_repair_orders_x_2",
    "description": "Number of repair orders",
    "mode": "published",
    "query": "SELECT 2.0 * count(repair_order_id) as num_repair_orders FROM repair_orders"
}'

which would succeed after this change. Now, 2.0 * count(repair_order_id) is identified as an aggregation

Test Plan

• PR has an associated issue: #
• make check passes
• make test shows 100% unit test coverage

[shangyian]

Hmm what about something like:

SELECT count(repair_order_id) + repair_order_id AS num_repair_orders FROM repair_orders

That wouldn't be an aggregation, but it would still pass this check right?

[samredai]

@shangyian is that a valid query? If not then is it reasonable to rely on the underlying query engine to catch that?

[shangyian]

@samredai It's not a valid query, but then we'd be storing an invalid query in a Metric node that's marked as "valid". Not sure if this is an issue though, since arguably we don't have to check for aggregations at all.

[samredai]

but then we'd be storing an invalid query in a Metric node that's marked as "valid"

Hmm, good point...

[CircArgs]

@shangyian I made another change now and reverted the original. This needed to be the case anyways, but now values are aggregations. That should work right? So, rather than 2.0 * count(repair_order_id) being an aggregation because BinaryOp has some child (count) that is an aggregation, all the children including Number(2.0) is naturally an aggregation being a scalar value

[shangyian]

That makes sense to me! Thanks @CircArgs

[agorajek]

SELECT count(repair_order_id) + repair_order_id AS num_repair_orders FROM repair_orders

That wouldn't be an aggregation, but it would still pass this check right?

@shangyian I may be missing your point, but isn't this a completely separate problem? Meaning "whether the query is valid SQL" from "whether any given expression is an aggregate expression" ?

[samredai]

@shangyian I may be missing your point, but isn't this a completely separate problem? Meaning "whether the query is valid SQL" from "whether any given expression is an aggregate expression" ?

@agorajek this was exactly my original thought but I think the update @CircArgs made is an even clearer approach that also solves the problem @shangyian raised. The updated check is more robust in that it gets to the core of the requirement which is that all elements resolve to some scalar value that can co-exist in a single expression.