feat: add plan builders #73
Conversation
tokoko
commented
Apr 16, 2025
•
tokoko
commented
- add plan builders for read, project, filter, etc
- add simple builders for types
tokoko
changed the title
src/substrait/builders/plan.py
Outdated
|
|
||
| return resolve | ||
|
|
||
| def fetch(plan: stp.Plan, |
There was a problem hiding this comment.
So this takes a plan (not a relation) and wraps it in a Fetch relation and returns a new plan? Is that worth adding a comment?
There was a problem hiding this comment.
yes, this is to allow passing along information about extensions and CTEs. Added a module-level comment.
src/substrait/builders/plan.py
Outdated
| return resolve | ||
|
|
||
| def cross( | ||
| left: Union[stp.Plan, UnboundPlan], |
There was a problem hiding this comment.
Why does this take either a Plan or an UnboundPlan where other relations don't? Is it worth adding a type definition for this Union so that all uses are kept in sync?
There was a problem hiding this comment.
all of them should take in Union[stp.Plan, UnboundPlan], I thought I made the changes everywhere, I'll fix it. type definition sounds like a good idea.
src/substrait/builders/type.py
Outdated
| def map(key: stt.Type, value: stt.Type, nullable=True): | ||
| return stt.Type(map=stt.Type.Map(key=key, value=value, nullability=stt.Type.NULLABILITY_NULLABLE if nullable else stt.Type.NULLABILITY_REQUIRED)) | ||
|
|
||
| def named_struct(names: Iterable[str], struct: stt.Type): |
There was a problem hiding this comment.
This doesn't return an stt.Type so it probably doesn't belong in this file. If you want to keep it here it needs a comment so that users don't mistakenly think that it is the same as others in this file.
There was a problem hiding this comment.
I thought I could get away with it since NamedStruct is also defined in type_pb2. Do you think a return type annotation should be enough to show it's different?
