-
Notifications
You must be signed in to change notification settings - Fork 8
ENH: lift_subqueries scheduler pass
#7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
src/finch/finch_logic/nodes.py
Outdated
| def children(self): | ||
| """Returns the children of the node.""" | ||
| return [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Finch.jl arguments(node) can be also called on non-tree IR instances. I think this should be implemented the same way here: Return empty list by default and is_expr=True IRs override it returning actual children.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have mixed thoughts on this. Implementing children(node) for literal feels dangerous because x.make_term(x.head(), x.children()) is not equal to x. What do you think Mateusz? @wraith1995 do you have thoughts on this one? Maybe as long as we don't also implement make_term for this IR node type it's okay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also hesitant about this, so I followed Finch.jl:
julia> using Finch
julia> sq = Finch.subquery("lhs", "arg")
Finch Logic: (lhs = arg)
julia> imm = Finch.immediate("imm")
Finch Logic: imm
julia> Finch.arguments(sq)
2-element Vector{Finch.FinchLogic.LogicNode}:
Finch Logic: lhs
Finch Logic: arg
julia> Finch.arguments(imm)
Finch.FinchLogic.LogicNode[]There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I would prefer to have it as:
def children(self):
"""Returns the children of the node."""
raise Exception(f"`children` isn't supported for {self.__class__}")as a base implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have two thoughts:
- Perhaps more thought should be given to how the language interacts with things outside of it - in an ideal world, nothing outside the IR except for constants would be in the IR or some process would remove such references and replace them with something else. Perhaps, this would fix the problem of
make_term? If you have tensors in the expressions, perhaps they should be pre-processed out before anything else happens. - I generally support that IR instances should have a uniform set of methods so I don't think either should not implement anything. But perhaps returning [] is a bad idea - a pythonic way to do this would be to return None and handle the consequences downstream. Alternatively, you could have make_term fail on expressions by raising an exception. Either way, it requires code in various places to catch this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I should add I believe (2) in part because I would want to move these methods to other places via just one big case statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with 1, and I think that from the point of the view of the IR, everything that isn't is_expr is basically different kinds of constants. Mateusz, let's go forward with your Error-related approach for now. I think Teo's return-none approach is similar to if x.is_expr() x.children else None, which would need an if-branch anyway. We'll use Mateusz' approach for now since we're still translating from Finch, but I don't think that changing it to Teo's approach later would be a big issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I made it an Exception then!
|
|
||
| tns: LogicNode | ||
| idxs: Iterable[LogicNode] | ||
| idxs: tuple[LogicNode] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use tuple explicitly. We expect these LogicNodes to be hashable, and non-hashable structures like list are also Iterable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call!
|
|
||
| lhs: LogicNode | ||
| rhs: LogicNode | ||
| arg: LogicNode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed it to follow Finch.jl subquery node.
willow-ahrens
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This PR:
lift_subqueriesdefault scheduler passpre-commitsetupruffrules