Add support for global CALC nodes #23
Conversation
|
|
||
| class GlobalCalc(PyDoughCollectionAST): | ||
| """ | ||
| The AST node implementation class representing a top-level CALC expression |
There was a problem hiding this comment.
Not saying there is any need to change this, but could there ever be a situation where the "global context" is really graph-level context in the future, meaning tied to a particular graph but not any node in the graph, for when multiple graphs are supported?
If there is then maybe GraphLevelCalc which could have all the same properties is a more accurate description.
There was a problem hiding this comment.
Yeah I just overhauled this by making a "global context" which table collections & calcs can be children of (the necessity of this became quite apparent when I started doing the tree strings), eliminating the need for this class. I'm closing this PR since it is now redundant.
| self._graph: GraphMetadata = graph | ||
| self._children: List[PyDoughCollectionAST] = children | ||
| # Not defined until with_terms is called | ||
| self._calc_term_indices: Dict[str, Tuple[int, PyDoughExpressionAST]] | None = ( |
There was a problem hiding this comment.
What's the reason for having an optional type as opposed to an empty dictionary? Is this because the dictionary is effectively "frozen" once initialized so this is avoid any invalid behavior?
There was a problem hiding this comment.
Explained in the next section. This seems fine.
| self._calc_term_indices: Dict[str, Tuple[int, PyDoughExpressionAST]] | None = ( | ||
| None | ||
| ) | ||
| self._all_terms: Dict[str, PyDoughAST] = None |
There was a problem hiding this comment.
Type annotation needs optional
| ) | ||
| self._all_terms: Dict[str, PyDoughAST] = None | ||
|
|
||
| def with_terms(self, terms: List[Tuple[str, PyDoughExpressionAST]]) -> "GlobalCalc": |
There was a problem hiding this comment.
I'm assuming this is necessary because terms can't be assigned until a later time. This is a less desirable design pattern because it makes it harder to track when the state is valid, but it's an understandable limitation.
Can you write the docstring now though so we can be very clear about the explained delay between with_terms and the constructor?
|
Rendered obsolete by changes to this PR: #20 |
Adds a variant of the CALC node that operates at a graph-level, used for no-groupby aggregation that just computes a single record of data (e.g. the "subcollections" in the child list are table collections).