-
Notifications
You must be signed in to change notification settings - Fork 192
feat: add unbound type for partially bound plans #1081
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ dependencies: | |
| supported_types: | ||
| - I8 | ||
| - I16 | ||
| - type: UNBOUND | ||
| - type: I32 | ||
| metadata: | ||
| storage_bytes: 4 | ||
|
|
||
This file was deleted.
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -24,6 +24,20 @@ Simple type classes are those that don't support any form of configuration. For | |||||
| | date | A date within [1000-01-01..9999-12-31]. | `int32` days since `1970-01-01` | ||||||
| | interval_year | Interval year to month. Supports a range of [-10,000..10,000] years with month precision (= [-120,000..120,000] months). Usually stored as separate integers for years and months, but only the total number of months is significant, i.e. `1y 0m` is considered equal to `0y 12m` or `1001y -12000m`. | `int32` years and `int32` months, with the added constraint that each component can never independently specify more than 10,000 years, even if the components have opposite signs (e.g. `-10000y 200000m` is **not** allowed) | ||||||
| | 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` | ||||||
| | unbound | A placeholder for a type that has not yet been bound to a concrete type. May only appear in partially bound plans, which must be bound before execution. | n/a | ||||||
|
|
||||||
| ### Unbound Type | ||||||
|
|
||||||
| 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: | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. looks like |
||||||
|
|
||||||
| - `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. | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wondering whether we should specify the reverse direction:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm... interesting.... |
||||||
| - `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. | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 if we treat unbound as a type we would have the following signatures in the plan
Even if we
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
|
||||||
|
|
||||||
| Consumers that do not support partially bound plans should reject any plan containing `unbound`. | ||||||
|
|
||||||
| ### Compound Types | ||||||
|
|
||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused. did this file exist before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, that has been there for a while #79
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to removing this. IMO
unboundis 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 😅