-
Notifications
You must be signed in to change notification settings - Fork 192
feat: introduction of simple table functions #876
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
Changes from 4 commits
1635a4d
092ba9d
26fec6b
b090c4e
a4ab6e9
15f6897
96860da
b652b87
2168c5b
4a4596f
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 |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| %YAML 1.2 | ||
| --- | ||
| # Table functions: Functions that produce relations (zero or more records). | ||
| # Currently, only 0-input functions are supported - these take constant arguments | ||
| # and generate data as leaf operators. | ||
| urn: extension:io.substrait:functions_table | ||
| table_functions: | ||
| - name: "generate_series" | ||
| description: >- | ||
| Generates a series of integer values from start to stop, incrementing by step. | ||
|
|
||
| Takes constant arguments and produces zero or more records containing a single | ||
| integer value. The series includes both the start and stop values if they fall | ||
| on a step boundary. If step is positive, stops when the value exceeds stop. | ||
| If step is negative, stops when the value is less than stop. Returns empty if | ||
| step is zero or if the step direction doesn't allow reaching stop from start. | ||
| impls: | ||
| - args: | ||
| - name: start | ||
| value: i64 | ||
| description: The starting value of the series | ||
| - name: stop | ||
| value: i64 | ||
| description: The ending value of the series (inclusive) | ||
|
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 there is an interesting spilit between
I guess either way is fine... Another problem is that some systems do support generating series for other numeric types (e.g., fp32, fp64, decimal) but we can add those later.
Member
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. We could add this as a function option which specifies if they include or exclude the stop value. What do others think? |
||
| - name: step | ||
| value: i64 | ||
| description: The increment between values | ||
| constant: true | ||
|
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. Is this
Member
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. https://substrait.io/expressions/scalar_functions/#argument-types
My understanding is that All that being said, postgresql does in fact accept column references in the arguments to generate_series (example here), so I think it makes sense to lift this restriction.
Member
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.
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. We shouldn't unnecessarily restrict ourselves to literals and this is another place we shouldn't IMO. Implementation may not support generic expression in certain places but we shouldn't prevent it from the spec. In fact, this is another potential information to encode in dialect whether an expression has some restrictions. |
||
| deterministic: true | ||
| sessionDependent: false | ||
| return: | ||
| names: | ||
| - value | ||
| struct: | ||
| types: | ||
| - i64 | ||
| - args: | ||
| - name: start | ||
| value: i32 | ||
| description: The starting value of the series | ||
| - name: stop | ||
| value: i32 | ||
| description: The ending value of the series (inclusive) | ||
| - name: step | ||
| value: i32 | ||
| description: The increment between values | ||
| constant: true | ||
| deterministic: true | ||
| sessionDependent: false | ||
| return: | ||
| names: | ||
| - value | ||
| struct: | ||
| types: | ||
| - i32 | ||
| - args: | ||
| - name: start | ||
| value: i64 | ||
| description: The starting value of the series | ||
| - name: stop | ||
| value: i64 | ||
| description: The ending value of the series (inclusive) | ||
| deterministic: true | ||
| sessionDependent: false | ||
| return: | ||
| names: | ||
| - value | ||
| struct: | ||
| types: | ||
| - i64 | ||
| - args: | ||
| - name: start | ||
| value: i32 | ||
| description: The starting value of the series | ||
| - name: stop | ||
| value: i32 | ||
| description: The ending value of the series (inclusive) | ||
| deterministic: true | ||
| sessionDependent: false | ||
| return: | ||
| names: | ||
| - value | ||
| struct: | ||
| types: | ||
| - i32 | ||
| - name: "unnest" | ||
| description: Expands a list literal into a set of rows, one row per element. | ||
|
benbellick marked this conversation as resolved.
Outdated
|
||
| impls: | ||
| - args: | ||
| - name: input | ||
| value: "list<T>" | ||
|
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. perhaps we can follow other extensions. for instance, in decimal rounding functions, we just write type rather than wrap it in a quote (i.e., decimal<P,S> rather than "decimal<P,S>"). Also, I'm wondering whether we can follow the same pattern as others (i.e., allow type expression).
Member
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. Ah yes, I am operating under the assumption that we will indeed need to allow type expressions. I altered the |
||
| description: The list to unnest | ||
| deterministic: true | ||
| sessionDependent: false | ||
| # Schema references type parameter T from list<T> | ||
| # The field type is derived from the list element type | ||
| return: | ||
|
Member
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. I have made this a draft PR again. I need to have a proper think about how we can represent this dynamic return type if we really want to have these functions be variadic 😰 |
||
| names: | ||
| - element | ||
| struct: | ||
| types: | ||
| - T | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -552,6 +552,51 @@ message ExpandRel { | |
| } | ||
| } | ||
|
|
||
| // Invokes a table-valued function that produces a relation (zero or more records). | ||
| // | ||
| // | ||
| // Table functions produce a table with either: | ||
| // - A schema that can be derived based on argument types (type-parameterized functions) | ||
| // - A schema that depends on runtime data (use derived: false) | ||
| // | ||
| // Future extensions may add an optional input field to support transformation | ||
| // table functions that operate on input relations. | ||
| message TableFunctionRel { | ||
|
Member
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. Is a more appropriate name for this In my mind there are two possible paths we can go down which determine the appropriate name.
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. From my biased view, it is better to categorize them... roughly
This gives way more modular and systematic way to describe a table function.
Member
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. Is your suggestion to have a different relation for each one of those? Or one shared relation? Maybe something like the following would be preferable... message TableFunctionRel {
...
oneof {
ScanLike scan_like = 1; // PR implements this one
//ProjectLike project_like = 2;
//JoinLike join_like = 3;
//GroupLike group_like = 4;
}
// implementations below
...
}
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 prefer |
||
| RelCommon common = 1; | ||
|
|
||
| // Points to a function_anchor defined in this plan, which must refer | ||
| // to a table function in the associated YAML file. Avoid using | ||
| // anchor/reference zero. | ||
| uint32 function_reference = 2; | ||
|
|
||
| // The arguments to be bound to the function. This must have exactly the | ||
| // number of arguments specified in the function definition from the YAML file, | ||
| // and the argument types must also match exactly: | ||
| // | ||
| // - Value arguments must be bound using FunctionArgument.value. | ||
| // Currently (0-input functions only), expressions must be constants | ||
|
benbellick marked this conversation as resolved.
Outdated
|
||
| // (literals or expressions evaluable without input data). | ||
| // - Type arguments must be bound using FunctionArgument.type. | ||
| // - Enum arguments must be bound using FunctionArgument.enum with a | ||
| // string that case-insensitively matches one of the allowed options. | ||
| repeated FunctionArgument arguments = 3; | ||
|
|
||
| // The derived fields indicates whether or not the YAML file produced the schema: | ||
|
Member
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. Every other function type has a message TableFunctionRel {
...
repeated FunctionOption options = 4;
...
}
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. For the sake of consistency, maybe it is worth it? I don't understand what this FunctionOption is for... (code archaeology time!)
Member
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. I think that these options are actually these bits of the yaml: %YAML 1.2
---
urn: extension:io.substrait:functions_logarithmic
scalar_functions:
-
name: "ln"
description: "Natural logarithm of the value"
impls:
- args:
- name: x
value: i64
options: # all of these parts below
rounding:
values: [ TIE_TO_EVEN, TIE_AWAY_FROM_ZERO, TRUNCATE, CEILING, FLOOR ]
on_domain_error:
values: [ NAN, "NULL", ERROR ]
on_log_zero:
values: [NAN, ERROR, MINUS_INFINITY]
return: fp64So given that, it would make sense to include them. |
||
| // - If true, the table_schema was produced purely from the type expressions in the | ||
| // YAML file + the types of the provided arguments | ||
| // - If false, the table_schema was produced by the plan producer | ||
| // | ||
| // This value is required to be true if and only if a schema is provided in the YAML | ||
| // definition of this 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. Can we simply have
Member
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. I wanted to be consistent with the behavior of message ScalarFunction {
...
// Must be set to the return type of the function, exactly as derived
// using the declaration in the extension.
Type output_type = 3;
...
}I figured that it makes the plans more explicit to always include the
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. It is fine to have |
||
| bool derived = 4; | ||
|
|
||
| // The schema of the output relation. This schema is required to match the implied schema | ||
| // by the YAML definition, if a schema is present in the definition. | ||
| NamedStruct table_schema = 5; | ||
|
|
||
| substrait.extensions.AdvancedExtension advanced_extension = 10; | ||
| } | ||
|
|
||
| // A relation with output field names. | ||
| // | ||
| // This is for use at the root of a `Rel` tree. | ||
|
|
@@ -581,6 +626,7 @@ message Rel { | |
| WriteRel write = 19; | ||
| DdlRel ddl = 20; | ||
| UpdateRel update = 22; | ||
| TableFunctionRel table_function = 23; | ||
| // Physical relations | ||
| HashJoinRel hash_join = 13; | ||
| MergeJoinRel merge_join = 14; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -106,6 +106,19 @@ message FunctionSignature { | |
| } | ||
| } | ||
|
|
||
| message Table { | ||
| repeated Argument arguments = 2; | ||
| repeated string name = 3; | ||
| Description description = 4; | ||
|
|
||
| bool deterministic = 7; | ||
| bool session_dependent = 8; | ||
|
|
||
| NamedStruct schema = 9; | ||
|
|
||
| repeated Implementation implementations = 10; | ||
|
Member
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. Other functions have variadic final argument logic like: message TableFunction {
...
oneof final_variable_behavior {
FinalArgVariadic variadic = 10;
FinalArgNormal normal = 11;
}
...
}However, I skipped it for now for simplicity. Is it necessary on a first pass?
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. Because you have unnest, I think it is actually makes sense to have variadic support. 😆
Member
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. I like the idea of introducing variadic argument handling, but as I understand it, it is a bit complicated in the current state to introduce variadics for things like So unlike the typical usage of variadic arguments for other function types, where the last argument is the same type, this usage is a bit more complicated and I'm not sure yet how it could be encoded. An approach is to instead keep the implementation as non-variadic and then introduce another tool to stitch relations together (something like a zip relation).
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. Indeed above case requires implicit type coercion rule which Substrait chose not to. So strictly speaking, your example is an invalid Substrait -- the user must have put cast (cough).
Member
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. Ah sorry I think I misunderstood the point you were making initially. I thought it was about wanting to call unnest across an arbitrary number of arrays all of potentially different types (e.g. If that is the case, then use it makes sense to me to include variadic handling. Postgresql does have some handling for
|
||
| } | ||
|
|
||
| message Description { | ||
| string language = 1; | ||
| string body = 2; | ||
|
|
||
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.
Do we want to say something about the default value of
step?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.
Would it be better to instead drop the implementations where
stepis missing? That way, we force the plans to be more explicit.