feat!: add id-based outer reference resolution for DAG plans by yongchul · Pull Request #1031 · substrait-io/substrait

yongchul · 2026-04-02T23:35:13Z

BREAKING CHANGE: NOT A REAL BREAKING CHANGE. OuterReference.steps_out field is now under oneof -- implicit optional to explicit optional. This is not a real breaking change as Substrait always required non-zero value.

Add optional RelCommon.id field and OuterReference.id_reference to support unambiguous outer reference resolution in plans with common subexpressions (ReferenceRel). The existing steps_out offset-based mechanism remains for backward compatibility with tree-shaped plans.

The RelCommon.rel_anchor can be used for other purpose but for now only required when it is used to resolve outer reference.

See #1024 for detailed motivating example and discussion.

AI disclaimer

This PR was assisted by Claude Opus 4.6.

NOT A REAL BREAKING CHANGE

OuterReference.steps_out field is moved into oneof. The field requires non-zero values thus there is no wire format change (the value has been always present on the wire for steps_out when used).

This change is

Add optional RelCommon.id field and OuterReference.id_reference to support unambiguous outer reference resolution in plans with common subexpressions (ReferenceRel). The existing steps_out offset-based mechanism remains for backward compatibility with tree-shaped plans. Closes substrait-io#1024

- Add bool lateral field to CrossRel for lateral cross product semantics - Add RelCommon.id and OuterReference.id_reference from PR substrait-io#1031 - Update JoinRel and CrossRel lateral comments to use id_reference - Update logical_relations.md with lateral docs for both JoinRel and CrossRel Depends-on: substrait-io#1031

benbellick · 2026-04-06T14:54:42Z

Am I understanding correctly that this problem only arises when a Plan.relations entry contains outer references that reach outside the bounds of that particular root?

My interpretation of Plan.relations is that each entry is a standalone thing. Having a relation use steps_out to reference something outside its own scope feels like a misuse of Plan.relations to me. Of course, we can always use a ReferenceRel to reference one of the other relations, but their evaluation wouldn't be context dependent.

I feel like we may need a new construct here rather than tacking on the ability to reference outside of the scope of a particular root. This feels like some kind of "parameterized" relation notion, where we would want to have call sites providing values to pass in dynamically. I'm not sure yet what the right way to represent this would be but wanted to get my thoughts down for now.

What do you think?

yongchul · 2026-04-06T20:55:03Z

I love github missing thread in the PR comment... 😠

Am I understanding correctly that this problem only arises when a Plan.relations entry contains outer references that reach outside the bounds of that particular root?

Yes.

My interpretation of Plan.relations is that each entry is a standalone thing. Having a relation use steps_out to reference something outside its own scope feels like a misuse of Plan.relations to me. Of course, we can always use a ReferenceRel to reference one of the other relations, but their evaluation wouldn't be context dependent.

Yes, you could put that restriction, you have to have each relation context independent. However, this proposal is actually making it context independent because the specific outer reference is bound to one consistent value without ambiguity [modulo there is still restriction where that particular value can coming from].

I feel like we may need a new construct here rather than tacking on the ability to reference outside of the scope of a particular root. This feels like some kind of "parameterized" relation notion, where we would want to have call sites providing values to pass in dynamically. I'm not sure yet what the right way to represent this would be but wanted to get my thoughts down for now.

This goes to inline table function (or parameterized view), which is a generic solution (or sledge hammer) and I actually feel like it is an overkill. Yes, I did think initially this is the way to go when I confine myself in existing Substrait outer reference structure, but this quickly went out of control. If your binding actually does not change in the plan, why do you have to go through indirect mappings only to point to the same thing?

To truly use inline table function, either you have a) a fairly sophisticated common subexpression engine to extract the common relational template or b) support for the parameterized views. I'm not aware of (a) but many systems do support (b). In (b), you won't actually have an outer reference if you inline the view definition or if you model it actual function call. Because you already have the common definition, it is straight forward to implement in producer and they are in fact used multiple times with different binding points. For (a), if you don't have such detection, it's pretty challenging for producer extract the common pattern, and generate the definition. It's nice to have such fancy decomposition for our system to have, but I don't see us doing this anytime soon...

So, I'm not arguing we don't need inline table function. We will need that and useful for parameterized views. But I don't feel like we have to use inline table function for outer reference, because we can represent outer reference in more generic way.

I'm biased. 😆 Happy to chat more about this but I strongly prefer anything else except github PR-level comment.

drin · 2026-04-07T23:37:31Z

Could there be a common mechanism with #726? or is this change totally independent?

