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: add anchor for non-ordinal plan reference #726

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

drin
Copy link
Member

@drin drin commented Oct 25, 2024

This adds an "anchor" to PlanRel that can be referenced from a ReferenceRel. This anchor and reference relationship provides a non-ordinal method for identifying and accessing a "subtree" or sub-graph. To maintain consistency with type_anchor and function_anchor, the new subtree_anchor attribute is uint32.

This PR leaves in the original subtree_ordinal attribute since it seems a (mildly) more performant method for referencing a subtree, but also since it is still relevant in the typical case. The new anchor attribute is an improvement for cases where multiple plans are merged and at least one already contains a ReferenceRel.

It is expected that only one of subtree_ordinal or subtree_reference will be used, however I don't see a good reason to enforce the use of only one, so I did not group the attributes in a oneof constraint.

If either of the above points ((1) keeping subtree_ordinal and (2) not enforcing a oneof constraint) are changed, then this PR would likely become a breaking change.

Also, this PR addresses #725 , but uses the name subtree_reference and subtree_anchor (instead of plan_anchor) to be consistent with the pre-existing subtree_ordinal attribute. I don't know that relations in a substrait plan are strictly trees, so I think this could be a good time to change "subtree" to "planrel" or "subgraph" or something similar.

This adds an "anchor" to `PlanRel` that can be referenced from a
`ReferenceRel`. This anchor and reference relationship provides a
non-ordinal method for identifying and accessing a "subtree" or
sub-graph.

This commit leaves in the original `subtree_ordinal` attribute since it
seems a (mildly) more performant method for referencing a subtree, but
also since it is still relevant in the typical case. The new anchor
improves cases where multiple plans are merged and at least one
already contains a `ReferenceRel`.

It is expected that only one of `subtree_ordinal` or `subtree_reference`
will be used, however I don't see a good reason to enforce the use of
only one, so I did not group the attributes in a `oneof` constraint.

Issue: substrait-io#725
@CLAassistant
Copy link

CLAassistant commented Oct 25, 2024

CLA assistant check
All committers have signed the CLA.

@tokoko
Copy link
Contributor

tokoko commented Oct 28, 2024

This PR leaves in the original subtree_ordinal attribute since it seems a (mildly) more performant method for referencing a subtree

This is technically true, but on the other hand we are already doing the same thing with function extensions when the number of functions used in a typical plan most likely will be a lot bigger than the number of subtrees. I don't think it makes sense to keep the existing behavior only due to performance considerations.

It is expected that only one of subtree_ordinal or subtree_reference will be used, however I don't see a good reason to enforce the use of only one, so I did not group the attributes in a oneof constraint.

I think leaving both options especially w/o oneof will lead to a lot more confusion and possible divergence in implementations. My choice would be to deprecate subtree_ordinal and eventually get rid of it, but oneof is also a viable option.

If either of the above points ((1) keeping subtree_ordinal and (2) not enforcing a oneof constraint) are changed, then this PR would likely become a breaking change.

Pretty sure putting an existing field in a oneof is a backwards-compatible change.

@drin
Copy link
Member Author

drin commented Nov 20, 2024

I should have time to finish this up in the next few days

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants