Skip to content
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

feat: clarify the meaning of plans #616

Merged
merged 2 commits into from
Aug 1, 2024

Conversation

westonpace
Copy link
Member

#612 and #613 highlighted a number of ambiguities in plan interpretation. I have made an attempt to clarify these based on my interpretation.

EpsilonPrime
EpsilonPrime previously approved these changes Mar 28, 2024
Copy link
Contributor

@ingomueller-net ingomueller-net left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very helpful -- thank you, @westonpace!

I think the new text makes a good job in distinguishing root and non-root relations and what their purposes are. The only comment I have is that the current terminology is not in line with the field names in the protobuf definitions: there we have one "plan" (i.e., one Plan message) that contains several (root or non-root) "relations" (i.e., several PlanRel messages), whereas here we have several "plans" that contain "a tree of relations" (which could be read as "one tree of relations").

How about aligning the terminology with the one from protobuf? Something like "Substrait is designed to allow users to express [...] transformations. [...] Transformations are expressed in a 'plan' that consists of several 'relations'. Each relation is a tree of 'relational operators'. We distinguish between 'root relations', which are interpreted as the possible result(s) of the plan depending on the context, and 'non-root relations', which represent intermediate results that other relations can refer to but which by themselves are not part of the semantics of the plan. (In particular, they can be removed, if unused.)"

If you agree, I can make the changes, which look quite mechanic, but feel free to do them yourself if you don't want to wait for the round-trip.

site/docs/relations/basics.md Outdated Show resolved Hide resolved
site/docs/relations/basics.md Outdated Show resolved Hide resolved
site/docs/serialization/basics.md Outdated Show resolved Hide resolved
site/docs/serialization/basics.md Outdated Show resolved Hide resolved
site/docs/serialization/basics.md Outdated Show resolved Hide resolved
@jacques-n
Copy link
Contributor

This looks really good. Seems like a few small edits proposed that look good and then we should be able to merge. Ping @westonpace to push over the finishline.

@jacques-n jacques-n added the awaiting-user-input This issue is waiting on further input from users label Aug 1, 2024
Co-authored-by: Ingo Müller <github.com@ingomueller.net>
@westonpace
Copy link
Member Author

Addressed comments. @jacques-n

Copy link
Contributor

@jacques-n jacques-n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good set of clarifications. Going to merge.

@jacques-n jacques-n merged commit c1553df into substrait-io:main Aug 1, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-user-input This issue is waiting on further input from users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants