-
Notifications
You must be signed in to change notification settings - Fork 192
feat: add support for unresolved expressions #1063
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 | ||
| - UNKNOWN | ||
| - type: I32 | ||
| metadata: | ||
| storage_bytes: 4 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1019,6 +1019,7 @@ message Expression { | |
| Lambda lambda = 15; | ||
| LambdaInvocation lambda_invocation = 16; | ||
| ExecutionContextVariable execution_context_variable = 17; | ||
| NamedExpression named_expression = 18; | ||
|
|
||
| // deprecated: enum literals are only sensible in the context of | ||
| // function arguments, for which FunctionArgument should now be | ||
|
|
@@ -1254,6 +1255,17 @@ message Expression { | |
| Nested.Struct arguments = 2; | ||
| } | ||
|
|
||
| // A named expression that is not yet resolved to a positional reference or | ||
| // concrete expression. The names field is used to represent namespacing | ||
| // (e.g. qualifier and field name). Producers and consumers must share enough | ||
| // external context to resolve this expression before execution unless the | ||
| // consumer defines semantics for unresolved named expressions. Until | ||
| // resolved, a NamedExpression's type is Type.Unknown. | ||
| message NamedExpression { | ||
| repeated string names = 1; | ||
| substrait.extensions.AdvancedExtension advanced_extension = 10; | ||
|
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 think this would be the only expression element which has |
||
| } | ||
|
|
||
| // A scalar function call. | ||
| message ScalarFunction { | ||
| // Points to a function_anchor defined in this plan, which must refer | ||
|
|
@@ -1271,6 +1283,9 @@ message Expression { | |
| // - Enum arguments must be bound using FunctionArgument.enum | ||
| // followed by Enum.specified, with a string that case-insensitively | ||
| // matches one of the allowed options. | ||
| // | ||
| // In a partially bound expression, Type.Unknown may stand in for any | ||
| // concrete type while binding value or type arguments. | ||
| repeated FunctionArgument arguments = 4; | ||
|
|
||
| // Options to specify behavior for corner cases, or leave behavior | ||
|
|
@@ -1279,7 +1294,8 @@ message Expression { | |
| repeated FunctionOption options = 5; | ||
|
|
||
| // Must be set to the return type of the function, exactly as derived | ||
| // using the declaration in the extension. | ||
| // using the declaration in the extension. In a partially bound expression, | ||
| // this may be Type.Unknown if the concrete return type is unresolved. | ||
| Type output_type = 3; | ||
|
|
||
| // Deprecated; use arguments instead. | ||
|
|
@@ -1310,6 +1326,9 @@ message Expression { | |
| // - Enum arguments must be bound using FunctionArgument.enum | ||
| // followed by Enum.specified, with a string that case-insensitively | ||
| // matches one of the allowed options. | ||
| // | ||
| // In a partially bound expression, Type.Unknown may stand in for any | ||
| // concrete type while binding value or type arguments. | ||
| repeated FunctionArgument arguments = 9; | ||
|
|
||
| // Options to specify behavior for corner cases, or leave behavior | ||
|
|
@@ -1318,7 +1337,8 @@ message Expression { | |
| repeated FunctionOption options = 11; | ||
|
|
||
| // Must be set to the return type of the function, exactly as derived | ||
| // using the declaration in the extension. | ||
| // using the declaration in the extension. In a partially bound expression, | ||
| // this may be Type.Unknown if the concrete return type is unresolved. | ||
| Type output_type = 7; | ||
|
|
||
| // Describes which part of the window function to perform within the | ||
|
|
@@ -1839,6 +1859,9 @@ message AggregateFunction { | |
| // - Optional enum arguments must be bound using FunctionArgument.enum | ||
| // followed by either Enum.specified or Enum.unspecified. If specified, | ||
| // the string must case-insensitively match one of the allowed options. | ||
| // | ||
| // In a partially bound expression, Type.Unknown may stand in for any | ||
| // concrete type while binding value or type arguments. | ||
| repeated FunctionArgument arguments = 7; | ||
|
|
||
| // Options to specify behavior for corner cases, or leave behavior | ||
|
|
@@ -1847,7 +1870,8 @@ message AggregateFunction { | |
| repeated FunctionOption options = 8; | ||
|
|
||
| // Must be set to the return type of the function, exactly as derived | ||
| // using the declaration in the extension. | ||
| // using the declaration in the extension. In a partially bound expression, | ||
| // this may be Type.Unknown if the concrete return type is unresolved. | ||
| Type output_type = 5; | ||
|
|
||
| // Describes which part of the aggregation to perform within the context of | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,6 +46,13 @@ message Type { | |
| Map map = 28; | ||
| Func func = 38; | ||
|
|
||
| // A placeholder type whose concrete type has not yet been resolved. | ||
| // Unknown may be used where a concrete type would normally appear in | ||
| // partially bound plans and expressions, and must be resolved to a | ||
| // concrete type before execution unless the consumer defines semantics for | ||
| // unknown-typed values. | ||
| Unknown unknown = 39; | ||
|
|
||
| UserDefined user_defined = 30; | ||
|
|
||
| // Deprecated in favor of user_defined, which allows nullability and | ||
|
|
@@ -225,6 +232,12 @@ message Type { | |
| Nullability nullability = 3; | ||
| } | ||
|
|
||
| message Unknown { | ||
| // Optional nullability constraint for this unknown type. If unspecified, | ||
| // both the concrete type and its nullability are unresolved. | ||
| Nullability nullability = 1; | ||
| } | ||
|
Comment on lines
+235
to
+239
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. If we were to resolve type later on, I don't see much point of having this... Let it be just pure place holder (i.e., empty proto message) and bound to a real concrete type including nullability.
Author
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. Thanks @yongchul will take note of that |
||
|
|
||
| message UserDefined { | ||
| // References a type_anchor defined in the plan's extension declarations. | ||
| uint32 type_reference = 1; | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,54 @@ | ||||||
| # Unbound Expressions | ||||||
|
|
||||||
| Substrait normally represents bound relational expressions: field references are positional, value types are known, and function invocations identify implementations whose argument and return types have been derived. Some producers need to serialize expressions earlier in planning, before names and types have been resolved. | ||||||
|
|
||||||
| An expression tree is partially bound when it contains either an [`unknown`](../types/type_classes.md#unknown-type) type or a `NamedExpression`. Consumers may validate and transform partially bound expressions, but must resolve them before execution unless they define their own semantics for unresolved names or unknown-typed values. | ||||||
|
|
||||||
| This applies anywhere Substrait uses `Expression`, including both full `Plan` messages and `ExtendedExpression`. The examples below use `ExtendedExpression` because expression-only filter / projection APIs are a common motivating use case, not because partially bound expressions are limited to that entry point. | ||||||
|
|
||||||
| ## Detecting Partial Binding | ||||||
|
|
||||||
| There is no separate "unbound expression" message. Instead, partially bound state is detected structurally: | ||||||
|
|
||||||
| - If any expression, function argument, or schema field type is `unknown`, the expression is partially bound. | ||||||
| - If any expression contains `NamedExpression`, the expression is partially bound. | ||||||
|
|
||||||
| This is the canonical way to distinguish fully bound expressions from partially bound expressions in Substrait. | ||||||
|
|
||||||
| ## Unknown Type | ||||||
|
|
||||||
| The `unknown` type marks an expression whose concrete type is not known yet. It may be used anywhere a concrete type would normally be expected in a partially bound function call. If only the nullability is known, set the nullability field; otherwise leave it unspecified. | ||||||
|
|
||||||
| ## Named Expression | ||||||
|
|
||||||
| `NamedExpression` represents a reference by name instead of ordinal position. The `names` field stores one or more namespace components, such as `["foo"]` for an unqualified name or `["orders", "amount"]` for a qualified name. Until resolved, a named expression's type is `unknown`. Resolution of these components is intentionally external to Substrait and must be understood by both producer and consumer. | ||||||
|
|
||||||
| ```protobuf | ||||||
| --8<-- "examples/proto-textformat/unbound_expression/named_expression.textproto" | ||||||
| ``` | ||||||
|
Comment on lines
+22
to
+28
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. I am almost allegic to strings at this point especially when the same string can appear multiple times. As @benbellick mentioned, Substrait has been stick to anchor model for this problem and it is better to be consistent with that pattern. That is, declare unbound type, unbound expression, unbound schema, and unbound relations at the query plan or extended expression level, then reference them via anchor. |
||||||
|
|
||||||
| ## Function Example | ||||||
|
|
||||||
| Partially bound function calls can use named expressions as value arguments and `unknown` as the output type when the return type cannot be derived yet. | ||||||
|
|
||||||
| ```protobuf | ||||||
| --8<-- "examples/proto-textformat/unbound_expression/scalar_function_unknown.textproto" | ||||||
| ``` | ||||||
|
|
||||||
| ## Extended Expression Protocols | ||||||
|
|
||||||
| Expression-level APIs, such as filters and projections exchanged outside a full `Plan`, should use `ExtendedExpression`. This lets the producer include output names, any known input names, and extension declarations next to the expression tree. | ||||||
|
|
||||||
| If the input names are known but their types are not, `base_schema` can contain fields with `unknown` types. If the function overload is also unresolved, the function can refer to the `extension:io.substrait:unknown` extension until a downstream binder replaces it with a concrete function reference and concrete output type. In that case, the referenced function name should use the normal Substrait function-signature form with `unknown` short names, such as `add:unknown_unknown`. | ||||||
|
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. This would look something like the following, correct? This does seem reasonable, though it raises a question for me: In which extensions do you go looking for the matching function?
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.
The answer, I guess, it's up to the consumer how you interpret or resolve this. :-) To my biased view, there is dependency in resolving these unbound things.
The producer allude a hint ("yeah, you should use something in this namespace or these namespaces"). But it is also possible that they can be entirely unknown -- only function name is known and this is the case you are describing. We could leave this completely open "it's up to the consumer, we don't care how it binds it. But I think it is quite reasonable to consumer to specify a preference list effectively meaning the resolution policy. This is much better story for the interoperability IMO.
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. that's exactly what I pointed out above. for python builder wrapper (pr) my preferred choice was to accept a list of function urns which might or might not even have the same name and "resolve" step simply finds the first compatible impl going through the list. if we want to support unknown types in substrait plan itself, I guess we could do the same in substrait proper.
Author
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. Thank you @vbarua @yongchul @tokoko for the continued feedback.
Yes, that is roughly the shape I had in mind. One small correction: in the example from the doc I would expect the unresolved function signature to be something like I also agree your question gets to the harder part very quickly: once we allow an unresolved function reference, what is the actual lookup / resolution policy? Right now I think the changes I made doesnt have a strong enough answer for that. In particular, based off the feedback I do not think we should implicitly standardize behavior like:
The producer may be able to provide hints, such as preferred URNs / namespaces, but the exact resolution policy does not feel settled enough to bake into this PR as written. So I think this is another sign that unresolved function resolution is really its own design topic, separate from |
||||||
|
|
||||||
| ```protobuf | ||||||
| --8<-- "examples/proto-textformat/extended_expression/unbound_named_projection.textproto" | ||||||
| ``` | ||||||
|
|
||||||
| Consumers that execute expressions must reject or bind away all `NamedExpression` and `unknown` types before execution unless they explicitly support unresolved semantics. A typical binder resolves `NamedExpression` values to `FieldReference`, replaces `unknown` input and output types with concrete types, and updates unresolved function references to concrete overloads. | ||||||
|
|
||||||
| ## Plans, ReadRel, and Dynamic Parameters | ||||||
|
|
||||||
| Partially bound expressions may also appear inside a `Plan`. In that case, relational operators such as `ReadRel` still provide the surrounding schema context. If field names are known, `ReadRel.base_schema` remains the source of those names even before ordinal binding has happened. A downstream binder can use that schema to resolve `NamedExpression` values into positional `FieldReference`s. If names are known but types are not, `base_schema` can use `unknown` types until type resolution completes. | ||||||
|
|
||||||
| `NamedExpression` is intentionally distinct from `DynamicParameter`. A `DynamicParameter` represents an externally supplied literal value, identified by `parameter_anchor` and resolved through `DynamicParameterBinding`. A `NamedExpression` represents an unresolved field-like or catalog-like reference that is expected to bind against schema or catalog context. Because these are different binding problems, this proposal keeps `DynamicParameterBinding` and `NamedExpression` separate. | ||||||
|
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. Thanks for adding the explanation. only comment I would have here is that once this PR gets merged it no longer is a proposal so I would remove the last sentence or change the wording.
Suggested change
|
||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -107,3 +107,12 @@ func<(any1, any2, any3) -> any4> | |||||
| ``` | ||||||
|
|
||||||
| Function types use the arrow syntax (`->`) to separate parameter types from the return type. For multiple parameters, use parentheses to group the parameter types. See [Lambda Expressions](../expressions/lambda_expressions.md) for more details on lambda expressions and their usage. | ||||||
|
|
||||||
| ### Unknown Type | ||||||
|
|
||||||
| The `unknown` type is written as `unknown` and may include the normal nullability marker: | ||||||
|
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. minor wording change: I don't think we need to qualify the nullability indicator with "normal" in the docs. Also, for consistenct: it seems it is called an "indicator" in the docs above and not a "marker".
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. I don't see much point to do the nullability with unknown because you will have to rederive nullability along with the conrete function binding etc, etc. It's just simpler to leave it as a pure hole without anything attached IMO. |
||||||
|
|
||||||
| ``` | ||||||
| unknown | ||||||
| unknown? | ||||||
| ``` | ||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -15,3 +15,5 @@ Refer to [Type Parsing](type_parsing.md) for a description of the syntax used to | |||||
|
|
||||||
| !!! note "Note" | ||||||
| Substrait employs a strict type system without any coercion rules. All changes in types must be made explicit via [cast expressions](../expressions/specialized_record_expressions.md). | ||||||
|
|
||||||
| Partially bound expressions may use the [`unknown`](type_classes.md#unknown-type) type as a placeholder until a downstream binder resolves the concrete type. | ||||||
|
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. minor: shouldn't change the rendering but I guess we can remove the leading spaces here
Suggested change
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| # message ExtendedExpression | ||
| # Arrow-style expression protocols should use ExtendedExpression so the message | ||
| # carries expression names and extension declarations with the expression tree. | ||
| version { | ||
| major_number: 0 | ||
| minor_number: 0 | ||
| patch_number: 0 | ||
| producer: "substrait" | ||
| } | ||
| extension_urns { | ||
| extension_urn_anchor: 1 | ||
| urn: "extension:io.substrait:unknown" | ||
| } | ||
| extensions { | ||
| extension_function { | ||
| extension_urn_reference: 1 | ||
| function_anchor: 1 | ||
| name: "add:unknown_unknown" | ||
| } | ||
| } | ||
| base_schema { | ||
| names: "a" | ||
| names: "b" | ||
| struct { | ||
| types { | ||
| unknown {} | ||
| } | ||
| types { | ||
| unknown {} | ||
| } | ||
| nullability: NULLABILITY_REQUIRED | ||
| } | ||
| } | ||
| referred_expr { | ||
| expression { | ||
| scalar_function { | ||
| function_reference: 1 | ||
| arguments { | ||
| value { | ||
| named_expression { | ||
| names: "a" | ||
| } | ||
| } | ||
| } | ||
| arguments { | ||
| value { | ||
| named_expression { | ||
| names: "b" | ||
| } | ||
| } | ||
| } | ||
| output_type { | ||
| unknown {} | ||
| } | ||
| } | ||
| } | ||
| output_names: "sum" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| # message Expression | ||
| # An unresolved reference to a named expression, such as a column name entered | ||
| # before a schema has been bound. | ||
| named_expression { | ||
| names: "foo" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| # message Expression | ||
| # A partially bound scalar function over unresolved named expressions. The | ||
| # function_reference is still plan-local and must be declared by the enclosing | ||
| # Plan or ExtendedExpression. The unknown output type records that concrete | ||
| # type derivation has not completed. | ||
| scalar_function { | ||
| function_reference: 1 | ||
| arguments { | ||
| value { | ||
| named_expression { | ||
| names: "a" | ||
| } | ||
| } | ||
| } | ||
| arguments { | ||
| value { | ||
| named_expression { | ||
| names: "b" | ||
| } | ||
| } | ||
| } | ||
| output_type { | ||
| unknown {} | ||
| } | ||
| } |
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.
🧵 Commenting here soas not to polute the global comment space but this is meant to discuss specifically the usage of a reference id vs names.
@malinjawi thanks for the writeup! Though I think my point may have been misunderstood.
I’m not suggesting that the producer should invent ordinal
FieldReferences before it knows the schema/layout. I agree that would defeat the purpose of this feature.What I’m suggesting is that the unresolved names could be declared once in the enclosing message and then referenced by anchor from the expression tree, rather than storing the name components inline in every
NamedExpression.Conceptually, something like:
A later binder would resolve
unresolved_name_ref(1)andunresolved_name_ref(2)against the appropriate local schema/catalog context and replace them with positionalFieldReferences.The registry would not solve binding by itself, but it would keep the expression tree closer to the existing Substrait pattern where expressions refer to declarations by anchor, while names/declarations live in the enclosing
PlanorExtendedExpression.So I’m not arguing for general named references throughout Substrait in this PR. I’m mostly asking whether, even for this narrower unresolved-reference use case, we should avoid embedding
repeated string namesdirectly in each expression and instead use an anchor-based declaration/reference pattern.Uh oh!
There was an error while loading. Please reload this page.
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.
Thanks @benbellick, that makes sense. I agree this is really a separate question from whether unresolved references are useful at all.
I could modify the use of inline
repeated string names. I see i misunderstood the mechanism for an unresolved field-like reference.The anchor / registry pattern you describe seems like a valid alternative and is closer to the way Substrait already handles other plan-level declarations. I think that design could make sense if the group wants unresolved named references to move forward.