feat: add unbound type for partially bound plans#1081
Conversation
|
@malinjawi I think you would need to rebase this PR and fix the conflicts. If you think this is ready for a review then please mark it as ready aka change it from draft status to an actual PR. |
6771477 to
ea1f0aa
Compare
| | uuid | A universally-unique identifier composed of 128 bits. Typically presented to users in the following hexadecimal format: `c48ffa9e-64f4-44cb-ae47-152b4e60e77b`. Any 128-bit value is allowed, without specific adherence to RFC4122. | 16-byte `binary` | ||
| | unknown | A placeholder for a type that has not been resolved yet. `unknown` may be used when a producer knows the query shape but not yet the concrete types. It must be resolved before execution. | n/a | ||
|
|
||
| ### Unknown Type |
There was a problem hiding this comment.
Hi @malinjawi, thanks for splitting this out!
I see the motivation section explains why an unknown type can be useful for unresolved plans. Do you expect to produce or consume plans that use unknown without also relying on the named field-like reference proposed in the other PR?
I ask because the discussion in the other PR hasn’t yet made the value of the full approach clear enough to me. I’d like to avoid merging a smaller change primarily as a step toward a larger approach that we haven’t agreed on yet. If this is mostly intended as a stepping stone toward that larger change, maybe we should discuss it in the sync tomorrow.
However, if unknown has standalone value, great! Would you mind sharing a bit more detail about the use cases where unknown is useful without the other changes from the original PR?
Thanks again, and appreciate your work here! 🚀
There was a problem hiding this comment.
Another point I'd add... I'd prefer to include "unresolved function-resolution / lookup policy" in this scope. Adding support for unknown types w/o some sort of direction with respect to functions feels like a half-baked feature to me.
There was a problem hiding this comment.
Thanks @benbellick and @tokoko, this is helpful.
Yes, unknown has standalone value for us. It is not only intended as a stepping stone toward the named field-like reference proposal.
The concrete use case is StreamSets-style flow authoring. Users can build and persist flows from a frontend before concrete schemas/types are known. This is similar to writing a SQL query before binding it against a catalog: the user may know the query/flow shape, but the exact field types are resolved later when the flow is executed against a specific source.
For example, a reusable transformation may reference an expression whose type depends on the dataset it is later bound to, such as a string field that could resolve as varchar(10), varchar(50), char(16), etc. In that state, unknown lets us represent the type hole in the plan without inventing a concrete type too early.
I agree that a plan containing unknown is not executable as-is, especially where function overload resolution depends on concrete argument types. My intent with this PR is not to define unresolved function lookup/resolution policy. It is to add the type-level placeholder needed to serialize partially bound plans, with the expectation that a later binding phase resolves concrete types and then resolves function overloads where needed.
So I see these as related but separable pieces:
unknownrepresents unresolved type holes in partially bound plans.- Unresolved field references and function-resolution policy are broader binding semantics that should be designed separately.
I’m happy to further clarify the docs to state that unknown is for partially bound/non-executable plans and must be resolved before execution.
There was a problem hiding this comment.
When the recording of the last community sync becomes available on Youtube you can find a bit more detail on what we discussed about this feature request on Wednesday.
Some things that I remember coming up:
- We should include a solution for function mappings as @tokoko also requested. @jacques-n suggested that maybe we should have a new expression type for an unbound / late-bound function.
- @jacques-n suggested that maybe
late bound typeorunbound typeis a slightly better name thanunknowntype. @vbarua was leaning towardsunbound. - there is currently a placeholder unknown type declared as an extension in https://github.com/substrait-io/substrait/blob/38d87906472c04cd957ba5a2ba3880dcc2904ff8/extensions/unknown.yaml which we should clean up when we officially introduce this new unbound type
- There was a bit of a concern that language in this split out PR potentially already anticipates that the other proposal from the original PR about adding a named field reference expression would follow exactly as originally proposed. I didn't have the time to check this PR. We should make sure this PR is self-contained if that is the case.
For the others who attended the sync: please add / correct anything I missed.
There was a problem hiding this comment.
Thanks @nielspardon for the summary, and thanks everyone for the discussion in the sync — I watched the recording and updated the PR to follow what was agreed there.
The type is now called unbound everywhere (proto, grammar, docs, dialect schema, tests), and I deleted extensions/unknown.yaml as @jacques-n suggested, since this replaces it.
On function mappings: I took the direction from the sync that Substrait shouldn't prescribe a resolution policy. The docs now just say that a plan containing unbound is partially bound and that filling the holes — including resolving function bindings once the types are concrete — is entirely up to the binding system. I'd be happy to draft the late-bound function expression @jacques-n described (function name carried as a constant, declared output type, no derivation) as a follow-up PR under #515 so the two can land close together.
I also rewrote the docs so they're self-contained — there's no language left that assumes the named-field-reference proposal from #1063, and I added a couple of clarifications that came up on the call: unbound has no literal form, and it's not the same as any (any participates in overload matching, unbound just means "not known yet").
I'll also do the sanity check that came up at the end of the call — looking at whether unbound plus a late-bound function expression would be enough to represent Spark Connect-style unresolved plans, or whether there are bigger gaps — and post what I find here.
Rebased onto main and regenerated the parsers while I was at it, so the conflicts are gone. Would appreciate another look when you get a chance.
ea1f0aa to
fbe1aa3
Compare
|
I think this needs to be rebased to get the breaking changes check to work. |
fbe1aa3 to
9969cb2
Compare
9969cb2 to
a3ade20
Compare
fb8274a to
f7a2c58
Compare
There was a problem hiding this comment.
I have not done a full review of these changes yet, but I think we do not want any mention of unbound in any of the grammar files. This is because we shouldn't allow the type in extension YAML files or in test files. Does everyone else agree?
There was a problem hiding this comment.
Since the unbound function expression is supposed to be covered as a follow-up we shouldn't need modifications to the grammars unless someone wanted to declare type variations of UNBOUND via extensions though I'm not sure how likely that is.
There was a problem hiding this comment.
Got it thanks @benbellick @nielspardon, reverted all the grammar changes. I'd added unbound to the type and function-test-case grammars without really thinking through that those are for extension YAMLs and test cases, where it has no business being. You're right that it shouldn't be there.
So unbound is proto-only now. I put the grammars (and their generated parsers) back to match main, dropped the grammar-parsing test, and removed the bit of the parsing docs that documented the syntax. The diff is down to just the proto type, the docs, and the dialect schema entry.
I left the dialect schema UNBOUND in for now since it felt like a separate thing from the grammars, but I don't feel strongly about it — happy to pull it out too if you'd rather keep this strictly proto + docs.
Also rebased onto main so the conflict's gone.
Adds a built-in placeholder type for plans serialized before all schema and type information is available: - add Type.Unbound to the protobuf type definitions - document the unbound type class as a pure placeholder: not a wildcard (distinct from `any`), no nullability or parameters, no literals, and no prescribed resolution policy - add UNBOUND to the dialect schema and dialect fixture coverage - remove the legacy extensions/unknown.yaml placeholder extension Unbound is a proto/plan-level type only. It is intentionally not added to the type or test-case grammars, since it should not appear in extension YAML files or function test cases.
f7a2c58 to
7e61982
Compare
nielspardon
left a comment
There was a problem hiding this comment.
LGTM, only one minor aspect we may want to specify
|
|
||
| A plan that contains `unbound` anywhere within it is *partially bound*. Partially bound plans may be serialized and exchanged, but they are not executable, and Substrait defines no runtime semantics for values of unbound type. Specifically: | ||
|
|
||
| - `unbound` is not a wildcard: it does not unify with, or implicitly coerce to, any other type. In particular, `unbound` is distinct from `any`: `any` is a function-signature wildcard that participates in overload matching, while `unbound` is a plan-level placeholder for a type that is not yet known. |
There was a problem hiding this comment.
I'm wondering whether we should specify the reverse direction: any matches unbound just like any other type while it is unbound?
There was a problem hiding this comment.
Hmm... interesting.... any2 f(any1, any2) would match unbound f(unbound, unbound)? 😆 I don't know these matching actually making sense or we want to try to bind this.
There was a problem hiding this comment.
I'm confused. did this file exist before?
There was a problem hiding this comment.
+1 to removing this. IMO unbound is a better approach for this, and I don't think anyone is using it. If they are, we'll find out when they come yelling 😅
|
|
||
| The `unbound` type class is a placeholder for a type that has not yet been bound to a concrete type. It may appear wherever a concrete type would normally appear, for example as a field type within `ReadRel.base_schema`. This allows a producer to serialize the shape of a plan — its relations and the names and ordering of its fields — before the concrete types of those fields are known. A downstream binder later replaces each occurrence of `unbound` with a concrete type once the necessary schema or catalog information is available. | ||
|
|
||
| A plan that contains `unbound` anywhere within it is *partially bound*. Partially bound plans may be serialized and exchanged, but they are not executable, and Substrait defines no runtime semantics for values of unbound type. Specifically: |
There was a problem hiding this comment.
| A plan that contains `unbound` anywhere within it is *partially bound*. Partially bound plans may be serialized and exchanged, but they are not executable, and Substrait defines no runtime semantics for values of unbound type. Specifically: | |
| A plan that contains `unbound` type is *partially bound*. Partially bound plans may be serialized and exchanged, but they are not executable, and Substrait defines no runtime semantics for values of unbound type. Specifically: |
There was a problem hiding this comment.
looks like unbound is consistently used instead of unbound type. I don't feel strongly but if it were type, spelling out type is not ambiguous.
|
|
||
| A plan that contains `unbound` anywhere within it is *partially bound*. Partially bound plans may be serialized and exchanged, but they are not executable, and Substrait defines no runtime semantics for values of unbound type. Specifically: | ||
|
|
||
| - `unbound` is not a wildcard: it does not unify with, or implicitly coerce to, any other type. In particular, `unbound` is distinct from `any`: `any` is a function-signature wildcard that participates in overload matching, while `unbound` is a plan-level placeholder for a type that is not yet known. |
There was a problem hiding this comment.
Hmm... interesting.... any2 f(any1, any2) would match unbound f(unbound, unbound)? 😆 I don't know these matching actually making sense or we want to try to bind this.
vbarua
left a comment
There was a problem hiding this comment.
This looks reasonable to me in terms of defining how unbound works for columns, but I think it breaks down a little bit once we start using unbound columns in functions. Until we define a mechanism to declare unbound functions, I'm not sure if plans using unbound columns in function arguments can be expressed validly. Left more details in my comments.
Also, I don't necessarily consider this a blocker if the plan is to flesh out unbound functions as a follow-up It will mean that plans containing unbound columns in functions arguments will be somewhat malformed / underspecified until we do.
There was a problem hiding this comment.
+1 to removing this. IMO unbound is a better approach for this, and I don't think anyone is using it. If they are, we'll find out when they come yelling 😅
| - `unbound` is not a wildcard: it does not unify with, or implicitly coerce to, any other type. In particular, `unbound` is distinct from `any`: `any` is a function-signature wildcard that participates in overload matching, while `unbound` is a plan-level placeholder for a type that is not yet known. | ||
| - `unbound` carries no nullability and accepts no parameters; those properties are determined when the type is bound to a concrete type. | ||
| - There is no literal of type `unbound`. | ||
| - Substrait prescribes no resolution policy for `unbound`. A function invocation whose argument types are unbound is part of a partially bound plan; how a binder assigns concrete types, and how it subsequently resolves function bindings, is left entirely to the system performing the binding. |
There was a problem hiding this comment.
A function invocation whose argument types are unbound is part of a partially bound plan;
One thing I'm still not sure about with what we're discussing here is how we would declare functions in plans that are consuming unbound arguments. From the example you gave
Project[add($0, $1)]
Filter[gt($0, 1)]
NamedTable("t", columns=["a"::unbound, "b"::unbound])
if we treat unbound as a type we would have the following signatures in the plan
gt:unbound_unboundadd:unbound_unbound
Even if we
took the direction from the sync that Substrait shouldn't prescribe a resolution policy.
As of today, neither of those are valid function signatures based on the current specification. substrait-java and substrait-go currently enforce function validity based on extension catalogs, so trying to convert a protobuf plan with these signatures would would fail because they wouldn't map to an existing function.
This is behaviour that we can change, but I think we need an have explicit mechanism to declare / identify unbound functions in the same way that we declare unbound columns. It sound like the plan is to look into this as a follow-up, which sounds reasonable to me, but until we do so I think that any plan that uses an unbound column in a function argument will be malformed.
Putting this another way: I'm not sure that we can re-use the existing mechanism to references functions with unbound types, because those mechanism assume concrete type arguments.
Caveat
unbound columns may work today with functions that consume only any arguments like eq:any_any.
Refs #515
Split from #1063
This PR adds a built-in
unboundtype: a placeholder for a type that isn't known yet.The use case: a producer sometimes knows the shape of a query — which relations, which fields, in what order — before it knows the concrete column types. Think of a pipeline authored in a UI before it gets pointed at a real data source. With
unbound, that partially bound plan can be serialized as-is, and a binder fills in the real types later, before execution.What's in the PR:
Type.Unboundproto messageUNBOUNDas an opt-in type in the dialect schemaextensions/unknown.yamlplaceholder extensionunboundis a proto/plan-level type only. It is deliberately not added to the type or function-test-case grammars: you shouldn't be able to declare a function overunboundin an extension YAML, or write a function test case with it (that was exactly what the oldextensions/unknown.yamlhack did). Per the review discussion, the grammars are left untouched.A few rules worth calling out from the docs:
unboundis not a wildcard and is notany(anyparticipates in overload matching,unboundjust means "not known yet"); it has no literal form; and Substrait deliberately does not say how resolution works. A plan containingunboundis partially bound, and it's up to the binding system to fill the holes, including resolving function bindings once the types are concrete.Not in this PR: unresolved named field references, and the late-bound function expression that came up in the sync. Happy to draft the late-bound function expression as a follow-up under #515 so the two can land close together.
Tested with
pytest tests/,check-jsonschemaon the dialect fixtures, andbuf lint/buf format/buf breakingagainst main.This change is