I don't have context on why an outer reference and a reference rel are both necessary. This seems to be the reason, but I'm not sure what context an outer reference has that can't be included in a reference rel:

Of course, we can always use a ReferenceRel to reference one of the other relations, but their evaluation wouldn't be context dependent.

(Of course, I would have to reopen #726 or maybe you can combine the change in this one since it's small)

yongchul · 2026-04-08T00:09:23Z

@drin

Could there be a common mechanism with #726? or is this change totally independent?

I believe they are independent. If you squint, you could use the rel_id of the root rel of PlanRel but it is indirect while you seemed to look into more direct reference in #726. Or, rel_id value is sufficient for PlanRel anchor value you are looking for but not necessary, and I expect you want more direct reference at PlanRel message level.

I don't have context on why an outer reference and a reference rel are both necessary. This seems to be the reason, but I'm not sure what context an outer reference has that can't be included in a reference rel:

reference rel is about sharing a relational subtree (e.g., common table expressions, views). outer reference is a concept that you can correlate one relational tree to another (see lateral join #973 for example). They are orthogonal.

Of course, we can always use a ReferenceRel to reference one of the other relations, but their evaluation wouldn't be context dependent.
(Of course, I would have to reopen #726 or maybe you can combine the change in this one since it's small)

I wouldn't combine them because they are different concept.

Please let me know if there is some misunderstanding. Again, I hate PR-level comment to discuss something. If you want to discuss, let's create a comment in a file, then starts a thread.

jacques-n · 2026-04-08T01:42:54Z

@yongchul , have you looked at all at how to change this in Datafusion/Duckdb? I think maybe one/both have implemented subqueries (maybe?) and am curious the complexity to update to new pattern.

yongchul · 2026-04-08T05:51:08Z

@yongchul , have you looked at all at how to change this in Datafusion/Duckdb? I think maybe one/both have implemented subqueries (maybe?) and am curious the complexity to update to new pattern.

@jacques-n conceptually, if the implementer already maintains schema stack, the new approach can be implemented by look up by the id. So the content of stack changes from (schema) to (id, schema), then you can simply go over the stack until you find the schema with matching id.

I schemed datafusion. With my inexperienced rusty eyes, it does seem to follow above structure so fairly easy to extend. I couldn't find handling of RelCommon but this could be done easily as well when you push schema to the stack.

I admit that I used a little loose definition of uniqueness of id here though. I don't know how hard to check the uniqueness of non-zero ids across the plan... maybe this is difficult in datafusion?

I will look into duckdb as well.

jacques-n

looks good to me.

benbellick

I generally think this is good now that I properly understand the problem, but just have some things I'd like clarified.

Thanks!

benbellick · 2026-04-15T18:18:02Z

+
+#### `id_reference` (id-based)
+
+When a plan contains **common subexpressions** via `ReferenceRel`, the same relation can be reached through multiple paths with different depths. In that case, offset-based resolution is ambiguous because `steps_out` depends on *which* path is followed.


I think that this section may read a bit better if instead it was

#### `steps_out` (offset-based) <explanation of WHAT steps_out is with an example> #### `id_reference` (id-based) <explanation of WHAT id_reference is with an example> #### When `id_reference` is required example of case where it is required.

I think it would read clearer to people without the context of the problem this is solving to just see the concept in isolation. Then in the subsequent section, they see why you need to use one vs the other. WDYT?

Revised accordingly. PTAL

benbellick · 2026-04-15T18:31:51Z

+
+      // Plan-wide unique id of the relation whose output this reference is
+      // rooted on. Must match an id defined on a RelCommon in the plan.
+      // Must be > 0 when set. Recommended when the plan contains shared


mentioned elsewhere, but could we just make it required in this case?

I am trying to avoid presenting this as a breaking change. Perhaps it is fine. Let's see how others think.

Co-authored-by: Ben Bellick <36523439+benbellick@users.noreply.github.com>

benbellick · 2026-04-23T19:32:02Z

+        └── ReferenceRel(0)
+```
+
+The shared relation `x` contains an outer reference to `tableA.a`. It is reached through two paths — one via subquery (2), one directly — so `steps_out` would need a different value depending on which path is followed. With `id_reference = 1`, the reference inside `x` unambiguously names `ProjectRel` to resolve the outer reference regardless of path.


It would be nice to show the example of the "good" case right after, i.e. the first relation but using an id reference.

Hmm... Do you mean making the example more explicit using steps out and id? I'll make the change.

Revised. PTAL

Co-authored-by: Ben Bellick <36523439+benbellick@users.noreply.github.com>

benbellick

Just some documentation suggestions I have suggested, feel free to take or leave them, they are mostly nits. Otherwise LGTM!

Co-authored-by: Ben Bellick <36523439+benbellick@users.noreply.github.com>

…it-io#1031) BREAKING CHANGE: NOT A REAL BREAKING CHANGE. OuterReference.steps_out field is now under oneof -- implicit optional to explicit optional. This is not a real breaking change as Substrait always required non-zero value. Add optional RelCommon.id field and OuterReference.id_reference to support unambiguous outer reference resolution in plans with common subexpressions (ReferenceRel). The existing steps_out offset-based mechanism remains for backward compatibility with tree-shaped plans. The `RelCommon.rel_anchor` can be used for other purpose but for now only required when it is used to resolve outer reference. See substrait-io#1024 for detailed motivating example and discussion. # AI disclaimer This PR was assisted by Claude Opus 4.6. # NOT A REAL BREAKING CHANGE `OuterReference.steps_out` field is moved into `oneof`. The field requires non-zero values thus there is no wire format change (the value has been *always* present on the wire for `steps_out` when used).  - - - This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/substrait-io/substrait/1031)  --------- Co-authored-by: Ben Bellick <36523439+benbellick@users.noreply.github.com>

- Add bool lateral field to CrossRel for lateral cross product semantics - Add RelCommon.id and OuterReference.id_reference from PR substrait-io#1031 - Update JoinRel and CrossRel lateral comments to use id_reference - Update logical_relations.md with lateral docs for both JoinRel and CrossRel Depends-on: substrait-io#1031

yongchul requested review from EpsilonPrime, cpcloud, jacques-n, vbarua and westonpace as code owners April 2, 2026 23:35

yongchul added the PMC Ready PRs ready for review by PMCs label Apr 3, 2026

yongchul mentioned this pull request Apr 3, 2026

feat: introduce lateral join in the JoinRel for a correlated subquery evaluation #973

Open

jacques-n approved these changes Apr 8, 2026

View reviewed changes

benbellick reviewed Apr 15, 2026

View reviewed changes

Apply suggestions from code review

4c94434

Co-authored-by: Ben Bellick <36523439+benbellick@users.noreply.github.com>

yongchul requested a review from nielspardon as a code owner April 15, 2026 20:50

yongchul force-pushed the correlated_reference branch from e36642e to 4c94434 Compare April 16, 2026 07:34

Changes to oneof. update example. resolved PR feedback.

cc1f7cd

jacques-n approved these changes Apr 22, 2026

View reviewed changes

yongchul changed the title ~~feat: add id-based outer reference resolution for DAG plans~~ feat!: add id-based outer reference resolution for DAG plans Apr 22, 2026

yongchul and others added 2 commits April 22, 2026 11:13

Consistently use gte 1

649c5ef

Merge branch 'main' into correlated_reference

e005437

benbellick approved these changes Apr 23, 2026

View reviewed changes

yongchul and others added 2 commits April 23, 2026 14:45

Apply suggestions from code review

bf65fdc

Co-authored-by: Ben Bellick <36523439+benbellick@users.noreply.github.com>

Address PR feedback

0471b8c

benbellick approved these changes Apr 30, 2026

View reviewed changes

benbellick reviewed Apr 30, 2026

View reviewed changes

Comment thread proto/substrait/algebra.proto Outdated

yongchul and others added 2 commits April 30, 2026 14:43

Apply suggestions from code review

11560b9

Co-authored-by: Ben Bellick <36523439+benbellick@users.noreply.github.com>

rename id to rel_anchor. id_reference to rel_reference.

0981644

yongchul merged commit ad664dc into substrait-io:main May 1, 2026
15 checks passed


		#### `id_reference` (id-based)

		When a plan contains common subexpressions via `ReferenceRel`, the same relation can be reached through multiple paths with different depths. In that case, offset-based resolution is ambiguous because `steps_out` depends on which path is followed.

Conversation

yongchul commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

AI disclaimer

NOT A REAL BREAKING CHANGE

Uh oh!

benbellick commented Apr 6, 2026

Uh oh!

yongchul commented Apr 6, 2026

Uh oh!

drin commented Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yongchul commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jacques-n commented Apr 8, 2026

Uh oh!

yongchul commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jacques-n left a comment

Choose a reason for hiding this comment

Uh oh!

benbellick left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

benbellick Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

yongchul Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benbellick Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

yongchul Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benbellick Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

yongchul Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

yongchul Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

benbellick left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yongchul commented Apr 2, 2026 •

edited

Loading

drin commented Apr 7, 2026 •

edited

Loading

yongchul commented Apr 8, 2026 •

edited

Loading

yongchul commented Apr 8, 2026 •

edited

Loading