[BSE-4155] Add support for the Aggregate relational node #39
Conversation
pydough/relational/aggregate.py
Outdated
| self._aggregations: MutableMapping[str, CallExpression] = aggregations | ||
| assert all( | ||
| agg.is_aggregation for agg in aggregations.values() | ||
| ), "All functions used in aggregations must be aggregation functions" |
There was a problem hiding this comment.
Can avoid this if we have a special version of CallExpression just for agg calls (its inputs are only allowed to be literals or column references, nothing else) -> use type annotations to verify that the aggregations argument maps to this special variant.
They can then check the operator passed in to make sure that its .is_aggregation field is true.
There was a problem hiding this comment.
Let's consider this as a followup. I think the extra class isn't worth the safety.
There was a problem hiding this comment.
Also I'm not supporting literals yet. I still need to figure that out.
| @@ -30,7 +30,7 @@ def inputs(self): | |||
| def input(self) -> Relational: | |||
| return self._input | |||
|
|
|||
| def node_equals(self, other: "Relational") -> bool: | |||
| def node_equals(self, other: Relational) -> bool: | |||
There was a problem hiding this comment.
Why is this still equals in some places and node_equals in other? Can we switch this to all/nothing?
There was a problem hiding this comment.
Everything is just node_equals now for Relational nodes.
Adds support for the basic definition of the Aggregate relational node.