feat: add support for unresolved expressions#1063
Conversation
|
Thanks for the proposal. Some initial thoughts from my side. The new I noticed that you are mainly covering the I'm mentioning this since with If the idea is that I remember we also had some discussions (#794, #809 (comment)) about the role of |
|
I think you would need to rebase on latest main to get the breaking changes check to pass. |
af389b6 to
abf5dcc
Compare
|
Thanks @nielspardon , this is helpful feedback. I rebased the branch on latest On the design question: yes, I did intend this to apply to both entry points, not only I don't think I agree the interaction with
If it would help, I can add a concrete |
I think the breaking change check only failed since your PR branch did not include newly added fields that were already in |
|
It would be great if you can also update the Substrait dialect schema definition to allow declaring support for the new type and expression. There are a couple simple test instances of dialects here: https://github.com/substrait-io/substrait/tree/main/dialects/tests |
abf5dcc to
11fa472
Compare
|
Thanks @nielspardon, I picked this up. I am updating the dialect schema definition to allow declaring support for the new I also rebased the branch so theCLA check can associate the head commit with my GitHub account. |
| // resolved, a NamedExpression's type is Type.Unknown. | ||
| message NamedExpression { | ||
| repeated string names = 1; | ||
| substrait.extensions.AdvancedExtension advanced_extension = 10; |
There was a problem hiding this comment.
I think this would be the only expression element which has AdvancedExtension. It would help if you can explain your use case for advanced extensions on an expression.
|
|
||
| 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. |
There was a problem hiding this comment.
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.
| `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. | |
| `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. |
|
|
||
| ### Unknown Type | ||
|
|
||
| The `unknown` type is written as `unknown` and may include the normal nullability marker: |
There was a problem hiding this comment.
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".
| The `unknown` type is written as `unknown` and may include the normal nullability marker: | |
| The `unknown` type is written as `unknown` and may include the nullability indicator: |
There was a problem hiding this comment.
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.
| !!! 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. |
There was a problem hiding this comment.
minor: shouldn't change the rendering but I guess we can remove the leading spaces here
| Partially bound expressions may use the [`unknown`](type_classes.md#unknown-type) type as a placeholder until a downstream binder resolves the concrete type. | |
| Partially bound expressions may use the [`unknown`](type_classes.md#unknown-type) type as a placeholder until a downstream binder resolves the concrete type. |
|
I marked this a ready for review since I would like to get some feedback from the other PMC members. |
|
@malinjawi thanks for the contribution! I am hoping to take a look at this sometime this week. Would you mind sharing what your concrete use-case is for this feature? I see you mentioned:
Can you share some additional details? Thanks! |
|
Thank you @malinjawi for contribution! ✋ I will get to this later this week -- likely Friday. One ask for you. Could you structure your PR description so that make it clear the intended usage with a simple example? |
Thanks @benbellick and @yongchul! The concrete use case on our side is that there is a separation between what the system knows at query plan creation time and what it knows at query plan execution time. At plan creation time, our platform may already know the structure of the query and the referenced names, but it may not yet know the concrete table schemas, expression types, or function overloads. Those may only become available later, closer to execution. That is the gap this PR is trying to address. Without unresolved expressions, generating Substrait effectively requires the system to already know:
With unresolved expressions, the system can emit Substrait earlier, and then a later binding phase can resolve:
A simple example would be something like: SELECT a + b AS sum
FROM t
WHERE a > 1 |
|
I think this kinda mirrors how we build plans/expressions in substrait-python. everything (types, function references) need to be known before the plan is built of course, but we still have a separation between user api that might not care about these things and final build step that needs to know everything (when Some of my remarks/concerns from that experience:
|
@tokoko Is your main point here that they could be introduced in separate PRs? Or is your point about how they are described in the documentation? |
|
@nielspardon separate PRs sounds good, but I think It's a bit more than that. If we're introducing named references, my preference would be to go all in on that, which would mean changes in more places:
aside from that, I might be wrong but I think treating them as separate would simplify building consensus. |
Thanks, got it. We could split this into at least 2 PRs though I'm thinking more of 3 PRs. I think the focus for named field references in this PR was to introduce this for unresolved / unbound plans or for unresolved / unbound extended expressions. I think introducing such a named field reference only for unbound / unresolved plans could also be done separately from looking into supporting names more broadly. We seem to often struggle with PRs that have a relatively large scope so I think breaking things down into smaller chunks could help to keep the discussion focused, getting changes reviewed and merged more quickly.
|
|
➕ to @tokoko’s point:
To keep this consistent with the way Substrait currently handles named/global declarations, could unresolved expressions refer to names through a plan-level registry/anchor instead of embedding That would make this closer to how functions and types are referenced. If that approach is not possible or does not fit the intended use case, could someone help me understand why? |
|
Thanks @benbellick, @tokoko, and @nielspardon for your insights. In our use case, there was a separation between what the system knows at plan creation time and what it knows later at binding / execution time. At creation time, we may know the query structure and referenced names, but not yet the concrete schema, ordinal field positions, concrete expression types, or final function overloads. That is why On the named-reference side, my intent here was not to propose general named references throughout Substrait. The intent was much narrower: just a way to represent field-like references before ordinal binding has happened. To @benbellick’s question specifically: from our side, the issue is not just that the producer knows a string like On the registry / anchor idea: I think that could be possible in principle, but my intent here was to keep this as small as possible rather than design a full named-reference system for Substrait. My assumption was that field-like references are more scope/context dependent than functions or types, so even with a registry the hard part would still be binding them in the right local schema context ( I also agree with @tokoko’s function-resolution point. In practice, unresolved types often also mean unresolved function references, since user intent is usually “add these two fields”, not “call this exact typed overload”. The breakdown @nielspardon suggested makes sense from my side:
That still covers the motivating use case for us, while keeping this discussion more focused and avoiding turning this PR into the full “named references in Substrait” design discussion. Happy to reiterate this design into something more aligned with the direction we should take. |
| Lambda lambda = 15; | ||
| LambdaInvocation lambda_invocation = 16; | ||
| ExecutionContextVariable execution_context_variable = 17; | ||
| NamedExpression named_expression = 18; |
There was a problem hiding this comment.
🧵 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:
unresolved_names:
1 -> a
2 -> b
project:
add(
unresolved_name_ref(1),
unresolved_name_ref(2)
) -> unknown
A later binder would resolve unresolved_name_ref(1) and unresolved_name_ref(2) against the appropriate local schema/catalog context and replace them with positional FieldReferences.
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 Plan or ExtendedExpression.
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 names directly in each expression and instead use an anchor-based declaration/reference pattern.
There was a problem hiding this comment.
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.
Can you expand on this? maybe an example substrait plan of how named reference would be used? of course, I understand the general use case, but in the context of substrait specifically where all Rels (and base schema of ExtendedExpression) are supposed to have a defined schema (even if one that contains a field with an unknown type), how can you build a valid substrait Plan or ExtendedExpression where you won't be able to use positional references instead of names? unless we go a step further and introduce some sort of an unresolved Rel that gives no indication regarding fields being emitted from it? |
Thanks @tokoko, this is a fair point, and I think it exposes the weak spot in my current implementation. Looking at the current Substrait model, I agree that once So to answer your question directly: In the current model, I do not have a good concrete I think that means I was overreaching by bundling unresolved types and unresolved named references together in this PR. The case for
So I think the right next step is to split this up along the lines @nielspardon suggested:
|
There was a problem hiding this comment.
+1 to the point about splitting this into separate PRs. The unknown types itself is a big enough change on it's own. I might even advocate for closing this PR and making a new one in order to preserve this discussion.
If I understand your use case, what you're saying is that your parsing frontend has enough information to take something like:
SELECT a + b AS sum
FROM t
WHERE a > 1and produce a plan like:
(pseudo-plan)
Project[add($0, $1)]
Filter[gt($0, $1)]
NamedTable("t", columns=["a"::unknown, "b"::unknown]
Table "t" may or may not exists, and columns "a" or "b" may not exists. But you want to hand this off to another system than can take the plan and does have the information to look all of this up and update the plan with concrete information:
(pseudo-plan)
Project[add($0, $1)]
Filter[gt($0, $1)]
NamedTable("t", columns=["a"::i32, "b"::i32]
Does this somewhat match what you're trying to deal with?
|
|
||
| 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`. |
There was a problem hiding this comment.
This would look something like the following, correct?
"extensionUrns": [
{
"extensionUrnAnchor": 1,
"urn": "extension:io.substrait:unknown
}
],
"extensions": [
{
"extensionFunction": {
"extensionUrnReference": 1,
"functionAnchor": 1,
"name": "add:unknown
}
}
]
This does seem reasonable, though it raises a question for me: In which extensions do you go looking for the matching function? add has implementations in both functions_arithmetic.yaml and functions_arithmetic_decimal.yaml. By default, would you search through all loaded function extensions for a match? If we are searching through all function extensions, how do we handle collisions if we have functions with the same name and signature in different extension. Which function to we resolve to there?
There was a problem hiding this comment.
In which extensions do you go looking for the matching function? add has implementations in both functions_arithmetic.yaml and functions_arithmetic_decimal.yaml.
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.
- schema
- relation
- function -- with all resolved arguments
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thank you @vbarua @yongchul @tokoko for the continued feedback.
This would look something like the following, correct?
"extensionUrns": [ { "extensionUrnAnchor": 1, "urn": "extension:io.substrait:unknown } ], "extensions": [ { "extensionFunction": { "extensionUrnReference": 1, "functionAnchor": 1, "name": "add:unknown } } ]
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 add:unknown_unknown, not add:unknown.
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:
- searching all loaded function extensions by default
- choosing the first compatible implementation
- resolving collisions between multiple extensions that expose the same function name/signature
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 Type.Unknown itself.
yongchul
left a comment
There was a problem hiding this comment.
I finally had a chance to go over the PR. My sentiments are similar to other reviewers.
+1 to split this PR
+1 to use consistent anchor based reference scheme to reference unbounded features
| message Unknown { | ||
| // Optional nullability constraint for this unknown type. If unspecified, | ||
| // both the concrete type and its nullability are unresolved. | ||
| Nullability nullability = 1; | ||
| } |
There was a problem hiding this comment.
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.
|
|
||
| ### Unknown Type | ||
|
|
||
| The `unknown` type is written as `unknown` and may include the normal nullability marker: |
There was a problem hiding this comment.
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.
| ## 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" | ||
| ``` |
There was a problem hiding this comment.
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.
Thanks for the review @vbarua! Yes, this is much closer to the core use case I was trying to describe. The important part for us is that the frontend may know enough to produce the query shape and positional expression structure, while still not knowing the concrete schema information yet. So something in the spirit of: |
|
Thanks all for the feedback here. I opened a focused draft follow-up PR for the first split:
That follow-up keeps the scope to
It intentionally leaves unresolved named references and unresolved function-resolution policy out of scope so those can be handled separately. I’m leaving this PR in place for now as the umbrella discussion/reference point while the narrowed work moves to #1081. |
Refs #515
This adds minimal protocol support for partially bound / unresolved expressions in Substrait.
Intended usage / concrete use case
The intended use case is a multi-phase planning pipeline that wants to use Substrait before binding is complete, not only after binding is complete.
On our side, the concrete motivation is that there is a separation between what the system knows at query plan creation time and what it knows at query plan execution time.
At plan creation time, the platform may already know:
but it may not yet know:
Without unresolved expressions, generating Substrait effectively requires the system to already know all of those details at plan creation time.
With unresolved expressions, the system can emit Substrait earlier, and a later binding phase can resolve:
FieldReferenceunknown-> concrete typesThis allows a cleaner pipeline such as:
In other words, this change lets Substrait serve as the IR across both the pre-binding and post-binding phases, instead of only after binding is complete.
Simple example
For a query like: