-
Notifications
You must be signed in to change notification settings - Fork 0
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
Creating the unqualified PyDough module #43
Conversation
), | ||
pytest.param( | ||
"answer = ROOT.Regions(region_name=ROOT.name, region_key=ROOT.key)", | ||
"TPCH.Regions(region_name=TPCH.name, region_key=TPCH.key)", |
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.
When the qualification process is done, TPCH.name
will be replaced with the .name
property from the current context (TPCH.Regions
). It is done this way since .name
could have been accessed on another line then placed into this context.
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! Thanks @knassre-bodo
|
||
class UnqualifiedOperator: | ||
""" | ||
Class for |
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.
Finish this docstring or add a TODO
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.
whoops
), | ||
pytest.param( | ||
"answer = (1 / _ROOT.x) ** 2 - _ROOT.y", | ||
"(((1:Int64Type() / TPCH.x) ** 2:Int64Type()) - TPCH.y)", |
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.
Question: Is there any reason literals are binding to int64 and not int8 here? Not saying its important at this time, but I want to understand if this is future work post demo that needs a ticket.
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.
Just a default I'm using (see coerce_to_unqualified
). We can refine later if need be. Which integer type is being used doesn't really matter at the moment since types are basically no-ops except for the fact that we expect them to be everywhere.
""" | ||
root: UnqualifiedNode = UnqualifiedRoot(get_sample_graph("TPCH")) | ||
env = {"_ROOT": root, "BACK": UnqualifiedBack, "datetime": datetime} | ||
exec(pydough_str, global_ctx, env) |
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.
So the idea here is that global_ctx defines the namespace? That seems reasonable for any testing.
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.
It's what I'm passing around instead of globals()
to exec
, partly so I don't have to mess around with # noqa
return obj | ||
if isinstance(obj, bool): | ||
return UnqualifiedLiteral(obj, BooleanType()) | ||
if isinstance(obj, int): |
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 don't think we need any changes in this PR, but it might be good to have a followup TODO to see if we want to include numpy literal types.
Initializing the module for PyDough unqualified nodes. These nodes are devoid of meaning until they are qualified, transforming them into PyDough AST nodes. Until then, most of them will be derived from a "root" node associated with the graph, and during the qualification process the nodes will be qualified by either verifying that the accessed terms are accessible from the root or that they have been placed in a context where they do exist.