docs: fix prose for extended expression #434
Conversation
wjones127
force-pushed
the
docs-extended-expressions-grammar
branch
from
b395e12 to
ac372ed
Compare
|
|
||
| ## Referred expression | ||
|
|
||
| Extended expressions can contain one or multiple expressions. A single expression is used for filters, while multiple expressions are used for projections and aggregations. Each of these expressions is represented by an `ExpressionReference` message, which can either contain an [Expression](https://github.com/substrait-io/substrait/blob/7f272f13f22cd5f5842baea42bcf7961e6251881/proto/substrait/algebra.proto) or [AggregateFunction](https://github.com/substrait-io/substrait/blob/7f272f13f22cd5f5842baea42bcf7961e6251881/proto/substrait/algebra.proto#L1170) message. In the future, other types might be allowed to handle different use cases. |
There was a problem hiding this comment.
A single expression is used for filters, while multiple expressions are used for projections and aggregations.
This seems overly prescriptive. For example, I feel like this implies you would only use this for one of those three things. However, someone might want to use this for expression evaluation that is completely unrelated to execution plans or engines.
Maybe something along the lines of "if multiple expressions are specified then there is no implicit relationship between the different top-level expressions. Each expression should be considered independent and operates on the same top-level schema."
There was a problem hiding this comment.
Perhaps I can offer that as example usage, but not present as only valid use?
There was a problem hiding this comment.
But I will also add that note about the expressions not depending on each other.
|
|
||
| For multi expressions, user can translate them following same order as it occurs in original plan rel. But it does NOT require the consume side to handle it strictly in previous order. Only need to make sure columns in final output are organized in same order as defined in extended expression message. | ||
| If there are multiple expressions, the order of the inputs and outputs matter. Input data will match the schema order |
There was a problem hiding this comment.
What does "the order of the inputs matter" mean?
I agree that the output schema must align with the expression order.
There was a problem hiding this comment.
Changed to "the field order of the input and output data matters".
wjones127
force-pushed
the
docs-extended-expressions-grammar
branch
from
afcf9fd to
f713073
Compare
wjones127
force-pushed
the
docs-extended-expressions-grammar
branch
from
f713073 to
9278d1a
Compare
wjones127
requested review from
westonpace and
vibhatha
and removed request for
vibhatha and
westonpace
| @@ -19,33 +19,42 @@ message ExpressionReference { | |||
| AggregateFunction measure = 2; | |||
| } | |||
| // Field names in depth-first order | |||
| // | |||
| // For primitive types, this will just be a single name. Nested types may | |||
| // have multiple names. | |||
| repeated string output_names = 3; | |||
There was a problem hiding this comment.
Is this a required field? The description below seems to suggest it is but it might not hurt to mention that here.
|
CC @yma11 did you want to confirm this description matches your understanding? |
Co-authored-by: Weston Pace <[email protected]>
You description is quite detailed and accurate for extended expression usage. Just one point: |
EpsilonPrime
added
the
awaiting-user-input
|
No progress has been made in more than six months. Closing without prejudice. |
Follow up on #405
#405 (comment)